[m-dev.] for review: cleanup of tabling

[Fergus Henderson]

On 28-Dec-1999, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> A major cleanup of the internals of tabling.

So far I've only had a look at the changes to the following files:

> library/private_builtin.m:
> 	Changes to conform to the changed types in mercury_tabling.h.
> 	In some cases, improve the debugging support.
> 
> runtime/mercury_tabling.h:
> 	Define the new types.
> 
> 	Add macros for allocating memory for holding one or more structures.
> 	Make the existing macros call the versions that check for malloc
> 	returning NULL.
> 
> runtime/mercury_tabling_macros.h:
> 	This new file contains macros that used to be part of the file
> 	mercury_tabling.h. The macros call the functions defined in
> 	mercury_tabling.c, but they also optionally print debugging messages.

What's the rationale for moving these macros into a different header file?

...
> Index: runtime/mercury_tabling.h
...
> +typedef	union MR_Union_TableNode	MR_TableNode;
> +typedef	struct MR_Struct_HashTable	MR_HashTable;
> +typedef	struct MR_Struct_StartTable	MR_StartTable;

For consistency with the conventions used elsewhere,
the `_Struct' or `_Union' part should be at the end of the
name, not immediately after the `MR_' prefix.

> +typedef	struct MR_SubgoalStruct		MR_Subgoal;
> +typedef	struct MR_SubgoalListNode	*MR_SubgoalList;
> +typedef	struct MR_AnswerListNodeStruct	MR_AnswerListNode;
> +typedef	struct MR_AnswerListNodeStruct	*MR_AnswerList;
> +typedef	struct MR_ConsumerListNode	*MR_ConsumerList;
> +typedef MR_TableNode			*MR_TrieNode;

(The comments immediately below address issues that are present
in the existing code; your changes just make the inconsistencies
more glaring.)

For consistency with the conventions used elsewhere,
	s/Struct/_Struct/g

For consistency with MR_AnswerListNode and MR_AnswerListNode_Struct,
etc., it would be better if MR_ConsumerListNode was a typedef, and
the name of the struct was MR_ConsumerListNode_Struct.
Likewise for MR_SubgoalListNode.

Also it would IMHO be clearer to use
   typedef	MR_AnswerListNode		*MR_AnswerList;
rather than
   typedef	struct MR_AnswerListNodeStruct	*MR_AnswerList;

> +union MR_Union_TableNode {
> +	Integer		MR_integer;
> +	Integer		MR_simpletable_status;
> +	MR_HashTable	*MR_hash_table;
> +	MR_TableNode	*MR_start_table;
> +	MR_TableNode	*MR_fix_table;
> +	Word		*MR_answerblock;
> +	MR_Subgoal	*MR_subgoal;
> +};

Some comments here would certainly be helpful.

>  struct MR_AnswerListNodeStruct {
>  	Integer		answer_num;
> -	Word		answer_data;
> +	MR_TableNode	answer_data; /* always uses the MR_answerblock field */
>  	MR_AnswerList	next_answer;
>  };

Talking about a "field" of a union was quite confusing to me.
I suggest
	s/field/member/
or 
	s/field/member of the union/

> -typedef enum {
> -	MR_SIMPLETABLE_UNINITIALIZED,
> -	MR_SIMPLETABLE_WORKING,
> -	MR_SIMPLETABLE_FAILED,
> -	MR_SIMPLETABLE_SUCCEEDED
> -} MR_SimpletableStatus;
> +#define	MR_SIMPLETABLE_UNINITIALIZED	0
> +#define	MR_SIMPLETABLE_WORKING		1
> +#define	MR_SIMPLETABLE_FAILED		2
> +#define	MR_SIMPLETABLE_SUCCEEDED	3

What's the rationale for that change?

In general enumeration constants are preferable to macros,
because C debuggers tend to have better support for
enumeration constants.
(Also because macros don't respect scope, but that's
less important in this case because of the use of a
long prefix `MR_SIMPLETABLE_'.)

> +** The function for tabling a float takes a word, not a float. If the float
> +** argument is not in the table, and floats are supposed to be boxed, then
> +** it is more efficient to reuse the existing box of the argument instead of
> +** allocating new one, and this requires passing the box as well as the float.

That comment seems to assume that floating point arguments will be boxed,
which is not true for the MLDS back-end.  The comment should be clarified
so that it is clear whether it is making that assumption and so that
it is clear whether it applies only to the LLDS back-end or to all
back-ends.

> +#ifdef CONSERVATIVE_GC
> +
> +  #define MR_table_allocate(type)					\
> +	MR_GC_NEW(type)
> +
> +  #define MR_table_allocate_array(type, count)				\
> +	MR_GC_NEW_ARRAY(type, count)
> +
> +  #define MR_table_reallocate_array(ptr, type, count)			\
> +	MR_GC_NEW_ARRAY(ptr, type, count)
> +
> +  #define table_allocate_bytes(size)					\
> +	MR_GC_malloc(size)
> +
> +  #define table_reallocate_bytes(pointer, size)				\
> +	MR_GC_realloc(pointer, size)

The second MR_GC_NEW_ARRAY should be MR_GC_RESIZE_ARRAY.
Also, all the parameters should be put in parentheses.
E.g. the last one there should be

  #define table_reallocate_bytes(pointer, size)				\
	MR_GC_realloc((pointer), (size))
	              ^       ^  ^    ^

It's not absolutely critical to do it in cases like these,
but it is a good idea to just do it all the time, lest some
maintainer forget to do it.

> +  #define table_allocate_words(size)					\
> +	MR_GC_malloc(sizeof(Word) * size)
> +
> +  #define table_reallocate_words(pointer, size)				\
> +	MR_GC_realloc(pointer, sizeof(Word) * size)

Likewise here.  In these cases, of course, it _is_ very important to put
the parentheses.

> +#define table_copy_bytes(Dest, Source, Size)				\
> +	MR_memcpy(Dest, Source, Size)
> +
> +#define table_copy_words(Dest, Source, Size)				\
> +	MR_memcpy((char *) (Dest), (char *) (Source), sizeof(Word) * Size)

Likewise here.
Also the parameter names here should be lower case.

> +/*
> +** mercury_tabling_macros.h
> +**
> +** This file defines macros used by the implementation of tabling
> +** (which means mostly the procedures defined in library/private_builtin.m).
> +** These macros just call the real implementation routines defined in
> +** runtime/mercury_tabling.c, but they also optionally print debugging
> +** information.
> +*/
> +
> +#define MR_RAW_TABLE_ANY(table, type_info, value)			\
> +	MR_table_type(table, type_info, value)
> +
> +#define MR_RAW_TABLE_TAG(table, tag)					\
> +	MR_int_fix_index_lookup_or_add(table, 1 << TAGBITS, tag)
> +
> +#define MR_RAW_TABLE_ENUM(table, range, value)				\
> +	MR_int_fix_index_lookup_or_add(table, range, value)
> +
> +#define MR_RAW_TABLE_WORD(table, value)					\
> +	MR_int_hash_lookup_or_add(table, value);
> +
> +#define MR_RAW_TABLE_INT(table, value)					\
> +	MR_int_hash_lookup_or_add(table, value);
> +
> +#define MR_RAW_TABLE_CHAR(table, value)					\
> +	MR_int_hash_lookup_or_add(table, value);
> +
> +#define MR_RAW_TABLE_FLOAT(table, value)				\
> +	MR_float_hash_lookup_or_add(table, value);
> +
> +#define MR_RAW_TABLE_STRING(table, value)	 			\
> +	MR_string_hash_lookup_or_add(table, value);
> +
> +#define MR_RAW_TABLE_TYPEINFO(table, type_info)				\
> +	MR_type_info_lookup_or_add(table, type_info)

Here too it would be a good idea to put all uses of parameters
in parentheses.

> +#ifdef CONSERVATIVE_GC
> +
> +  #define MR_TABLE_SAVE_ANSWER(table, offset, value, type_info)		\
> +	do {								\
> +		(table)->MR_answerblock[offset] = value;		\
> +	} while(0)
> +
> +#else /* not CONSERVATIVE_GC */
> +
> +  #define MR_TABLE_SAVE_ANSWER(table, offset, value, type_info)		\
> +	do {								\
> +		save_transient_hp();					\
> +		{ Word local_val = Value;				\
> +		(table)->MR_answerblock[offset] =			\
> +			deep_copy(&local_val, (Word *) &type_info,	\
> +				NULL, NULL);				\
> +		}							\
> +		restore_transient_hp();					\
> +	} while(0)

s/Value/(value)/
s/(Word *)/(const Word *)/

Rather than taking the address of the type_info parameter
(which might in general not be suitable, e.g. it might be in a register),
wouldn't it be better to assign it to a local variable, as is done
with the `value' parameter?

Also the indentation in that code doesn't reflect the nesting level.

Combining these suggestions:

		{ const Word local_val = (value);			\
		  const Word local_type_info = (type_info);		\
		  							\
		  (table)->MR_answerblock[offset] =			\
			deep_copy(&local_val, &local_type_info,		\
				NULL, NULL);				\
		}							\

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