[m-dev.] for review: Accurate garbage collection.

[Fergus Henderson]

On 01-Jul-1998, Tyson Dowd <trd at cs.mu.OZ.AU> wrote:

> library/builtin.m:
> library/mercury_builtin.m:
> library/std_util.m:
> runtime/mercury_tabling.h:
> 	Deep copy terms using the address of the value instead of
> 	just the value.
...
> 	deep_copy (all variants) have been modified to take
> 	a pointer to the data to be copied, because some variants
> 	need to be able to modify it.

There are probably other calls to deep_copy() somewhere in extras/*.

I think it would be better to leave the interface to deep_copy()
as is, if necessary by making deep_copy() a forwarding macro that
calls the function that actually does the work.

> runtime/mercury_regorder.h:
> runtime/machdeps/alpha_regs.h:
> runtime/machdeps/i386_regs.h:
> 	Add defintions to make the real machine registers name/number
> 	for MR_sp available.

s/defintions/definitions/

WORK_IN_PROGRESS:
> +* There is a new garbage collector that does accurate garbage
> +  collection (.agc grade).  It is currently limited to deterministic
> +  code, and needs a great deal of tuning and optimization.

You should add "testing" to that list ;-)

> Index: library/io.m
...
> +#ifdef NATIVE_GC
> +	MR_agc_add_root(&ML_io_stream_names, (Word *) StreamNamesType);
> +	MR_agc_add_root(&ML_io_user_globals, (Word *) UserGlobalsType);
> +#endif 

In general, users will need to call MR_agc_add_root() for any
global variable which contains pointers to the Mercury heap.
So I think it would be better to have a macro

	#ifdef NATIVE_GC
	  #define MR_add_root(root_ptr, type_info) \
	  	MR_agc_add_root((root_ptr), (type_info))
	#else
	  #define MR_add_root(root_ptr, type_info) /* nothing */
	#endif

defined somewhere in the runtime, e.g. runtime/mercury_memory.h.
That way the #ifdef doesn't need to be in so many places.

> Index: runtime/mercury_accurate_gc.c
...
> +#ifdef NATIVE_GC
> +
> +#include "mercury_imp.h"
> +#include "mercury_trace_util.h"
> +#include "mercury_deep_copy.h"
> +#include "mercury_agc_debug.h"

ANSI C requires that a compilation unit not be completely empty;
you need at least one declaration.  So put the #ifdef
after the #include of "mercury_imp.h".

> +	/*
> +	** Function prototypes.
> +	*/
> +static void	garbage_collect(Code *saved_success, Word *stack_pointer, Word

The C code in the Mercury runtime is not entirely consistent on this issue,
but for the majority of it, comments do not get any extra indentation --
they are at the same level as the stuff of indentation as the stuff they
are describing.  Our C coding guidelines document is mostly silent on
the issue, but the examples in it do use non-intended comments.
I think this is preferable, in general.  So would you mind unindenting
all those comments?

> +void
> +garbage_collect_roots() 
> +{

s/()/(void)/

> Index: runtime/mercury_agc_debug.c
...
> +void
> +MR_agc_dump_stack_frames(Label *label, MemoryZone *heap_zone,
> +	Word *stack_pointer, Word *current_frame)
> +{
> +			success_ip = (Code *) field(0, stack_pointer, -number);

I think it might be better to use the based_stackvar() macro here:

			success_ip = (Code *)
				based_stackvar(stack_pointer, number);

> +		top_frame = top_frame && FALSE;

What are you trying to do here?  Should that be `top_frame = FALSE;'?

> +static void
> +dump_live_value(MR_Live_Lval locn, MemoryZone *heap_zone, Word *stack_pointer,
> +	Word *current_frame, bool do_regs)
> +{
...
> +		case MR_LVAL_TYPE_STACKVAR:
> +			value = stack_pointer[-locn_num];

Here it would definitely be better to use the based_stackvar() macro.

> +	/*
> +	** Define these variables to turn on more debugging information
> +	** for accurate garbage collection.
> +	*/
> +#if MR_DEBUG_AGC
> +  #define MR_DEBUG_AGC_SCHEDULING
> +  #define MR_DEBUG_AGC_COLLECTION
> +  #define MR_DEBUG_AGC_FORWARDING
> +  #define MR_DEBUG_AGC_PRINT_VARS
> +#endif

These configuration macros should be documented in mercury_conf_param.h.

> +++ mercury_deep_copy.c	1998/06/30 06:56:26
> @@ -5,7 +5,11 @@
>  */
>  
>  /*
> -** This module defines the deep_copy() function.
> +** This module defines the deep_copy() functions.
> +**
> +** Deep copy is used for a number of different purposes.  Each variant
> +** has the same basic control structure, but differs in how memory
> +** is allocated, or forwarding pointers left behind, or 
>  */

... or whether forwarding pointers are left behind, ...
       ^^^^^^^                     ^^^

> -Word deep_copy(Word data, Word *type_info, Word *lower_limit, 
> +Word deep_copy(Word *data_ptr, Word *type_info, Word *lower_limit, 
> +	Word *upper_limit);

That really ought to be `const Word *data_ptr'.
(The other pointers should all be const too...)

> Index: runtime/mercury_deep_copy_body.h

Can you give a diff of that against the old mercury_deep_copy.c?

> Index: runtime/mercury_trace_util.c
> +void
> +MR_trace_write_variable(Word type_info, Word value)
...
> +		restore_transient_registers();
> +		r1 = type_info;
> +		r2 = value;
> +		save_transient_registers();
> +		call_engine(MR_library_trace_browser);
> +}

That code is indented one tab too many.

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