[m-rev.] for review: Optimise dependency file index maps.

[Peter Wang]

On Mon, 28 Nov 2022 16:47:26 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> 2022-11-28 15:06 GMT+11:00 "Peter Wang" <novalazy at gmail.com>:
> > Replace the type used in the mappings between dependency_file and
> > dependency_file_index from dependency_file, which refers to targets by
> > module_name, to a similar type that refers to targets by module_index.
> 
> I didn't understand that sentence until I read the diff, which is not good
> for something that is supposed to explain the diff.
> 

Ok, I've rewritten it.

> > make.dependencies.of_3
> 
> This is unrelated to this diff, but if you feel you understand
> of_2 and of_3's tasks well enough to write a comment explaining those
> tasks, please do so.
> 

See below.

> > This change improves the run time of a do-nothing build of Prince
> > on my machine is improved from 1.8 s to 1.7 s.
> 
> The two halves of that sentence don't go together.

Fixed.

> 
> The diff looks good, but I have a question about it. You add this new type
> with the long name:
> 
> > +:- type dependency_file_with_module_index
> > +    --->    dfmi_target(module_index, module_target_type)
> > +    ;       dfmi_file(file_name).
> 
> This new type is isomorphic to a flattened version of the existing dependency_file
> type, with the module_name replaced with a module_index. Have you considered
> an approach that, instead of adding this new type, just changes the args of the dep_target
> cons_id of dependency_file with a triple: module_name, module_index, and module_target_type?
> This would eliminate all the conversions, but would work only if you could assign
> a module_index to every module_name you put into a dep_target. I think you can,
> but then, I haven't tried to make it happen. If you can make it happen, I think that
> approach would be better than the one in this diff. (The hash would of course ignore
> the module name field.)

As I recall it required additional lookups in places where we don't
already have module_index or module_name. But I might try it again
as a separate change.

Peter

---

Optimise dependency file index maps.

In make_info we maintain a mapping between dependency_file and
dependency_file_index. Replace the type dependency_file used in those
maps with a new type, in which modules are referred to by module_index
instead of by module_name.

The reason for the change is that make.dependencies.of_3 is called
frequently to convert a module_index and module_target_type to a
dependency_file_index. With this change, it does not need to look up
the module_name corresponding to a module_index, but uses the
module_index as given. Also, using module_index instead of module_name
as part of a hash table key should be faster as (1) hashing is faster,
and (2) comparing keys within the same hash table bucket is faster.

This change improves the run time of a do-nothing build of Prince
on my machine from 1.8 s to 1.7 s.

diff --git a/compiler/make.dependencies.m b/compiler/make.dependencies.m
index df6802215..ebaf2a92c 100644
--- a/compiler/make.dependencies.m
+++ b/compiler/make.dependencies.m
@@ -384,6 +384,12 @@ imports = combine_deps_list([

 %---------------------------------------------------------------------------%

+    % TargetType `of` F is function that returns the set of TargetType targets
+    % based on the modules generated by F.
+    %
+    % e.g. module_target_int0 `of` ancestors takes a module and returns the
+    % set of .int0 targets for the module's ancestor modules.
+    %
 :- func of(module_target_type, find_module_deps(module_index)) =
     find_module_deps(dependency_file_index).
 :- mode of(in, in(find_module_deps)) = out(find_module_deps) is det.