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

Paul Bone paul at bone.id.au
Fri Nov 28 16:59:46 AEDT 2014


On Tue, Nov 04, 2014 at 12:55:33AM +1100, Zoltan Somogyi wrote:
> For review by Paul.

I think that the patch is good and makes sense.  Subject to these items:

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

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.

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

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

> 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 but my vision is still blurred and I think that it will be
until I buy new glasses that match my differently-shaped cornea.


-- 
Paul Bone



More information about the reviews mailing list