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

[Zoltan Somogyi]

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"? All of Log.mu*,
the whole of the commit described by Log.mu5, the part of that commit
that affects make.dependencies.m, or the change to the
get_dependency_status predicate? Your wording suggests the last,
but the diff to that predicate is:

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

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

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.

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.

Zoltan.