[m-rev.] Updated diff for deep profiling.

[Zoltan Somogyi]

On 18-May-2001, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> Those options should be documented later, in the section which says

Done.

> I suggest formatting that as
> 		printf("watch addr %p: %ld (0x%lx)\n", MR_watch_addr,
> 		                       ^^^^^^^^^^^

Done.

> > +#if 0
> > +	if (MR_sp >= &MR_CONTEXT(MR_ctxt_detstack_zone)->min[300]) {
> >  		for (i = 321; i < 335; i++) {
> >  			MR_printdetslot_as_label(i);
> >  		}
> >  	}
> > +#endif
> 
> Why is that code #if'd out?
> There should be a comment explaining.

That code is long obsolete. I deleted it.

> > +	fp = fopen("Deep.data", "w+");
> 
> That should be "wb+", I think.

Done.

> > +	rewind(fp);
> > +	MR_write_out_id_string(fp);
> > +	MR_write_fixed_size_int(fp, MR_call_site_dynamic_table->last_id);
> > +	MR_write_fixed_size_int(fp, MR_call_site_static_table->last_id);
> > +	MR_write_fixed_size_int(fp, MR_proc_dynamic_table->last_id);
> > +	MR_write_fixed_size_int(fp, MR_proc_static_table->last_id);
> > +	(void) fclose(fp);
> 
> You should not ignore the value of fclose().
> 
> Also instead of using rewind(), you should use `fseek(stream, 0L,
> SEEK_SET)' and check the return value.

Done.

> Otherwise the code will end up ignoring disk full errors,
> which can be quite common.
> 
> > +static void
> > +MR_write_out_id_string(FILE *fp)
> > +{
> > +	/* This string must match id_string deep/deep.io.m */
> > +	const char	*id_string = "Mercury deep profiler data";
> > +	int		i;
> > +
> > +	for (i = 0; id_string[i] != '\0'; i++) {
> > +		putc(id_string[i], fp);
> > +	}
> 
> Just use `fputs(id_string, fp)'.

Done.

> > +	/*
> > +	** This shouldn't really happen except that we don't have correct
> > +	** handling of nondet pragma_foreign_code yet.
> > +	*/
> > +
> > +	if (ptr == NULL) {
> > +		return;
> > +	}
> 
> Delete the blank line after the comment.

I moved it into the block instead.

> > +	i = 0;
> > +	do {
> > +		pieces[i] = num & 0x7f;
> > +		num = num >> 7;
> > +		i++;
> > +	} while (num != 0);
> > +
> > +	i--;
> > +
> > +	while (i > 0) {
> > +		putc(pieces[i--] | 0x80, fp);
> > +	}
> > +	putc(pieces[0], fp);
> > +}
> 
> A comment here explaining the encoding would be helpful.
> 
> There's a lot of magic numbers in this code.
> The code would be easier to understand if these were named.

I took Tom's advice instead.

> > +static MR_ProfilingHashTable *
> > +MR_create_hash_table(int size)
> > +{
> > +	MR_ProfilingHashTable *ptr;
> > +	ptr = MR_NEW(MR_ProfilingHashTable);
> > +	ptr->length = size;
> > +	ptr->last_id = 0;
> > +	ptr->nodes = MR_NEW_ARRAY(MR_ProfilingHashNode *, size);
> > +
> > +	return ptr;
> > +}
> 
> Why does this code define it's own hash table routines,
> rather than using runtime/mercury_hash_table.{c,h}?
> 
> If there is a good reason, it should be documented here.

The reason is efficiency; I documented what overhead this code avoids
that mercury_hash_table.c incurs.

> > +	hash = ((unsigned int) ptr >> 2) % table->length;
> 
> That line of code is duplicated elsewhere.
> It is worth abstracting out into a macro, e.g. MR_hash_pointer(), IMHO.

Done.

> > Index: runtime/mercury_deep_profiling.h
> ...
> > +  #define MR_maybe_move_to_front(csdlist, prev, pd, csn)		\
> > +	if (prev != NULL) {						\
> > +		prev->MR_csdlist_next = csdlist->MR_csdlist_next;	\
> > +		csdlist->MR_csdlist_next = (MR_CallSiteDynList *)	\
> > +			pd->MR_pd_call_site_ptr_ptrs[(csn)];		\
> > +		pd->MR_pd_call_site_ptr_ptrs[(csn)] =			\
> > +			(MR_CallSiteDynamic *) csdlist;			\
> > +	}
> 
> That should be wrapped inside `do { ... } while (0)'.
> 
> > +/* If these are volatile, a lot of other things must be too */
> > +extern	MR_CallSiteDynamic		*MR_current_call_site_dynamic;
> > +extern	MR_CallSiteDynamic		*MR_next_call_site_dynamic;
> > +extern	MR_CallSiteDynList		**MR_current_callback_site;
> > +extern	MR_CallSiteDynamic		*MR_root_call_sites[];
> > +
> > +extern	volatile bool			MR_inside_deep_profiling_code;
> > +extern	unsigned long			MR_quanta_inside_deep_profiling_code;
> > +extern	unsigned long			MR_quanta_outside_deep_profiling_code;
> 
> The last two globals there are modified by a signal handler,
> and so should be `volatile'.  So should the `MR_own_quanta'
> field of MR_ProfilingMetrics_Struct.
> 
> That's all that needs to be volatile, I'm pretty sure.
> I think the comment above about volatile above is not correct
> and should be deleted.
> 
> > Index: scripts/mdprof.in
> > ===================================================================
> > RCS file: mdprof.in
> > diff -N mdprof.in
> > --- /dev/null	Fri Dec  1 02:25:58 2000
> > +++ mdprof.in	Thu May 17 16:41:43 2001
> > @@ -0,0 +1,8 @@
> > +#!/bin/sh
> > +# This should set up PATH to include the directory containing the installed
> > +# mdprof_cgi and mprof_server programs. This should allow this shell script
> > +# to find the right version of mdprof_cgi, and it should allow the mdprof_cgi
> > +# process to find the right mdprof_server.
> > +PATH=@PREFIX@/bin:$PATH
> > +export PATH
> > +exec mdprof_cgi "$@"
> 
> "$@" is not portable, at least for the case when $# is 0
> (I'm not sure if that can arise for this script).

Yes, and usually will. The web server passes the query to mdprof via an
environment variable.

> A more portable alternative is to use ${1+"$@"} instead.

Actually, I deleted the "$@", and made both mdprof and mdprof_cgi.m report an
error if their command line is not empty.

> > +# Don't let any single test run for more than ten minutes.
> > +ulimit -t 600
> > +
> >  if test "$subdir_failures" = ""
> >  then
> >  	subdir_status=0
> 
> That change is good, but should be committed separately.
> It was not in the log message.

Done.

I found out the need for it the hard way :-(

> > +EXCEPTION_PROGS = \
> > +	test_exceptions.m \
> > +	test_exceptions_func.m \
> > +	test_try_all.m \
> > +	test_uncaught_exception.m \
> > +	tricky_try_store.m
> ...
> > +# Deep profiling grades cannot yet handle catching exceptions.
> 
> The test_uncaught_exception.m test case needn't be disabled
> in deep profiling grades.

Yes, it must, since the runtime system implicitly "catches" exceptions
when it calls the library finalizer, and that call won't be set up properly
if the program exits with an exception.

> Also probably there should be an XXX next to the comment there.

It is not a bug, merely an unimplemented feature :-)

> > -#ifdef MR_USE_DECLARATIVE_DEBUGGER
> > -
> >  /*
> >  ** When in declarative debugging mode, the internal debugger calls
> >  ** MR_trace_decl_debug for each event.  
> > @@ -45,5 +43,4 @@
> >  #define MR_TRACE_STATUS_FAILED		(MR_Word) 1
> >  #define MR_TRACE_STATUS_UNDECIDED	(MR_Word) 2
> >  
> > -#endif  /* MR_USE_DECLARATIVE_DEBUGGER */
> >  #endif	/* MERCURY_TRACE_DECLARATIVE_H */
> 
> That change is not mentioned in the log message and looks unrelated.

It was unintentional; must have been an artifact of the move across branches.

> > +# GETOPT_FLAGS suppresses warnings about the prototype of getopt
> > +GETOPT_FLAGS=-D__GNU_LIBRARY__ 
> > ...
> >  .c:
> > -	$(MGNUC) $(GRADEFLAGS) $(ALL_MGNUCFLAGS) -o $@ $< $(GETOPT_SRC)
> > +	$(MGNUC) $(GRADEFLAGS) $(ALL_MGNUCFLAGS) $(GETOPT_FLAGS) -o $@ $< $(GETOPT_SRC)
> 
> That change is unrelated and should be committed separately.

Done.

Zoltan.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------