feat: per-module inter-library dependency filtering (#4572)

[robinbb]

Summary

Use the already-computed ocamldep output to filter inter-library
dependencies on a per-module basis.

Previously, when libA depends on libB, every module of libA got a
glob dependency on all .cmi files in libB. Now each module only
depends on .cmi/.cmx files from libraries it actually imports,
eliminating unnecessary recompilation.

• Both .ml and .mli ocamldep output are read for each module, since
  the interface can reference different libraries than the implementation
• -open flags are included in the referenced module set
• Cross-library transparent aliases (module M = Mylib): correctly
  handled by transitively closing filtered libraries via Lib.closure,
  bounded by the compilation context's library set

Fallback cases (no change from current behavior)

• Virtual library implementations and parameterised modules: parameter
  libraries are not in requires_compile, so filtering falls back
• Libraries with virtual implementations in requires: filtering falls
  back to all libs
• Melange mode: filtering is OCaml-only
• Modules not in the dep graph (e.g., menhir-generated): filtering
  falls back

Prerequisite PRs

• test: document inter-library recompilation behavior (#4572) #14017 — baseline tests documenting current recompilation behavior
• test: document module name shadowing between stanza and library (#4572) #14031 — test documenting module name shadowing between stanzas and libraries
• test: verify library file deps in compilation rules and sandboxed builds (#4572) #14100 — test verifying library file deps in compilation rules and sandboxed builds
• test: verify transparent alias incremental build safety (#4572) #14101 — safety test for incremental builds with transparent aliases
• test: verify incremental builds with alias re-exported libraries (#4572) #14129 — test verifying incremental builds with alias re-exported libraries
• test: document ocamldep behavior with transparent alias chains #14178 — test documenting ocamldep behavior with transparent alias chains

Fixes #4572

[robinbb]

Tagging folks who were interested in the now-deprecated PR 14021. This one supersedes that one, with a more elegant solution, and tests that actually pass. It is out of draft mode as of now.
@rgrinberg @Leonidas-from-XIV @Khady @nojb @Alizter @art-w

[robinbb]

I need help running the so-called revdeps tests, as I have no credentials on this repo and therefore cannot initiate CI runs manually. I also would like to be instructed on how I can compare the results of the revdeps tests on this branch versus those on 'main'.

[robinbb]

Note: reduced package-level cycle detection

This PR removes the @package-cycle test case from reporting-of-cycles.t. The underlying cycle still exists (library a.1 declares (libraries b), library b declares (libraries a.2)), but the modules in those libraries are empty — they don't reference any library in their source code. With per-module filtering, the compilation graph no longer has inter-library edges for those modules, so the package-level cycle is no longer detected.

This is an inherent trade-off of the approach: package cycle detection previously piggybacked on compilation-level glob deps as a proxy for declared library dependencies. More precise per-module deps mean that proxy no longer exists for modules that don't actually import their declared libraries.

This only affects the case where a module declares a library dependency but never references it in source code. In practice, this is uncommon — but it does mean some package cycles will only be caught at opam-level resolution rather than at build time.

[art-w]

Very nice! I left some comments regarding the remaining edge-cases where module aliases might cause issues with other features :)

[rgrinberg]

Note that this PR slows null builds by 30% (see the benchmark job). It is reasonable to expect some overhead from this refining step, but we should strive to reduce it. Getting it within 5% would be ideal.

[robinbb]

In conclusion, there's a lot more work than just stat'ing. Dune's internal representation of the rules has to be built from scratch from a very high level description and it is hard to make that instant.

Yes, no argument there. I was just giving a reasonable bound on the 'stat' ing part of what must be done.

[robinbb]

[...] dune is doing much more than in a null build:

1. It is parsing every dune file
2. It is generating the rules for every targets it needs to validate. This is non trivial and requires creating around 10 targets for every source that we've stat'ed.
3. Then it is confirming that each of those rules is up to date by computing the rule digest against what it saved in the last run

Oh! I would have guessed that it first loads its own cached data, and uses that to find the lastModified times of the dune files. It then stats the dune files. Then, if the dune files are older than the cached data, it uses its cached version of the dependency graph. This would require no dune file parsing, no rules logic. It still must stat the files referenced in the deps graphs, however.

The 3-step procedure that you mention is what I would expect happens when the dune files are found to be newer than the cached data.

[rgrinberg]

To do something like that dune would need a way to serialize its dependency graph. Turns out that's not so easy.

We have a watch mode that makes this issue a lot more bearable for large workspaces, but null build times are still quite important.

[robinbb]

We have a watch mode that makes this issue a lot more bearable for large workspaces, but null build times are still quite important.

No disagreement from me on that point. However, this PR is now at an impasse. It provides a lot of progress toward the agreed-upon goal of refining and improving the dependency graph - something that is crucial for improving the incremental build situation (and with possible improvements to the build time on clean builds, too). However, this refined build graph is not guaranteed to produce a wall clock time improvement on incremental builds for every build. Unfortunately, this PR produces a null build time regression. If we do not merge this, then we have no progress toward our refined build graph goal, and issue 4572 remains unresolved. If we do merge it, null builds take longer. I propose that this is a "two steps forward, one step back" situation, and that it's worth merging despite the drawback.

[nojb]

Question from a distance (sorry if this has already been mentioned in the discussion): is the root cause of the slowdown understood, at least in principle? And, if so, are there potential approaches available that could be used to mitigate the slowdown in future work?

[rgrinberg]

I am not advocating for blocking this PR until we fix all the performance problems, but it just seems like we're giving up without really trying. If the null build isn't minimized, I think that dune's rules will eventually converge on having both modes and users choosing between the two depending on release vs dev builds. I think that would be suboptimal and a maintenance headache, so I would really prefer to address performance issues early.

In the interest of making progress on this PR, I'll propose some actionable steps that we can take to improve performance:

1. Could you cherry pick my commit in refactor: memoize library closure #14256 into this branch? It should apply cleanly and perhaps reduce the cost of all of these closure.

2. Could you address the double parsing issue of ocamldep files? We've discussed leaving this for later, but parsing ocamldep files is on the hot path for null builds.

After these two steps, we can consider merging and solving the performance issues separately. I think that's a fair compromise.

@art-w could you have another go at reviewing this PR and perhaps pointing out any other performance issues?

@nojb do you mind benchmarking this on your internal code base to see if this improves anything (clean or incremental builds)?

[nojb]

@nojb do you mind benchmarking this on your internal code base to see if this improves anything (clean or incremental builds)?

Will do. It will take me a few days to get around to it, though. I will post back here with the results.

[robinbb]

@rgrinberg Thanks for your suggestions, and thanks for the coding effort, too. I have cherry-picked your commit. I have also addressed the other issue, if I have understood correctly.

[robinbb]

Question from a distance (sorry if this has already been mentioned in the discussion): is the root cause of the slowdown understood, at least in principle? And, if so, are there potential approaches available that could be used to mitigate the slowdown in future work?

Short answer is 'no', hence @rgrinberg's call to investigate further.

[robinbb]

@rgrinberg Thanks for your suggestions, and thanks for the coding effort, too. I have cherry-picked your commit. I have also addressed the other issue, if I have understood correctly.

Together, they have made an impact, but not a big one. The null build time was 6.6 seconds.

[robinbb]

On PR #14273 I'm trying another experiment to try to reduce the null build time.