[m-rev.] for review: stack frame size stats

[Fergus Henderson]

On 21-Dec-2001, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> runtime/mercury_conf.h.in:
> 	Include MR_INT_LEAST{16,32}_{MAX,UMAX} among the macros whose values
> 	are determined by autoconfiguration.

For consistency with C99, you should name that MR_UINT_LEAST<N>_MAX
rather than MR_INT_LEAST<N>_UMAX.                 ^

> +++ mercury_dword.h	Fri Dec 21 16:51:46 2001
> @@ -0,0 +1,120 @@
> +  typedef unsigned MR_INT_LEAST64_TYPE MR_dword;

You should use `MR_uint_least64_t' rather than `unsigned MR_INT_LEAST64_TYPE'
(this may require including mercury_types.h).

Also type names should start with the `MR_' prefix and then a capital letter,
so s/MR_dword/MR_Dword/
(or s/MR_dword/MR_DWord/ if you prefer).

> +#elif MR_INT_LEAST32_TYPE
...
> +#else /* not MR_INT_LEAST32_TYPE */
> +  #error "mercury_dword.h: neither MR_INT_LEAST64_TYPE nor MR_INT_LEAST32_TYPE"
> +#endif /* not MR_INT_LEAST32_TYPE */

I think it would be a good idea to test this code in some configuration
that doesn't support 64-bit integers (e.g. by compiling with `lcc' on x86),
especially since it contains at least one bug: that should be
`#elif defined(MR_INT_LEAST32_TYPE)'.

In fact, it would be better to change the `#elif' to just `#else',
since `defined(MR_INT_LEAST32_TYPE)' will always be true.
The existence of a >=32-bit type is guaranteed by the C standard,
and we already assume it elsewhere, e.g. in mercury_types.h.

> +  /*
> +  ** oh well, guess we have to do it the hard way :-(
> +  */
> +
> +  typedef struct MR_dword
> +  {
> +	unsigned MR_INT_LEAST32_TYPE	MR_dword_low;
> +	unsigned MR_INT_LEAST32_TYPE	MR_dword_high;
> +  } MR_dword;

Use MR_uint_least32_t.

> +  #include <limits.h>

The #include should go at the top of the file.

> +  #define MR_increment_dword(dword, inc) 				\
> +	do { 								\
> +		unsigned long	old_low_word = (dword).low_word;	\

`unsigned long' is probably the wrong type to use there.
I think you want `MR_uint_least32_t'.

> +  #define MR_increment_dword_tmp(dword, inc, low_tmp)			\
> +	( 								\
> +		low_tmp = (dword).MR_dword_low,				\
> +		(dword).MR_dword_low += (inc),				\
> +		((dword).MR_dword_low < low_tmp) ?			\
> +			(dword).MR_dword_high += 1 : (void) 0		\
> +	)

s/low_tmp/(low_tmp)/g

> +  #define MR_add_two_dwords(src_dest_dword, src_dword) 			\
> +	do { 								\
> +		unsigned long	old_low_word = (src_dest_dword).low_word; \

Use `MR_uint_least32_t'.

> +void
> +MR_print_stack_frame_stats(void)
> +{
...
> +	fp = fopen(MR_STACK_FRAME_STATS, "a");
> +	if (fp == NULL) {
> +		return;
> +	}
...
> +	(void) fclose(fp);
> +}

It would be better to use MR_checked_fopen()
and MR_checked_fclose(), I think.

i.e. I'd rather see it abort with a decent error message
than quietly ignore failure here.

> +++ runtime/mercury_stacks.h	2001/12/21 05:46:15
> @@ -17,6 +17,53 @@
>  #include "mercury_tabling.h"
>  #include "mercury_engine.h"
>  
> +#ifdef	MR_STACK_FRAME_STATS
> +  #include "mercury_dword.h"
> +
> +  extern MR_dword	MR_det_frame_count;
> +  extern MR_dword	MR_det_frame_total_size;
> +  extern MR_Word	*MR_det_frame_max;
> +  extern MR_dword	MR_non_frame_count;
> +  extern MR_dword	MR_non_frame_total_size;
> +  extern MR_Word	*MR_non_frame_max;
> +
> +  /*
> +  ** This temporary is for use in the MR_increment_dword_tmp macro only.
> +  ** Making the temporary variable global (nonlocal to the macro) allows
> +  ** the macro have the form of an expression, instead of a statement,
> +  ** without relying on GNU extensions to C.
> +  */
> +  extern unsigned MR_INT_LEAST32_TYPE	MR_old_low_tmp;

Use `MR_int_least32_t'.

This code is not thread-safe (due to both the global counters,
and the global temporaries).  It would be a good idea to add

	#if defined(MR_THREAD_SAFE) && defined(MR_STACK_FRAME_STATS)
	  /* XXX This combination not supported */
	  #error ...
	#endif

or equivalent somewhere appropriate.

Otherwise that looks fine.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  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
--------------------------------------------------------------------------