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

[Peter Wang]

On Tue, 11 Jul 2023 18:10:42 +1000 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> On 2023-07-11 06:22 +02:00 CEST, "Peter Wang" <novalazy at gmail.com> wrote:
> > On Wed, 28 Jun 2023 23:15:12 +1000 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> >> Add verbose_make_N_part_msg predicates.
> >> 
> >> compiler/make.util.m:
> >>     Add verbose_make_N_part_msg predicates, and the more general
> >>     option_set_make_N_part_msg predicates, to allow simpler code
> >>     to construct progress and informational messages.
> >> 
> >>     Pass to debug_make_msg a message *generator*, instead of a predicate
> >>     that *writes out* a message. Make debug_make_msg return the message
> >>     (if debug_make is enabled) to be written out (hopefully) to an explicitly
> >>     specified stream.
> >> 
> >> compiler/make.dependencies.m:
> >>     Rename the dependency_status predicate to get_dependency_status.
> >>     Make it return the original dependency_file, and the filename it
> >>     corresponds to, alongside the status, because this is the simplest
> >>     way to avoid requiring its callers to reconstruct that information
> >>     after their calls to get_dependency_status.
> > 
> > Unfortunately this change forces get_dependency_status to incur a call
> > to get_make_target_file_name, even when the dependency status can be
> > looked up from the hash table.
> 
> May I ask what *exactly* are you referring by "this change"?

The change that makes get_dependency_status return the filename
alongside the status.

> -dependency_status(Globals, Dep, Status, !Info, !IO) :-
> +get_dependency_status(Globals, Dep, Tuple, !Info, !IO) :-
>      (
> -        Dep = dep_file(_FileName),
> +        Dep = dep_file(TargetFileName),
>          DepStatusMap0 = make_info_get_dependency_status(!.Info),
>          ( if version_hash_table.search(DepStatusMap0, Dep, StatusPrime) then
>              Status = StatusPrime
> @@ -1463,102 +1443,138 @@ dependency_status(Globals, Dep, Status, !Info, !IO) :-
>                  DepStatusMap1, DepStatusMap),
>              make_info_set_dependency_status(DepStatusMap, !Info)
>          )
> -    ).
> +    ),
> +    Tuple = {Dep, TargetFileName, Status}.
> 
> I don't see how constructing Tuple can result in a sixfold slowdown.

The slowdown is due to the call to get_make_target_file_name,
not constructing the tuple.

> > This causes a large performance regresssion in mmc --make. A do-nothing
> > build of Prince on my machine goes from ~0.55 secs to ~3.0 seconds.
> 
> Are the two compilers being compared in that test the compilers from
> just before and just after the Log.mu5 commit? Because ...
> 
> > How should we fix it? When the dep status is up-to-date, we could just
> > return a dummy TargetFileName on the assumption that the caller will
> > throw it away, although that is obviously not the cleanest fix.
> 
> ... 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.

> Given this fact, and your pinpointing of this call as the cause of the slowdown,
> may I presume that you have profiling data showing it as the bottleneck?
> If yes, I would like to see it.

I timed the building of Prince with mmc --make:
    - before and after commit 083d376e65
or equivalently,
    - commit 9eb003458
    - commit 9eb003458 + the hack to return an invalid filename

> 
> About possible solutions: I too would like to avoid returning an invalid
> file name if possible.
> 

> I would think that a cleaner solution would be replace the target file name
> and status fields of the tuple with a value of a new du type,
> whose function symbols would mirror those of the status, but would
> also have a filename argument for all statuses for which the callers
> *could need* the filename, which would not include the up-to-date case.
> 
> Another possible solution would be to keep both fields, but change the
> filename field to contain a MAYBE filename, set to yes(...) in cases where
> the code of get_dependency_status itself needs that filename, and to no
> otherwise. The caller would then need to construct TargetFileName only
> if get_dependency_status has not already constructed it. Note that
> get_dependency_status will need TargetFileName even in the up-to-date case
> if the conditions in maybe_warn_up_to_date_target_msg are satisfied.
> (This approach would need to move those conditions to get_dependency_status
> itself.) Compared to the approach in the previous paragraph, this avoids
> the computation of TargetFileName both in get_dependency_status AND
> its caller in the up-to-date warned about case. We can't make a
> reasoned decision about whether that gain is worthwhile without
> profiling data, but we could choose this approach on aesthetic grounds
> as well.
> 
> I would be fine either either approach.

I'll have a look tomorrow.

Peter