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

[Paul Bone]

On Fri, Nov 28, 2014 at 11:46:08PM +1100, Zoltan Somogyi wrote:
> 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.

That seems backwards but I suspect that the misunderstanding is mine.  I
think I'll understand better when you've commited your code with this
replacement and I read them together.

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

True.

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

An error message that says "This feedback file doesn't match the program
you're trying to analyse" or similar would be much clearer than some
"procedure 263 not found" or similar error.  Detecting this type of error
early may prevent confusing the user.

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

No, the testsuite tests sensable use of the feedkback tools.  In practice a
user in a real situation could break the code in ways that I can't imagine
and may never make sense.

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

That makes sense for now, I'm happy with that.


> > > - 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'd prefer to remove the code completely and leave a comment.  IMHO this is
one of the reasons to use version control and archive our old builds.  It's
also very unlikey that someone will want to test this code.  It only makes
sense to test either Jerome's or my older work to attempt to reproduce
results in our papers, so it's important that there is a pointer to this old
code/data.  However I don't think it's likely that someone will want to do
this.


-- 
Paul Bone