[m-rev.] for review: Reset target file timestamp cache before installing library grade.

[Peter Wang]

On Tue, 12 Dec 2023 14:54:56 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> On 2023-12-12 14:39 +11:00 AEDT, "Peter Wang" <novalazy at gmail.com> wrote:
> > diff --git a/compiler/make.program_target.m b/compiler/make.program_target.m
> > index 385a07f2f..3a3144297 100644
> > --- a/compiler/make.program_target.m
> > +++ b/compiler/make.program_target.m
> > @@ -1847,6 +1847,11 @@ install_library_grade(LinkSucceeded0, ModuleName, AllModules,
> >          make_info_set_dep_file_status_map(StatusMap, !Info),
> >          make_info_set_option_args(OptionArgs, !Info),
> >  
> > +        % Reset the target file timestamp cache, as the information it contains
> > +        % is not valid for the changed grade and grade-subdir setting.
> > +        make_info_set_target_file_timestamps(init_target_file_timestamps,
> > +            !Info),
> > +
> >          % Building the library in the new grade is done in a separate process
> >          % to make it easier to stop and clean up on an interrupt.
> >          globals.lookup_bool_option(LibGlobals, very_verbose, VeryVerbose),
> 
> This code to clear out the cache is done before installing grade.
> If you are installing several grades, this will clean out the cache before each grade.
> However, is it possible for a compiler invocation to (a) install one or more grades,
> and then (b) do something else? If yes, then we will need to clear out the cache
> before step b as well.

The compiler can do something else after installing several grades
(see foldl2_make_top_targets).

However, after the call to install_library_grade_2 (which occurs in a
child process if possible), we continue with the make_info from before
the call to install_library_grade_2, which has the empty cache.

In the rare case that install_library_grade_2 is called in the same
process, there MAY be a tiny benefit from starting over with a new cache.
The make_info structure that we continue with will have a reference to
old version hash table, which requires unwinding all the updates made to
it in order to access that old version -- which we know is an empty hash
table.

> > +:- pred verify_cached_target_file_timestamp(io.text_output_stream::in,
> > +    globals::in, maybe_search::in, target_file::in, timestamp::in,
> > +    make_info::in, make_info::out, io::di, io::uo) is det.
> > +
> > +verify_cached_target_file_timestamp(ProgressStream, Globals, Search,
> > +        TargetFile, Timestamp0, !Info, !IO) :-
> > +    ForSearch = maybe_search_to_maybe_for_search(Search),
> > +    module_maybe_nested_target_file_to_file_name(ProgressStream,
> > +        Globals, $pred, ForSearch, TargetFile, FileName, !Info, !IO),
> > +    get_target_timestamp_2(ProgressStream, Globals,
> > +        Search, TargetFile, FileName, MaybeTimestamp, !Info, !IO),
> 
> Until now, get_target_timestamp_2 was called only from get_target_timestamp,
> and its analysis registry version, so it was ok that its name was not descriptive.
> With the addition of this call, it needs a descriptive name. Sadly, I don't know
> enough the motivation for what exactly it does to propose such a name.
> 

I'm not sure it can be understood independently of its call sites,
but I've renamed it to get_target_timestamp_uncached.

> > +    (
> > +        MaybeTimestamp = ok(Timestamp),
> > +        ( if Timestamp0 \= Timestamp then
> 
> I would reverse this test, and exchange the then- and else-parts.
> I would also rename Timestamp0 and Timestamp; they are not initial
> and final versions, but come from different sources. I would put the source
> in each name.
> 

Done.

Thanks for the review.

Peter