[m-rev.] for post-commit review: verbose_make_N_part_msg

Peter Wang novalazy at gmail.com
Wed Jul 12 17:01:03 AEST 2023


On Tue, 11 Jul 2023 20:21:39 +1000 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> On 2023-07-11 10:45 +02:00 CEST, "Peter Wang" <novalazy at gmail.com> wrote:
> >> ... the only computation in get_dependency_status of TargetFileName,
> >> the call on what is now line 1389, was introduced almost a month earlier,
> >> on June 1, in commit 083d376e65.
> > 
> > Yes, the regression was first introduced in commit 083d376e65.
> > 
> > But it is commit 9eb003458 that "forces" the call to
> > get_make_target_file_name in get_dependency_status,
> > by returning TargetFileName to the caller.
> 
> By "forcing", did you mean that
> 
> - without 9eb..., the compiler would, at the right optimization level,
>   move the calls to get_make_target_file_name to just the branches
>   of the if-then-elses and switches in get_dependency_state that
>   need its output, which does NOT include the branch that is
>   executed after the search in DepStatusMap0 succeeds, but
> 
> - with 9eb..., it can no longer do so?
> 
> If you did, you could have said so. I was looking at the code as it is
> written, and arguing on that basis.
> 
> If you meant something else, then please tell me what you meant
> in more detail.
> 

The performance regression was first introduced in commit 083d376e65,
but is not INHERENT to that change. It would be easy to manually move
the calls to get_make_target_file_name into just the branches of
get_dependency_status that required its output, without changing
anything else.

On the other hand, the intention behind commit 9eb003458 was
specifically so that the caller of get_dependency_status would not need
to compute the filename, by making (forcing) get_dependency_status to
compute the filename, whether or not it is needed by
get_dependency_status or its caller.
But get_dependency_status is called very often, so the extra time spent
computing filenames (that are never used) used adds up.
That is why I consider the problem to be with the second commit.

Peter


More information about the reviews mailing list