[m-dev.] for review: reorganize label table, reduce executable sizes

[Fergus Henderson]

On 25-Jun-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> Reorganize the label table to better fit the needs of accurate gc
> and the tracer, and reduce the size of executables.
> 
> With these changes, the size of hello world, compiled with static
> Mercury libraries but dynamic C libraries, goes from 150+ Kb to
> 138+ Kb, much of which is beyond our control (e.g. Boehm gc).

What's the size if you link in a non-gc grade?

> Estimated hours taken: 20

ObPedantry: the standard log template has the hours at the top
and the description following that.

> runtime/mercury_trace_permanent.[ch]:
> 	Split the old mercury_trace module into two. The functions and
> 	variables that will always be linked in (because they are referenced
> 	from modules such as mercury_wrapper which are always needed) are
> 	now in mercury_trace_permanent; the other functions and variables
> 	stay in mercury_trace.

Hmm, I don't really like the name `mercury_trace_permanent', but then
I can't think of anything better off-hand.  Suggestions, anyone?

> runtime/mercury_wrapper.c:
> runtime/mercury_conf_param.h:
> 	Remove some obsolete MERCURY_OPTIONS options, which would be
> 	difficult to support with the new label table.
> 
> 	Replace the long help message for malformed MERCURY_OPTIONS
> 	environment variables with a pointer to the appropriate manual.

I suggest s/appropriate manual/user's guide/

> +++ mercury_goto.h	1998/06/23 08:02:17
> @@ -12,6 +12,7 @@
>  #include "mercury_conf.h"
>  #include "mercury_types.h"	/* for `Code *' */
>  #include "mercury_debug.h"	/* for debuggoto() */
> +#include "mercury_label.h"	/* for insert_{entry,internal}_label() */
>  
>  #define paste(a,b) a##b
>  #define stringify(string) #string
> @@ -19,13 +20,15 @@
>  #define skip(label) paste(skip_,label)
>  
>  #if defined(MR_USE_STACK_LAYOUTS)
> -  #define MR_STACK_LAYOUT(label)        (Word *) (Word) \
> +  #define MR_ENTRY_LAYOUT(label)	(MR_Stack_Layout_Entry *) (Word) \
>  	&(paste(mercury_data__layout__,label))

Shouldn't that be a cast to `const MR_Stack_Layout_Entry *'?
                             ^^^^^

> +  #define MR_INTERNAL_LAYOUT(label)	(MR_Stack_Layout_Label *) (Word) \
> +	&(paste(mercury_data__layout__,label))

Likewise.

> mercury_label.c
> +#define	INIT_ENTRY_SIZE	(1 << 8)

What's the units?
A comment here would help.

> +static int
> +compare_entry_addr(const void *e1, const void *e2)
> +{
> +	const MR_Entry	*entry1;
> +	const MR_Entry	*entry2;
> +
> +	entry1 = (const MR_Entry *) e1;
> +	entry2 = (const MR_Entry *) e2;

Personally I would combine the declarations and initializations here.

> +MR_Entry *
> +MR_prev_entry_by_addr(const Code *addr)
>  {
> +	int	lo;
> +	int	hi;
> +	int	mid;
> +	int	i;
> +
> +	if (!entry_array_sorted) {
> +		qsort(entry_array, entry_array_next, sizeof(MR_Entry),
> +			compare_entry_addr);
>  	}

I think you should set `entry_array_sorted = TRUE' there,
otherwise you will get a performance bug.

> +static const void *
> +internal_addr(const void *internal)
>  {
> -	return streq(((const char *) name1), ((const char *) name2));
> +	return (const void *) (((const MR_Internal *) internal)->i_addr);
>  }

Is the cast to `const void *' really needed here?

> +++ mercury_label.h	1998/06/23 07:55:51
> +#if     defined(NATIVE_GC) || defined(MR_DEBUG_GOTOS)
> +#define	MR_NEED_ENTRY_LABEL_ARRAY
> +#endif
> +
> +#if     defined(MR_NEED_ENTRY_LABEL_ARRAY) || defined(PROFILE_CALLS)
> +#define	MR_NEED_ENTRY_LABEL_INFO
> +#endif

Please indent the #defines by two spaces, since they are within `#if'.

> +typedef struct s_entry {
> +	Code			*e_addr;
> +	MR_Stack_Layout_Entry	*e_layout;
> +	const char		*e_name;
> +} MR_Entry;
> +
> +typedef struct s_internal {
> +	Code			*i_addr;
> +	MR_Stack_Layout_Label	*i_layout;
> +	const char		*i_name;
> +} MR_Internal;

Some brief documentation explaining the purpose of these structures
would be helpful here.

> +#ifdef	MR_NEED_ENTRY_LABEL_INFO
> +extern	void		MR_insert_entry_label(const char *name, Code *addr,
> +				MR_Stack_Layout_Entry *entry_layout);
> +#else
> +#define	MR_insert_entry_label(n, a, l)	/* nothing */
> +#endif	/* not MR_NEED_ENTRY_LABEL_INFO */
> +
> +#ifdef	MR_NEED_ENTRY_LABEL_ARRAY
> +extern	MR_Entry	*MR_prev_entry_by_addr(const Code *addr);
> +#endif	/* MR_NEED_ENTRY_LABEL_ARRAY */

Please indent the stuff inside `#ifdef' by two spaces.

> +	printf("The MERCURY_OPTIONS environment variable"
> +		"contains an invalid option.\n"
> +		"Please refer to the Mercury users' guide for details.\n");

s/users'/user's/

You should specify which section.

Currently all the Mercury user's guide says about MERCURY_OPTIONS is

`MERCURY_OPTIONS'
     A list of options for the Mercury runtime that gets linked into
     every Mercury program.  Options are available to set the size of
     the different memory areas, and to enable various debugging traces.
     Set `MERCURY_OPTIONS' to `-h' for help.

so if you want to have the help message point to the documentation
you had better make sure there is actually some documentation there
to point to.

> --- c2init.in	1998/05/30 13:34:09	1.15
> +++ c2init.in	1998/06/25 06:17:53
> @@ -19,6 +19,15 @@
>  Name:	c2init - Create Mercury initialization file.
>  Usage:	c2init [options] *.c *.init ...
>  Options: 
> +	-c <n>, --max-calls <n>
> +		Break up the initialization into groups of at most <n> function
> +		calls.  (Default value of <n> is 40.)
> +	-i, --include-initialization-code
> +		Always include code that calls the initialization functions
> +		of the various modules. With this option, the debugger can use
> +		information from any modules that were compiled with execution
> +		tracing to do (partial) stack tracing and uplevel printing
> +		even in grades in which this not normally possible.

What's uplevel printing?  I don't think the average user will know what
that means.


I think this change will need another round of reviewing.

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.