[m-rev.] for review: Cache timestamps by target file.

[Peter Wang]

On Wed, 30 Nov 2022 12:33:17 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> 
> On Wed, 30 Nov 2022 12:04:54 +1100, Peter Wang <novalazy at gmail.com> wrote:
> > @@ -59,8 +58,8 @@
> >  
> >                  mki_file_timestamps     :: file_timestamps,
> >  
> > -                % Cache chosen file names for a module name and extension.
> > -                mki_search_file_name_cache :: map(module_name_ext, file_name),
> > +                mki_target_file_timestamps
> > +                                        :: target_file_timestamps,
> 
> Everywhere in the code diff where you delete anything from mki_file_timestamps,
> you also either delete the corresponding entry from mki_target_file_timestamps,
> or mention why you don't have to. I think you should add a comment about the
> invariant you are trying to maintain to the mki_file_timestamps field, to inform
> future maintainers.
>   

Ok, I have added a comment.

> > +        % This path is hit very frequently so it is worth caching timestamps by
> > +        % target_file. It avoids having to compute a file name for a
> > +        % target_file first, before looking up the timestamp for that file.
> > +        TargetFileTimestamps0 = !.Info ^ mki_target_file_timestamps,
> > +        ( if map.search(TargetFileTimestamps0, TargetFile, Timestamp) then
> > +            MaybeTimestamp = ok(Timestamp)
> > +        else
> > +            get_file_name(Globals, Search, TargetFile, FileName, !Info, !IO),
> > +            get_target_timestamp_2(Globals, Search, TargetFile,
> > +                FileName, MaybeTimestamp, !Info, !IO),
> > +            (
> > +                MaybeTimestamp = ok(Timestamp),
> > +                TargetFileTimestamps1 = !.Info ^ mki_target_file_timestamps,
> > +                map.det_insert(TargetFile, Timestamp,
> > +                    TargetFileTimestamps1, TargetFileTimestamps),
> > +                !Info ^ mki_target_file_timestamps := TargetFileTimestamps
> > +            ;
> > +                MaybeTimestamp = error(_)
> > +                % Do not record errors. These would usually be due to files not
> > +                % yet made, and the result would have to be updated once the
> > +                % file is made.
> > +            )
> > +        )
> 
> If the map.search call is still a bottleneck, consider replacing the one stage map
> map(target_file, timestamp) with the two stage map
> map(module_name, map(module_target_type, timestamp)).
> 

It's very low in the profile now.

> BTW, do you know why the first field of target_file is named
> target_file_name, when it contains a MODULE name, not a FILE name?
> If you don't, then I think it should be changed (in a separate diff).

I think it's just that "target_file_" is a common prefix for fields of
target_file, and "name" and "type" are the actual field names so to
speak. Similarly with linked_target_file whose fields have a prefix
"linked_tf_".

Peter