[m-rev.] for review: simplify profiler feedback code

[Zoltan Somogyi]

On Fri, 28 Nov 2014 16:59:46 +1100, Paul Bone <paul at bone.id.au> wrote:
> > Paul, I would like you to check the following in particular:
> > 
> > - Should the calls to add_feedback_candidate_parallel_conjunctions be
> >   calls to replace_feedback_candidate_parallel_conjunctions instead?
> >   Under what circumstances did you plan to have existing feedback
> >   information simply replaced by new information of the same kind?
> 
> I remember allowing this (or something like it) for flexibility.  The idea
> was that if there are multiple types of feedback, and multiple tools
> generating feedback, each tool can add it's information and update the file
> before the file is used by the compiler.  In particular, this may be useful
> as the feedback system grows.
> 
> Now that I think about this (specifically for candidate parallel
> conjunctions), I think that I'd prefer to use add_feedback_* and receive an
> error if a feedback file already exists with this information in it, rather
> than silently updating it.

OK, I will replace the add_* calls with replace_* calls.
 
> Silently updating a file makes sense when used from a Makefile, so allowing
> this, possibly with a --force option, also makes sense.  Don't worry about
> implementing that now however, it's unnecessary for this patch.

I won't worry about it then, but this is not about updating files from a Makefile;
it is about updating a data structure inside a Mercury program, which is not
the same thing.

> > - The old code had two sources for the name of the program that created
> >   the information in a feedback_info: the one recorded in the feedback_info
> >   itself, and the caller of whatever predicate needed that program name.
> >   Why was the second source added, and could the two sources ever disagree?
> >   If they can disagree, why? If they cannot, I will delete the extra
> >   ProgramName args. In both cases, I will delete the assertions on them.
> > 
> >   I note that those assertions did not fail during a bootcheck, but the
> >   bootcheck may not have exercised the full functionality of the feedback
> >   mechanism.
> 
> I can't find the second source for the program name in your patch.  I
> remember using the program name as a check to ensure that the user didn't
> mix feedback information from different programs.  It was deliberate. 

The second source of the program name is not in the patch; it is supplied
by the caller of the predicate in the patch, and the caller wasn't changed.

However, I don't understand your reasoning here. Surely the compatibility
or otherwise of feedback provided by different programs depends on what
those programs are. At the moment, we have just one, so we have no
basis for any concrete argument one way or the other. Wouldn't this be
the kind of thing that is better decided when we get a second kind of feedback?

However, my original question was more basic, though it seems to have been
buried in its original sentence. This was: is the testing of the functionality
of the feedback mechanism during bootcheck thorough enough that
the absence of an abort due to a program name mismatch during a bootcheck
gives you confidence that the abort won't happen in real life either?

After the discussion above, I have an updated question: would you be happy
if I simply disabled the check for matching program names? Since there is
only one feedback type and it is generated by only one program, there doesn't
seem to be any point to it now; it seems to be just checking that two independently
supplied copies of that name are equal.
 
> > - Do you have any plans for the use of the now-commented-out feedback type?
> 
> No.  This was Jerome's work and we kept and maintained it as best we could
> so that we could use it to compare with my work.  I don't remember actually
> being able to reproduce Jerome's work from his own code though, so it's
> probably not useful.  The candidate parallel conjunctions analysis does
> include an option that disables the overlap analysis, and instead count the
> number of shared variables in a parallel conjunction and use that to
> approximate the amount of parallelism available.

Thanks for that. In that case, I think there are two reasonable choices
for what we should do with that code:

- keep it around in commented out form, as in the patch, but with
  an explanatory/historical comment saying what it is, or

- delete even the commented-out form of the code, but add the explanatory/historical
  comment as above, together with a pointer to where someone needs to dig in git
  to find the old code.

Any preference? 

> > I hope you had a good long weekend, and that the upcoming procedure on
> > your eye is successful.
> 
> Thanks.
> 
> Sorry for the delay.  I'm now feeling a lot better, I have very little pain
> and discomfort

I am glad to hear that.

Zoltan.