[m-rev.] for review: use trail segments by default in trailing grades

[Julien Fischer]

Hi Zoltan,

On Mon, 17 Feb 2020, Zoltan Somogyi wrote:

> On Sun, 16 Feb 2020 14:24:22 +1100 (AEDT), Julien Fischer <jfischer at opturion.com> wrote:
>> Attached to this mail.
>
> Thank you. The review is attached to this email.


> > Use trail segments by default in trailing grades.
> > 
> > We currently support two variants of trailing grades, those that use a
> > fixed-size trail (.tr) and those that use trail segments (.trseg). This change
> > removes non-developer support for fixed sized trails and renames the .trseg
> > grade component to .tr. The .trseg grade now acts a synonym for .tr; we will
> > eventually delete it.
> 
> I would replace this with:
>
>     Until now, we have supported two variants of trailing grades, those that
>     use a fixed-size trail (.tr) and those that use trail segments (.trseg).
>     This change removes support for fixed sized trails, and renames
>     the .trseg grade component to .tr. The .trseg grade now acts a synonym
>     for .tr; it is deprecated, since we intend to eventually delete it.
>     Until then, the behavior of the old .tr grade component should be
>     available, though to developers only, by compiling the whole system
>     with EXTRA_CFLAGS = -DMR_USE_FIXED_SIZE_TRAIL.

Done.

> > runtime/mercury_conf_param.h:
> >     Delete the MR_TRAIL_SEGMENTS macro; its now implied by MR_USE_TRAIL.
> >
> >     Add a new macro, MR_USE_FIXED_SIZE_TRAIL, that developers can use
> >     to disable trail segments (should the need for doing that arise).
> 
> I would replace this with:
>
>     Delete the MR_TRAIL_SEGMENTS macro. Its effect is now implied by
>     MR_USE_TRAIL, unless a new macro, MR_USE_FIXED_SIZE_TRAIL, is defined.
>     Developers can use this new macro to disable trail segments, should
>     the need for doing that arise.

Done.

> > runtime/mercury_grade.h:
> >     Bump the binary compatibility number; old .tr grades are not compatible
> >     with the new ones.
> 
> But all non-trailing grades are. I think it would be better to keep the
> MR_GRADE_PART_0 unchanged, and instead add MR_GRADE_TRAIL_VERSION_NO as
> a new macro, which is handled the same way we now handle the other
> grade-component-specific version numbers, e.g. MR_GRADE_LLC_PAR_VERSION_NO.
> Besides yielding less disruption now, it is also more future proof.

Ok, I have added MR_GRADE_TRAIL_VERSION_NO and reverted the compatibility
number bump.

> > runtime/mercury_trail.[ch]:
> > runtime/mercury_context.h:
> >     Enable trail segments by default, only disabling the if
> >     MR_USE_FIXED_SIZE_TRAIL is enabled.
> 
> s/the/them/

Fixed.

> > runtime/mercury_wrapper.c:
> > trace/mercury_trace_cmd_developer.c:
> >     Conform to the above changes.
> 
> May I presume that you searched for all occurrences of MR_TRAIL_SEGMENTS
> in all directories and updated them to refer to !MR_USE_FIXED_SIZE_TRAIL
> instead?

You may.

> diff --git a/NEWS b/NEWS
> > index 1a7ed40..2db5839 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -90,6 +90,26 @@ Changes to the Mercury standard library
> >      - pred `string_hash/2`
> >      - pred `generic_hash/2`
> > 
> > +Changes to the Mercury compiler
> > +-------------------------------
> > +
> > +### Changes to code model options
> > +
> > +* `--trail-segments`
> > +
> > +   This option is deprecated and no longer has any effect.  Trail segments
> > +   are now always use in grades that support trailing (see below).
> 
> I would reword that as
>
>     Grades that support trailing now always use trail segments. This means
>     that the --trail-segments option now has no effect, and it is therefore
>     deprecated.

Done.

> > +Changes to the Mercury implementation
> > +-------------------------------------
> > +
> > +* In grades that support trailing, trail segments are now used by default.
> 
> "by default" implies that trail segments can nevertheless be turned off.
> This is true for developers, but not for users, so I would reword that as
>
>     Grades that support trailing now always use trail segments;
>     it is no longer possible to use a fixed size trail.
>
>     One consequence of this is that the `trseg` grade component now acts
>     as a synonym for the `tr` component. Since `tr' is shorter, `trseg'
>     is deprecated in its favor.
>
>     Another consequence is that the `--trail-size` and `--trail-size-kwords`
>     runtime options no longer have any effect, and are deprecated.

Done.

> > diff --git a/doc/user_guide.texi b/doc/user_guide.texi
> > index 6d2468f..55637dd 100644
> > --- a/doc/user_guide.texi
> > +++ b/doc/user_guide.texi
> > @@ -8470,10 +8471,8 @@ This option is only supported by the C back-ends.
> >  @cindex Constraint solving
> >  @cindex Backtrackable destructive update
> >  @cindex Destructive update, backtrackable
> > -Enable the use of a dynamically sized trail that is composed of small segments.
> > -This can help to avoid trail exhaustion at the cost of increased execution
> > -time.
> > -This option is only supported by the C back-ends.
> > +This option is deprecated as trail segments are now used by default.
> > +The @samp{.trseg} grade modifier is a synonym for @samp{.tr}.
> 
> Again, I would avoid the use of the word "default" here.

Done.

> > + at c These two options are commented out as we only support a fixed size tail
> > + at c for developers.
> 
> Emphasis: ... as we support a fixed trail ONLY for developers.

Done.

> > index 64c081b..38f6618 100644
> > --- a/runtime/mercury_conf_param.h
> > +++ b/runtime/mercury_conf_param.h
> > @@ -394,6 +392,10 @@
> >  // Enables low-level debugging messages when updating the list of
> >  // trail segments.
> >  //
> > +// MR_USE_FIXED_SIZE_TRAIL
> > +// Disables the trail segment mechanism; this is sometimes use for
> > +// developers working on that mechanism.
> 
> s/use/useful/

Fixed.

> > diff --git a/runtime/mercury_grade.h b/runtime/mercury_grade.h
> > index faeda1b..d5260d4 100644
> > --- a/runtime/mercury_grade.h
> > +++ b/runtime/mercury_grade.h
> > @@ -237,13 +237,8 @@
> >  #endif
> > 
> >  #ifdef MR_USE_TRAIL
> > -   #ifdef MR_TRAIL_SEGMENTS
> > -    #define MR_GRADE_PART_7     MR_PASTE2(MR_GRADE_PART_6, _trseg)
> > -    #define MR_GRADE_OPT_PART_7 MR_GRADE_OPT_PART_6 ".trseg"
> > -  #else
> > -    #define MR_GRADE_PART_7     MR_PASTE2(MR_GRADE_PART_6, _tr)
> > -    #define MR_GRADE_OPT_PART_7 MR_GRADE_OPT_PART_6 ".tr"
> > -   #endif // ! MR_TRAIL_SEGMENTS
> > +  #define MR_GRADE_PART_7     MR_PASTE2(MR_GRADE_PART_6, _tr)
> > +  #define MR_GRADE_OPT_PART_7 MR_GRADE_OPT_PART_6 ".tr"
> 
> This change looks like a bug magnet to me, allowing undetected mixing
> of code with and without trail segments. I would keep the old logic,
> replacing #ifdef MR_TRAIL_SEGMENTS with #ifndef MR_USE_FIXED_SIZE_TRAIL.
> (Actually, I would use #ifdef MR_USE_FIXED_SIZE_TRAIL and invert the
> then and else parts.)

I have changed the above to:

   #ifdef MR_USE_TRAIL
     #ifdef MR_USE_FIXED_SIZE_TRAIL
       #define MR_GRADE_PART_7     MR_PASTE3(MR_GRADE_PART_6, _trfix, MR_GRADE_TRAIL_VERSION_NO)
     #else
       #define MR_GRADE_PART_7     MR_PASTE3(MR_GRADE_PART_6, _trseg, MR_GRADE_TRAIL_VERSION_NO)
     #endif
       #define MR_GRADE_OPT_PART_7 MR_GRADE_OPT_PART_6 ".tr"
   #else
     #define MR_GRADE_PART_7       MR_GRADE_PART_6
     #define MR_GRADE_OPT_PART_7   MR_GRADE_OPT_PART_6
   #endif

I think it will be clearer to use "_trfix" in the definition of MR_GRADE,
rather than the exisiting "_tr", so I have done that.

> Other than that, the diff is fine.

Thanks for that.

Julien.