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

[Zoltan Somogyi]

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.
  
> +        % 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)).

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

The diff is fine.

Zoltan.