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

Zoltan Somogyi zoltan.somogyi at runbox.com
Mon Nov 28 16:47:26 AEDT 2022


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.

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

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

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

Zoltan.


More information about the reviews mailing list