[m-dev.] for review: --call-trace-struct

[Fergus Henderson]

On 29-Apr-1999, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> Index: compiler/options.m
> +		"--call-trace-struct\t\t(This option is not for general use.)",
> +		"\tPass information to the tracing system in one struct.",

I suggest adding "rather than as separate arguments" here.

> Index: doc/user_guide.texi
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/doc/user_guide.texi,v
> retrieving revision 1.170
> diff -u -b -u -r1.170 user_guide.texi
> --- user_guide.texi	1999/04/23 01:03:24	1.170
> +++ user_guide.texi	1999/04/26 07:22:58
> @@ -3091,6 +3091,11 @@
>  Combines the effect of the two options above.
>  
>  @sp 1
> + at item @code{--call-trace-struct}
> +(This option is not intended for general use.)
> +Pass information to the tracing system in one struct.

Likewise here.

> +++ mercury_trace_base.c	1999/04/08 06:14:02
> @@ -103,13 +103,87 @@
>  #endif
>  
>  Code *
> +MR_trace_struct(const MR_Trace_Call_Info *trace_call_info)
> +{
> +	/*
> +	** You can change the 0 to 1 in the #if if you suspect that
> +	** MR_trace and MR_trace_struct have diverged.
> +	*/
> +
> +#if 0
> +
> +	return MR_trace(trace_call_info->MR_trace_sll,
> +		trace_call_info->MR_trace_port,
> +		trace_call_info->MR_trace_path,
> +		trace_call_info->MR_trace_max_r_num);
> +
> +#else
> +
> +	const MR_Stack_Layout_Label	*layout;
> +	Integer				maybe_from_full;
> +	Unsigned			seqno;
> +	Unsigned			depth;
> +
> +	/*
> +	** WARNING WARNING WARNING
> +	**
> +	** The code of this function is duplicated from MR_trace,
> +	** modulo references to the arguments. Any changes here
> +	** must also be done there as well.
> +	**
> +	** This duplication is for efficiency.
> +	*/
> +
> +	if (! MR_trace_enabled) {
> +		return NULL;
> +	}
...

Hmm... is it really worth duplicating that code for efficiency?
Did you consider writing that function like this instead?

	Code *
	MR_trace_struct(const MR_Trace_Call_Info *trace_call_info)
	{
		if (! MR_trace_enabled) {
			return NULL;
		}
		return MR_trace(trace_call_info->MR_trace_sll,
			trace_call_info->MR_trace_port,
			trace_call_info->MR_trace_path,
			trace_call_info->MR_trace_max_r_num);
	}

I can understand the wish to minimize code size (and hence compilation time)
for code with tracing enabled, and so the move to using MR_trace_struct()
is a win since it reduces the code size of the calls to the tracing system.
And I can understand the wish to minimize the efficiency impact of
compiling code for tracing if tracing is not enabled, and again the move
to using MR_trace_struct() is a win since the smaller code size results
in better locality.  But is it really important to optimize for speed
in the case where tracing is enabled, at the expense of code size and
maintainability?

(It's OK if the answer is "yes", I just want to make sure that you
considered this alternative.)

> --- mercury_trace_base.h	1998/12/14 16:05:14	1.5
> +++ mercury_trace_base.h	1999/04/08 05:55:43
> @@ -43,6 +43,24 @@
>  #define MR_trace_reset_depth(d)		(MR_trace_call_depth = (Unsigned) (d))
>  
>  /*
> +** This structure holds all the information passed to MR_trace() by the
> +** tracing code generated by the compiler. This info is all in one structure
> +** to allow calls to MR_trace to simply pass the address of a static data
> +** structure. This should be faster than moving several separate arguments
> +** into their registers. If tracing is not enabled, the arguments are
> +** not looked at anyway. If tracing is enabled, the speed advantage
> +** may or may not evaporate, depending on how many of these fields
> +** the current trace command looks at.
> +*/

I think this comment should be modified to mention the code size advantage,
rather than just talking about the speed advantage -- I think
the code size advantage is more important than the speed advantage.

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