perf: run parse_compilation_units once

[jchavarri]

parse_compilation_units can be expensive when there are a lot of modules in the map. This PR makes sure it only runs once per lib (or other stanzas).

Some measurements for a few actions in Ahrefs repo. All actions are "noops" (the command was run previously successfully so no user actions should be executed):

Command	main	This branch
dune exec gen_esgg	real 0m10.121s, user 0m9.677s, sys 0m0.440s	real 0m1.372s, user 0m1.157s, sys 0m0.216s
dune build @fe.all	real 0m32.039s, user 0m30.391s, sys 0m1.685s	real 0m12.716s, user 0m11.615s, sys 0m1.119s
dune build @gen_dune	real 0m25.436s, user 0m24.253s, sys 0m1.223s	real 0m5.407s, user 0m4.908s, sys 0m0.504s

TODO

• Create public benchmark to test this PR (perf: add synthetic benchmark #7189, benchmark: add warm run #7198)

[rgrinberg]

Fix looks right. I'm wondering if we could simplify things even further though. How about saving the object map as part of Modules.t? This would remove a lot of the staging as well.

[jchavarri]

@rgrinberg how do you think the types should be modified in order to move in that direction? Something like this?

type t =
  { obj_map : Module.t Module_name.Unique.Map.t
  ; modules : modules
  }

and modules =
  | Singleton of Module.t
  | Unwrapped of Unwrapped.t
  | Wrapped of Wrapped.t
  | Impl of impl
  | Stdlib of Stdlib.t

and impl =
  { impl : t
  ; vlib : t
  }

[jchavarri]

Another thing I wonder about the idea of saving the object map as part of Modules.t is if there can be cases where this object map would be created unnecessarily, in a wasteful way 🤔

[rgrinberg]

how do you think the types should be modified in order to move in that direction? Something like this?

Yeah that sounds good.

Another thing I wonder about the idea of saving the object map as part of Modules.t is if there can be cases where this object map would be created unnecessarily, in a wasteful way 🤔

You could make it lazy to avoid that.

[jchavarri]

@rgrinberg please take a look to see if this is going in the right direction, thanks!

[jchavarri]

@rgrinberg thanks for the review, I think I tackled all your comments, the code looks better now.

If it looks good to merge, before doing so please take a look at #7198. I would like to merge that benchmark PR first, so that we can make measurements compare and understand better the impact in warm builds, thanks!

[rgrinberg]

Should we just make this valued by Sourced_module.t? It doesn't really hurt for ocamldep, and it will remove further redundant computations.

[jchavarri]

Do you mean the type being

val obj_map : t -> Sourced_module.t Module_name.Unique.Map.t

?
I tried something like that in c8e81be, but then I get errors in deps_of_module and deps_of_vlib_module because of the change, as some functions like Modules.alias_for still operate on Module.t.

[jchavarri]

File "src/dune_rules/dep_rules.ml", line 62, characters 16-20:
62 |       aliases @ deps)
                     ^^^^
Error: This expression has type Modules.Sourced_module.t list
       but an expression was expected of type Module.t list
       Type Modules.Sourced_module.t is not compatible with type Module.t

[rgrinberg]

Right, but you can extract the Module.t out of the Sourced_module.t without too many issues.

[jchavarri]

This builds, but I'm not sure I got the idea right, as it seems more complex than before 🤔 559e885

[rgrinberg]

I think you can simplify it by keeping ocamldep.mli the same, and moving the to_module conversion to ocamldep.ml.

[jchavarri]

Tried something in 81d3339.

[rgrinberg]

Should this be moved to a separate commit?

[rgrinberg]

I simplified things a little further. We don't need Obj_map at all.