perf: memoise per-(dep_m, ml_kind, cm_kind, is_consumer) raw-refs builder by robinbb · Pull Request #14454 · ocaml/dune

robinbb · 2026-05-07T22:26:13Z

Deprecated — superseded by #14474.

Summary

Stacked atop #14186. Addresses @art-w's review concern at https://github.com/ocaml/dune/pull/14116/files#r3116025155.

In module_compilation.ml's lib_deps_for_module, each consumer module iterates over m :: trans_deps and calls read_dep_m_raw per dep. Sibling consumers in the same stanza share large parts of trans_deps but used to reconstruct fresh Action_builder.t trees per call — the inner ocamldep result is shared via Ocamldep's path-keyed cache, but the wrapping need_impl_deps_of / Module_name.Set.union logic was rebuilt N×K times per stanza.

This PR adds a per-cctx Raw_refs.t = (Key.t, _ Action_builder.t) Table.t in Compilation_context, keyed on (obj_name, ml_kind, cm_kind, is_consumer). Table.find short-circuits before allocating, mirroring the pattern used by Ocamldep.read_immediate_deps_words's top-level cache.

Copilot

Pull request overview

Adds intra-Compilation_context memoisation for the “raw referenced modules” Action_builder.t constructed in lib_deps_for_module, reducing repeated Action_builder tree construction across sibling consumer modules that share large portions of trans_deps.

Changes:

Memoise per-Compilation_context raw reference builders keyed by (dep_m, ml_kind, cm_kind, is_consumer).
Switch module_compilation.ml to use the new Compilation_context.cached_raw_refs helper.
Document the new memoisation API in compilation_context.mli.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/dune_rules/module_compilation.ml	Uses `Compilation_context.cached_raw_refs` to reuse raw-ref builders across repeated dep reads.
src/dune_rules/compilation_context.mli	Exposes and documents the `cached_raw_refs` API.
src/dune_rules/compilation_context.ml	Implements a per-cctx `Raw_refs` cache and the `cached_raw_refs` accessor.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

robinbb · 2026-05-09T01:43:28Z

+           shareable across both passes. *)
+        let cache_ml_kind = if is_consumer then ml_kind else Ml_kind.Impl in
+        Compilation_context.cached_raw_refs
+          cctx
+          ~dep_m
+          ~ml_kind:cache_ml_kind
+          ~cm_kind


Correct: when is_consumer = true, the body is independent of cm_kind (consumer branch of need_impl_deps_of reads only ml_kind; Ocamldep.read_immediate_deps_raw_of is cm_kind-agnostic), so the Cmi/Cmo/Cmx triple produces three identical builders instead of sharing one.

The key is correct, just non-minimal. Wasted work is the wrapping let*/let+/union shell — the actual ocamldep calls are deduplicated by Ocamldep.read_immediate_deps_words. For dune-on-dune that's a few thousand redundant Action_builder.t cells, sub-ms time, low-MB memory. Below noise floor.

Deferring on cost-vs-churn. If a hot spot surfaces, the right shape is Consumer { obj_name; ml_kind } | Trans_dep { obj_name; cm_kind } — also collapses the symmetric ml_kind redundancy.

Copilot

Pull request overview

Copilot reviewed 81 out of 81 changed files in this pull request and generated 2 comments.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

+let cached_raw_refs t ~dep_m ~ml_kind ~cm_kind ~is_consumer compute =
+  let cache_key =
+    { Raw_refs.Key.obj_name = Module.obj_name dep_m; ml_kind; cm_kind; is_consumer }
+  in
+  match Table.find t.raw_refs cache_key with
+  | Some builder -> builder
+  | None ->
+    let builder = compute () in
+    Table.set t.raw_refs cache_key builder;
+    builder


…uilder In [module_compilation.ml]'s [lib_deps_for_module], each consumer module iterates over [m :: trans_deps] and calls [read_dep_m_raw] per dep. Sibling consumers in the same stanza share large parts of [trans_deps] but used to reconstruct fresh [Action_builder.t] trees per call — the inner [ocamldep] result is shared via [Ocamldep]'s path-keyed cache, but the wrapping [need_impl_deps_of] / [Module_name.Set.union] logic was rebuilt N×K times per stanza. Add a per-cctx [Raw_refs.t = (Key.t, _ Action_builder.t) Table.t] in [Compilation_context], keyed on (obj_name, ml_kind, cm_kind, is_consumer). [Table.find] short-circuits before allocating, mirroring the pattern used by [Ocamldep.read_immediate_deps_words]'s top-level cache. Two prior attempts at this memoisation failed: * Apr 21 (`e1b638664`, reverted): recursive memo across direct module deps; infinite loop on module-level cycles (`alias/check-alias/ocamldep-cycles.t`). * Apr 25 (`3a70bfaa0`, dropped): seen-set shape; OOM-killed CI because [Action_builder.memoize] dedupes evaluation by string key but does NOT dedupe construction. With N modules × M consumers, each call still allocated a fresh [Action_builder.t] tree before the memoize wrapper saw the key. This third attempt avoids both failure modes: the [Table.find] short-circuit prevents construction-time blowup, and the cache is intra-stanza only (the cross-library walk has its own [seen]-set termination), so module-level cycles are not visited by this loop. Addresses art-w's review concern at https://github.com/ocaml/dune/pull/14116/files#r3116025155 Signed-off-by: Robin Bate Boerop <me@robinbb.com>

robinbb · 2026-05-10T03:03:02Z

Continuing this thread on the active PR: #14474 (comment)

robinbb force-pushed the robinbb-issue-4572-filtered-includes branch from 763baad to 16ee216 Compare May 7, 2026 22:29

robinbb closed this May 7, 2026

robinbb reopened this May 7, 2026

robinbb self-assigned this May 7, 2026

robinbb requested a review from Copilot May 7, 2026 22:35

Copilot started reviewing on behalf of robinbb May 7, 2026 22:35 View session

robinbb force-pushed the robinbb-memoise-intra-stanza branch from 19088d9 to b7b7125 Compare May 7, 2026 22:36

Copilot AI reviewed May 7, 2026

View reviewed changes

Comment thread src/dune_rules/module_compilation.ml Outdated

robinbb force-pushed the robinbb-memoise-intra-stanza branch from b7b7125 to 59b113c Compare May 7, 2026 22:44

robinbb force-pushed the robinbb-issue-4572-filtered-includes branch from 16ee216 to b78aefa Compare May 7, 2026 23:36

robinbb force-pushed the robinbb-memoise-intra-stanza branch 5 times, most recently from a9ebe08 to 1368704 Compare May 8, 2026 07:25

robinbb force-pushed the robinbb-issue-4572-filtered-includes branch 2 times, most recently from 885ab2b to d10761d Compare May 8, 2026 15:37

robinbb force-pushed the robinbb-memoise-intra-stanza branch from 1368704 to 51fe109 Compare May 8, 2026 15:37

robinbb force-pushed the robinbb-issue-4572-filtered-includes branch from d10761d to be3c604 Compare May 8, 2026 18:10

robinbb force-pushed the robinbb-memoise-intra-stanza branch from 51fe109 to a76223e Compare May 8, 2026 18:11

robinbb force-pushed the robinbb-issue-4572-filtered-includes branch from be3c604 to 7d4b6ae Compare May 8, 2026 18:49

robinbb force-pushed the robinbb-memoise-intra-stanza branch 2 times, most recently from b482b94 to 516c6d4 Compare May 8, 2026 22:09

robinbb requested a review from Copilot May 8, 2026 22:13

Copilot started reviewing on behalf of robinbb May 8, 2026 22:14 View session

Copilot AI reviewed May 8, 2026

View reviewed changes

robinbb force-pushed the robinbb-memoise-intra-stanza branch from 516c6d4 to c66c893 Compare May 9, 2026 01:25

robinbb requested a review from Copilot May 9, 2026 01:28

Copilot started reviewing on behalf of robinbb May 9, 2026 01:29 View session

Copilot AI reviewed May 9, 2026

View reviewed changes

robinbb force-pushed the robinbb-memoise-intra-stanza branch from c66c893 to 715d202 Compare May 9, 2026 01:33

robinbb force-pushed the robinbb-issue-4572-filtered-includes branch from eb1af74 to 0fbc431 Compare May 9, 2026 03:48

robinbb requested a review from Copilot May 9, 2026 18:11

Copilot started reviewing on behalf of robinbb May 9, 2026 18:11 View session

Copilot AI reviewed May 9, 2026

View reviewed changes

Comment thread src/dune_rules/compilation_context.ml

Comment thread src/dune_rules/compilation_context.ml

robinbb force-pushed the robinbb-memoise-intra-stanza branch from 715d202 to 6b72386 Compare May 9, 2026 20:35

robinbb requested a review from Copilot May 9, 2026 20:44

Copilot started reviewing on behalf of robinbb May 9, 2026 20:45 View session

Copilot AI reviewed May 9, 2026

View reviewed changes

robinbb force-pushed the robinbb-issue-4572-filtered-includes branch from 0fbc431 to 1e0dfeb Compare May 10, 2026 00:17

robinbb force-pushed the robinbb-memoise-intra-stanza branch from 6b72386 to 93a528b Compare May 10, 2026 02:59

robinbb mentioned this pull request May 10, 2026

perf: memoise per-(dep_m, ml_kind, cm_kind, is_consumer) raw-refs builder #14474

Closed

robinbb closed this May 10, 2026

robinbb deleted the robinbb-memoise-intra-stanza branch May 10, 2026 03:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: memoise per-(dep_m, ml_kind, cm_kind, is_consumer) raw-refs builder#14454

perf: memoise per-(dep_m, ml_kind, cm_kind, is_consumer) raw-refs builder#14454
robinbb wants to merge 1 commit into
ocaml:robinbb-issue-4572-filtered-includesfrom
robinbb:robinbb-memoise-intra-stanza

robinbb commented May 7, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

robinbb May 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

robinbb commented May 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

robinbb commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

robinbb May 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

robinbb commented May 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robinbb commented May 7, 2026 •

edited

Loading