[m-dev.] for review: deep profiling changes [part 3/3]

[Fergus Henderson]

On 26-Feb-2000, Thomas Conway <conway at cs.mu.OZ.AU> wrote:
> Index: runtime/mercury_wrapper.h
> :- type callsite
> 	--->	fo(first_order_call)
> 	;	ho(higher_order_call)
> 	;	cm(class_method_call)
> 	.

I think it might be clearer if you expand those acronyms:

	s/fo/first_order/
	s/ho/higher_order/
	s/cm/class_method/

> :- pred profiling__add_callsite(callsite, string, int, code_info, code_info).
> :- mode profiling__add_callsite(in, out, out, in, out) is det.
> 
> profiling__add_callsite(CallSite, UpdateFunc, SlotNum) -->
> 	profiling__my_ppid(CallerPPId),
> 	code_info__get_scc_info(SCCInfo0),
> 	{ SCCInfo0 = scc_info(ProcSCCs, SCCCallSites0) },
> 	{ map__lookup(ProcSCCs, CallerPPId, CallerSCC) },
> 	( { map__search(SCCCallSites0, CallerSCC, SCCData0a) } ->
> 		{ SCCData0 = SCCData0a }
> 	;
> 		{ SCCData0 = scc_data(0, [], 0, [], 0, [],
> 			will_not_call_mercury) }
> 	),
> 	{ SCCData0 = scc_data(A0, As0, B0, Bs0, C0, Cs0, Rec) },
> 	(
> 		{ CallSite = fo(ThisSite) },
> 		{ A is A0 + 1 },
> 		{ list__append(As0, [ThisSite], As) },
> 		{ UpdateFunc = "MR_fo_call" },
> 		{ B = B0, Bs = Bs0 },
> 		{ C = C0, Cs = Cs0 },
> 		{ SlotNum = A0 }
> 	;
> 		{ CallSite = ho(ThisSite) },
> 		{ B is B0 + 1 },
> 		{ list__append(Bs0, [ThisSite], Bs) },
> 		{ UpdateFunc = "MR_ho_call" },
> 		{ A = A0, As = As0 },
> 		{ C = C0, Cs = Cs0 },
> 		{ SlotNum = B0 }
> 	;
> 		{ CallSite = cm(ThisSite) },
> 		{ C is C0 + 1 },
> 		{ list__append(Cs0, [ThisSite], Cs) },
> 		{ UpdateFunc = "MR_cm_call" },
> 		{ A = A0, As = As0 },
> 		{ B = B0, Bs = Bs0 },
> 		{ SlotNum = C0 }
> 	),
> 	{ SCCData = scc_data(A, As, B, Bs, C, Cs, Rec) },
> 	{ map__set(SCCCallSites0, CallerSCC, SCCData, SCCCallSites) },
> 	{ SCCInfo = scc_info(ProcSCCs, SCCCallSites) },
> 	code_info__set_scc_info(SCCInfo).

I think that code would be clearer if you used record syntax for the scc_data
type.

> :- type scc_id_data
> 	--->	scc_id_data(int, list(comp_gen_c_data), list(comp_gen_c_var)).

Some comments here explaining this data type and the meaning of
its fields would be helpful.

> profiling__generate_scc_ids(ModuleInfo0, ModuleInfo, Data, Vars) :-
> 	module_info_get_scc_info(ModuleInfo0, SCCInfo),
> 	module_info_get_cell_count(ModuleInfo0, InitialCount),
> 	SCCInfo = scc_info(_ProcSCCs, SCCCallSites),
> 	InitialSCCIdData = scc_id_data(InitialCount, [], []),
> 	map__foldl(lambda([SCCId::in, SCCData::in,
> 			SCCIdData0::in, SCCIdData7::out] is det, (
> 		SCCId = scc_id(ModuleName, SCCNum),
> 		SCCData = scc_data(NFO, FOCalls, NHO, HOCalls, NCM, CMCalls,
> 				_Rec),
> 		(
> 			FOCalls = [],
> 			FOVector = yes(const(int_const(0))),
> 			SCCIdData2 = SCCIdData0
> 		;
> 			FOCalls = [_|_],
> 			list__map_foldl(first_order_scc_ids(ModuleInfo0),
> 				FOCalls, FOCallRvals, SCCIdData0, SCCIdData1),
> 			profiling__scc_id_data_next_cell(LabNum1, SCCIdData1,
> 				SCCIdData2),
> 			FOVector = yes(create(0, FOCallRvals, uniform(no),
> 				must_be_static,
> 				LabNum1, "first order call data", no))
> 		),
> 		(
> 			HOCalls = [],
> 			HOVector = yes(const(int_const(0))),
> 			SCCIdData2 = SCCIdData4
> 		;
> 			HOCalls = [_|_],
> 			list__map_foldl(higher_order_scc_ids(ModuleInfo0),
> 				HOCalls, HOCallRvals, SCCIdData2, SCCIdData3),
> 			profiling__scc_id_data_next_cell(LabNum2, SCCIdData3,
> 				SCCIdData4),
> 			HOVector = yes(create(0, HOCallRvals, uniform(no),
> 				must_be_static,
> 				LabNum2, "higher order call data", no))
> 		),
> 		(
> 			CMCalls = [],
> 			CMVector = yes(const(int_const(0))),
> 			SCCIdData6 = SCCIdData4
> 		;
> 			CMCalls = [_|_],
> 			list__map_foldl(class_method_scc_ids(ModuleInfo0),
> 				CMCalls, CMCallRvals, SCCIdData4, SCCIdData5),
> 			profiling__scc_id_data_next_cell(LabNum3, SCCIdData5,
> 				SCCIdData6),
> 			CMVector = yes(create(0, CMCallRvals, uniform(no),
> 				must_be_static,
> 				LabNum3, "class method data", no))
> 		),

Very similar code is repeated three times there; I think it would be
clearer if you extracted that out into a subroutine.

> :- pred profiling__scc_id_data_next_cell(int, scc_id_data, scc_id_data).
> :- mode profiling__scc_id_data_next_cell(out, in, out) is det.
> 
> profiling__scc_id_data_next_cell(Cell, Data0, Data) :-
> 	Data0 = scc_id_data(Cell, Structs, Vars),
> 	Cell1 is Cell + 1,
> 	Data = scc_id_data(Cell1, Structs, Vars).

I think you should be returning Cell1 rather than Cell as the output
argument here.

Also you should keep the naming convention consistent:
	s/Cell/Cell0/g
	s/Cell1/Cell/g

> ===================================================================
> New file: runtime/mercury_prof_deep.c
...
> MR_Count		MR_prof_num_sigs=0;

s/=/ = /

> static const MR_SCCId *
> MR_prof_get_scc_id(const MR_Stack_Layout_Proc_Id *p)
> {
> 	const MR_SCCId *sccid;
> 
> 	sccid = p->MR_proc_user.MR_user_scc_id;
> 	if (MR_tag((const Word) sccid))
> 	{

The `{' should be on the same line as the `if'.

Also it might be a little clearer to write that as `if (... != 0)'.

> MR_ProcCallProfile *
> MR_prof_ensure_fo_call_slot(MR_SCCInstance *scc, int site_num)
> {
> 	MR_InterCallProfile *tmp;
> 
> 	assert((scc != NULL) && (scc->fo_calls != NULL));
> 
> 	if (scc->fo_calls[site_num] != NULL)
> 		return &(scc->fo_calls[site_num]->prof);
> 	
> 	tmp = (MR_InterCallProfile *)
> 		MR_malloc(sizeof(MR_InterCallProfile));

It would be very helpful to have some comments describing
what this function is supposed to do (the same applies to
most of the other functions in this source file).

Can this function be invoked from a profiling signal
handler?  If so, you should you be using MR_prof_malloc()
here, rather than MR_malloc().

Also I would recommend using MR_NEW() or MR_PROF_NEW(), e.g.

 	tmp = MR_PROF_NEW(MR_InterCallProfile);

rather than using MR_malloc() or MR_prof_malloc() directly.

The preceding two comments apply to every call to MR_malloc()
in this file.

> MR_ProcCallProfile *
> MR_prof_proc_const_call(MR_MultiCallProfile **call_list, MR_Closure *closure)
> {
> 	MR_Stack_Layout_Proc_Id *proc_id;
> 
> 	assert((call_list != NULL) && (closure != NULL));
> 
> 	proc_id = closure->MR_closure_layout->proc_id;
> 	return MR_prof_special_proc_const_call(call_list, closure->MR_closure_code, proc_id);

That line exceeds 80 columns.

> MR_ProcCallProfile *
> MR_prof_special_proc_const_call(MR_MultiCallProfile **call_list, Code *code_ptr, const MR_Stack_Layout_Proc_Id *proc_id)


Likewise.

> 	** This should probably use a move-to-front list to exploit locality.

I think that comment is worth an "XXX".

> 	*/
> 	for(tmp = *call_list; tmp; tmp = tmp->next)
> 	{

The `{' should be on the same line as the `for'.

> 	/*
> 	** If we didn't find the proc in the list, then allocate a new
> 	** HigherOrderCallProfile and stick it on the front of the list.
> 	*/
> 
> 	tmp = (MR_HigherOrderCallProfile *)
> 		MR_malloc(sizeof(MR_HigherOrderCallProfile));

See my earlier comments about MR_prof_malloc() and MR_NEW()/MR_PROF_NEW().

> MR_SCCInstance *
> MR_prof_allocate_scc_instance(const MR_SCCId *child_scc_id)
> {
> 	static int	scc_num=1;
> 	MR_SCCInstance *child;
> 	size_t nbytes;
> 
> 	if (child_scc_id != NULL)
> 	{
> 		child = (MR_SCCInstance *)
> 			MR_malloc(sizeof(MR_SCCInstance));
> 		child->scc = child_scc_id;
> 		child->scc_num = scc_num++;
> 		if (child_scc_id->num_fo_calls > 0)
> 		{
> 			nbytes = sizeof(MR_InterCallProfile *)
> 					* child_scc_id->num_fo_calls;
> 			child->fo_calls = (MR_InterCallProfile **)
> 				MR_malloc(nbytes);
> 			memset(child->fo_calls, 0, nbytes);
> 
> 		} else
> 			child->fo_calls = NULL;
> 		if (child_scc_id->num_ho_calls > 0)
> 		{
> 			nbytes = sizeof(MR_HigherOrderCallProfile *)
> 					* child_scc_id->num_ho_calls;
> 			child->ho_calls = (MR_HigherOrderCallProfile **)
> 				MR_malloc(nbytes);
> 			memset(child->ho_calls, 0, nbytes);
> 		} else
> 			child->ho_calls = NULL;
> 		if (child_scc_id->num_cm_calls > 0)
> 		{

The `{'s here should be all the same line as the corresponding `if'.

Likewise in a number of other places; please check the placement of all the
`{'s in this source file.

> /*
> **	prof_time_profile:
> **		Signal handler to be called whenever a profiling signal is
> **		received. Saves the current code address into a hash table.
> **		If the address already exists, it increments its count.
> **	XXX
> */

The comment there looks incomplete/incorrect.

> void
> prof_deep_time_profile(int signum);
> 
> void
> prof_deep_time_profile(int signum)
> {

Why is there a declaration for this function
immediately before the definition of the function?

> #ifdef MR_PROFILE_DEEP
> 
> static MR_SCCInstance *instance_stack[1000];

Eek, a fixed limit! ;-)

It would be better to give a name to that constant,
and to document what quantity it is limiting.

> static int isp=0;

s/=/ = /

The variable name `isp' is not clear;
I suggest you replace it with something a bit
more descriptive.

Why does this need to be a global variable?
I think it would probably be much nicer to declare
this as a local variable and pass its address
to the places that need to modify it, rather
than defining it as a file-scope static variable.

> #define MR_PPID_TABLE_SIZE	5021

Why 5021?

Also a comment here explaining what the PPID table was
would help.

> static void MR_prof_dump_proc_call_profile(FILE *fp, MR_ProcCallProfile *p)

The function name should be at the start of a new line.

> static void MR_prof_dump_string(FILE *fp, const char *s)
> {

Likewise here and in several other places in this source file.

> 	int i,l;
> 	l = strlen(s);
> 	MR_prof_write_word(fp, l);
> 	for(i = 0; i < l ; i++)
> 		MR_prof_write_word(fp, s[i]);

I suggest

	s/,l/, l/
	s/l/len/g

> static void MR_prof_dump_ppid(FILE *fp, const MR_Stack_Layout_Proc_Id *ppid)
> {
> 	int num;
> 	unsigned ind;
> 	MR_PPIdNode *tmp;
> 
> 	if (ppid)
> 	{
> 		ind = (((const unsigned) ppid) >> 3) % MR_PPID_TABLE_SIZE;
> 		tmp = MR_prof_ppid_table[ind];
> 		num = -1;
> 		while (tmp)
> 		{
> 			if (ppid == tmp->ppid)
> 			{
> 				num = tmp->ppid_num;
> 				break;
> 			} else {
> 				tmp = tmp->next;
> 			}
> 		}
> 		if (num < 0)
> 		{
> 			tmp = (MR_PPIdNode *)
> 					MR_malloc(sizeof(MR_PPIdNode));
> 			tmp->ppid = ppid;
> 			tmp->ppid_num = MR_prof_ppid_counter++;
> 			tmp->next = MR_prof_ppid_table[ind];
> 			MR_prof_ppid_table[ind] = tmp;
> 			num = tmp->ppid_num;
> 		}
> 	} else {
> 		num = 0;
> 	}

What is this code supposed to be doing?  Some comments here
would certainly help.

Is this code doing insertion into a hash table?
If so, you should use the macros defined in
mercury_hash_table.h.

> static void MR_prof_dump_ppid_table(FILE *fp)
> {
> 	int i;
> 	MR_PPIdNode *tmp;
> 
> 	for (i = 0; i < MR_PPID_TABLE_SIZE; i++)
> 	{
> 		tmp = MR_prof_ppid_table[i];
> 		while (tmp)
> 		{
> 			MR_prof_dump_ppid_defn(fp, tmp->ppid_num, tmp->ppid);
> 			tmp = tmp->next;
> 		}
> 	}
> 	MR_prof_write_word(fp, 0); /* end marker */
> }

You should clearly document somewhere the format of this data file.

> static void MR_prof_dump_SCCid_table(FILE *fp)
> {
> 	int i,j;
> 	MR_SCCIdNode *tmp;
> 
> 	for (i = 0; i < MR_PPID_TABLE_SIZE; i++)
> 	{
> 		tmp = MR_prof_SCCid_table[i];
> 		while (tmp)
> 		{
> 			assert(tmp->SCCid_num != 0);
> 			MR_prof_write_word(fp, tmp->SCCid_num);
> 			MR_prof_output_call_site_list(fp,
> 				tmp->SCCid->num_fo_calls,
> 				tmp->SCCid->fo_calls);
> 			MR_prof_output_call_site_list(fp,
> 				tmp->SCCid->num_ho_calls,
> 				tmp->SCCid->ho_calls);
> 			/*
> 			MR_prof_output_call_site_list(fp,
> 				tmp->SCCid->num_cm_calls,
> 				tmp->SCCid->cm_calls);
> 			*/

You should explain why that code is commented out.

> void
> MR_prof_output_deep_tables(void)
> {
> 	FILE *fp;
> 
> 	fp = checked_fopen("Prof.data", "create", "w");

Shouldn't that be "wb" (binary) rather than "w" (text)?

> #define MR_PROF_USE_7BIT_ENCODING
> 
> #ifdef MR_PROF_USE_7BIT_ENCODING

That configuration macro should be documented in mercury_conf_param.h.

> /*
> ** 7bit encoding is an arbitary length encoding that uses the topmost
> ** bit of each byte to mark whether there are any more bits to come.
> ** The 7bit groups are written least significant first.

s/arbitary/arbitrary/

> void
> MR_prof_write_word(FILE *fp, Word w)
> {
> 	Word tmp;
> 
> 	do
> 	{
> 		tmp = w & 0x7f;
> 		w = w >> 7;
> 		if (w != 0)
> 			tmp = tmp | 0x80; 
> 		fputc(tmp, fp);
> 	} while (w != 0);
> }

You should check the return value of fputc().

Also it would be better to declare `tmp' as
`unsigned char' rather than `Word', and to
name it `next_byte' rather than `tmp'.

> Word
> MR_prof_read_word(FILE *fp, int *eof_marker)
> {
> 	Word w = 0;
> 	int c, i=0;

I suggest declaring `i' on a new line, and s/=/ = /

> 	while ((c = fgetc(fp)) != EOF)
> 	{
> 		w = w | ((c & 0x7f) << i);

Here you need to cast (c & 0x7f) to `Word',
otherwise it will do the left shift in type `int'
rather than type `Word'.


> 		if (!(c & 0x80))
> 			break;
> 		i+=7;

s/+=/ += /

> 	}
> 
> 	/*
> 	if (i+7 > sizeof(Word))
> 		fatal_error("MR_prof_read_word: overflow");
> 	*/
> 
> 	*eof_marker = (c == EOF);
> 
> 	return w;
> }
> 
> #elif MR_PROF_USE_MULTI_ENCODING
> 
> #else

I think you probably want 

	#error "MR_PROF_USE_MULTI_ENCODING not yet implemented"

there.

> #error No encoding specified

I think that for at least some [broken?] C compilers,
you need to enclose the message there in double quotes:

	#error "No encoding specified"

> New file: runtime/mercury_prof_deep.h
...
> #include "mercury_types.h"		/* for `Word' */
> #include "mercury_stack_layout.h"	/* for `MR_Stack_Layout_Entry *' */
> #include "mercury_ho_call.h"		/* for `MR_Closure *' */

You should also #include "mercury_std.h", for MR_PASTE2().

> #define MR_IF_PROFILE_DEEP(x)	x
> 
> /*
> ** The SCCId structures are generated as part of the static data
> ** along with the stack-layout data.
> */
> 
> typedef struct MR_SCC_ID {
> 	const int			num_fo_calls;
> 	const struct MR_CALL_SITE	**fo_calls;
> 	const int			num_ho_calls;
> 	const struct MR_CALL_SITE	**ho_calls;
> 	const int			num_cm_calls;
> 	const struct MR_CALL_SITE	**cm_calls;
> } MR_SCCId;

Some comments here explaining what the fields represent
would help.  The abbreviations are a little cryptic.

> #define MR_MAKE_SCC_ID(nm, focs, hocs, cmcs)				    \

It might help to document what the parameters to this macro
are supposed to be.  Likewise for the five or so macros that
follow it.

> static int MR_prof_debugging_dummy;

What's this for?
It's not a good idea to define static variables in header files.
I think you may have left that in by mistake.

----------

OK, a few remaining comments:

	- MR_PROFILE_DEEP should be documented in
	  runtime/mercury_conf_param.h or runtime/mercury_conf.h.in

	- You should modify runtime/mercury_grade.h to
	  include MR_PROFILE_DEEP in the mangled grade identifier
	  (I think -- it doesn't make sense to mix part of the
	  code compiled with deep profiling and part of it without,
	  does it?)
	
	- I think the stuff for handling the --deep-profiling option
	  and the `.profdeep' grade component is not complete. 
	  In particular, you need to modify scripts/mgnuc.in and
	  scripts/ml.in to handle them.  There may perhaps be
	  other places -- it's worth checking all the files listed
	  in the comment in runtime/mercury_grade.h.

Apart from that, this change looks great -- thanks for doing the
work to get it to this point.  There's a lot of minor issues that
should be pretty trivial to fix, and the new code could be better
documented, but apart from that it all looks good.

Cheers,
	Fergus.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>  |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3        |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------