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

[Fergus Henderson]

On 17-May-2001, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> +++ runtime/mercury_conf_param.h	2001/05/12 15:27:16
> +**
> +** MR_DEEP_PROFILING_PORT_COUNTS.
> +** Enables deep profiling of port counts.
> +**
> +** MR_DEEP_PROFILING_TIMING.
> +** Enables deep profiling of time.
> +**
> +** MR_DEEP_PROFILING_MEMORY.
> +** Enables deep profiling of memory usage.
>  */

The section where the documentation for those was added says

	/*
	** Documentation for configuration parameters which can be set on the
	** command line via `-D'.
	*/

but the code below

> +#ifdef	MR_DEEP_PROFILING
> +  /* this is the default set of measurements in deep profiling grades */
> +  #define MR_DEEP_PROFILING_PORT_COUNTS
> +  #ifndef MR_DEEP_PROFILING_PERF_TEST
> +    #define MR_DEEP_PROFILING_TIMING
> +    #define MR_DEEP_PROFILING_MEMORY
> +  #endif
> +#else
> +  #undef  MR_DEEP_PROFILING_PORT_COUNTS
> +  #undef  MR_DEEP_PROFILING_TIMING
> +  #undef  MR_DEEP_PROFILING_MEMORY
> +#endif

means that any command-line setting of those options will be ignored.

Those options should be documented later, in the section which says

	** Configuration parameters whose values are determined by the settings
	** of other configuration parameters.  These parameters should not be
	** set on the command line.

and instead the MR_DEEP_PROFILING and MR_DEEP_PROFILING_PERF_TEST
options should be documented in the earlier section.

> Index: runtime/mercury_debug.c
...
> @@ -276,6 +297,20 @@
>  	printf("%-9s", "sp:");      MR_printdetstack(MR_sp);
>  
>  	MR_print_ordinary_regs();
> +
> +	if (MR_watch_addr != NULL) {
> +		printf("watch addr %p: %lx %ld\n", MR_watch_addr,
> +			(long) *MR_watch_addr, (long) *MR_watch_addr);
> +	}

I suggest formatting that as

		printf("watch addr %p: %ld (0x%lx)\n", MR_watch_addr,
		                       ^^^^^^^^^^^

It's easier to read if the hex version is preceded with 0x.

> -	if (MR_sp >= &MR_CONTEXT(detstack_zone)->min[300]) {
> +#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.

> Index: runtime/mercury_deep_profiling.c
...
> +void
> +MR_write_out_profiling_tree(void)
> +{
> +	int			i;
> +	MR_ProfilingHashNode	*n;
> +	MR_Proc_Id		*pid;
> +	int			root_pd_id;
> +	FILE			*fp;
> +
> +	fp = fopen("Deep.data", "w+");

That should be "wb+", I think.

> +	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.

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)'.

> +static void
> +MR_write_out_proc_dynamic(FILE *fp, const MR_ProcDynamic *ptr)
> +{
> +	int	i;
> +	int	pd_id;
> +	int	ps_id;
> +	bool	already_written;
> +
> +	/*
> +	** 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.

> +static void
> +MR_write_num(FILE *fp, unsigned long num)
> +{
> +	unsigned char	pieces[sizeof(unsigned long) * 8 / 7 + 1];
> +	int		i;
> +
> +#ifdef	MR_DEEP_PROFILING_DETAIL_DEBUG
> +	fprintf(debug_fp, "num: %ld\n", num);
> +#endif
> +
> +	MR_deep_assert((int) num >= 0);
> +
> +	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.

> +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.

> +static bool
> +MR_hash_table_insert(MR_ProfilingHashTable *table, const void *ptr,
> +	int *id, bool *already_written, bool init_written)
> +{
> +	int			hash;
> +	MR_ProfilingHashNode	*node;
> +
> +	if (ptr == NULL) {
> +		MR_fatal_error("NULL ptr in MR_hash_table_insert");
> +	}
> +
> +	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.

> 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).

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

> Index: tests/debugger/runtests
> ===================================================================
> RCS file: /home/mercury1/repository/tests/debugger/runtests,v
> retrieving revision 1.10
> diff -u -b -r1.10 runtests
> --- tests/debugger/runtests	2001/05/15 07:10:41	1.10
> +++ tests/debugger/runtests	2001/05/15 07:17:08
> @@ -17,6 +17,9 @@
>  
>  . ../subdir_runtests
>  
> +# 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.

> Index: tests/hard_coded/exceptions/Mmakefile
...
> -PROGS = test_exceptions.m test_uncaught_exception.m test_exceptions_func.m \
> -	test_try_all.m tricky_try_store.m
> +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.
> +
> +ifneq "$(findstring profdeep,$(GRADE))" ""
> +	PROGS=$(EXCEPTION_PROGS)
> +else
> +	PROGS=
> +endif

The test_uncaught_exception.m test case needn't be disabled
in deep profiling grades.

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

> +++ trace/mercury_trace_declarative.h	2001/05/03 06:41:39
> @@ -11,8 +11,6 @@
>  #include "mercury_trace.h"
>  #include "mercury_trace_internal.h"
>  
> -#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.

> cvs diff: Diffing util
> Index: util/Mmakefile
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/util/Mmakefile,v
> retrieving revision 1.11
> diff -u -b -r1.11 Mmakefile
> --- util/Mmakefile	2001/04/08 08:59:32	1.11
> +++ util/Mmakefile	2001/05/03 06:41:39
> @@ -24,6 +24,8 @@
>  PROGFILENAMES=$(PROGS:%=%$(EXT_FOR_EXE))
>  SRC=$(PROGS:%=%.c)
>  
> +# GETOPT_FLAGS suppresses warnings about the prototype of getopt
> +GETOPT_FLAGS=-D__GNU_LIBRARY__ 
>  GETOPT_SRC=$(RUNTIME_DIR)/GETOPT/getopt.c $(RUNTIME_DIR)/GETOPT/getopt1.c
>  
>  # mkinit.c needs `struct stat'
> @@ -34,7 +36,7 @@
>  all:	$(PROGS)
>  
>  .c:
> -	$(MGNUC) $(GRADEFLAGS) $(ALL_MGNUCFLAGS) -o $@ $< $(GETOPT_SRC)
> +	$(MGNUC) $(GRADEFLAGS) $(ALL_MGNUCFLAGS) $(GETOPT_FLAGS) -o $@ $< $(GETOPT_SRC)
>  
>  tags:
>  	ctags $(SRC)

That change is unrelated and should be committed separately.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
                                    |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------