Repository navigation
fix(core): replace per-input visited clones with undo-log scoping in hash planner - #36248
Merged
Merged
Conversation
…hash planner The sibling-inputs fix in #35071 scoped cycle detection per dependency input by cloning the whole visited set at every traversal node — O(set) allocation + rehash per input per node, across all rayon threads. Bisected to +91 MiB peak RSS on a 500k-file workspace. Keep the same per-input scoping with an undo log instead: record each input's insertions and roll exactly those back when the input finishes. Identical visitation semantics (the single-input fast path still persists visits, the multi-input loop still leaves the caller's set untouched), zero clones, and one hash lookup per visit instead of two. Also drops the needless Box around the set. NXC-4605
✅ Deploy Preview for nx-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for nx-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
View your CI Pipeline Execution ↗ for commit 969edb1
☁️ Nx Cloud last updated this comment at |
leosvelperez
approved these changes
Jul 7, 2026
FrozenPandaz
added a commit
that referenced
this pull request
Jul 7, 2026
…36249) ## Current Behavior Hash plans are `HashMap<task, Vec<HashInstruction>>` — every task owns deep copies of every instruction in its dependency closure. The distinct-instruction population is only a few thousand values (a couple per project × input), but plans store them O(tasks × closure) times: ~1.1M copies × ~200 bytes on a 1,110-task benchmark with deep closures. This is the structure that #35071's sibling-inputs correctness fix legitimately inflated (the real mechanism behind NXC-4605's +91 MiB), and it exists in every parallel DTE agent process (#36152). ## Expected Behavior Plans store `u32` ids into an `InstructionPool` interner and travel with it as `HashPlans` through the planner→hasher `External`: - The interner's entry()-serialized id allocation guarantees value-equal ⇒ id-equal, so integer sort+dedup ≡ value-level dedup. - Dependency subtrees are memoized per (project, propagated input) as id lists and spliced into plans as integer memcpy — zero materialization on the O(tasks × closure) path. Deps-outputs subtrees and cyclic graphs use the existing per-task traversal, interned at the boundary. - `task_hasher` / `hash_plan_inspector` resolve ids in place; the string-returning `getPlans` API materializes and Ord-sorts, keeping observable behavior identical. **Measured** (densified bench: 1,110 tasks, avg closure ~555, 3 runs): planning 373 ms → 144 ms (~2.6×), peak process RSS 947 MB → 658 MB (−31%), `hash_plans` unchanged. Task hashes byte-for-byte identical — 72 TS hasher+planner tests and all Rust tests pass. Negative control: the same memo returning materialized instructions measured 2.8× slower; the representation change is the entire win. > [!NOTE] > **Stacked on #36248** (uses its `VisitedTracker`); retarget to `master` once that merges. Draft pending: full e2e sweep, a committed `bench:plan` variant (the stock benchmarks workspace has no dependency inputs and cannot exercise this path), scoped clone of the pool `Ref` in the Runtime hash arm, and a proper opaque d.ts type name. ## Related Issue(s) Fixes NXC-4607 <!-- polygraph-session-start --> --- [View session information ↗](https://app.trypolygraph.com/orgs/6a061dcb561c062131116eca/sessions/NXC-4603-Workspace-Fileset-Cache-Memory-Fix-69f28e06) <!-- polygraph-session-end --> --------- Co-authored-by: nx-cloud[bot] <71083854+nx-cloud[bot]@users.noreply.github.com>
FrozenPandaz
added a commit
that referenced
this pull request
Jul 9, 2026
…hash planner (#36248) ## Current Behavior The sibling-inputs correctness fix in #35071 scopes cycle detection per dependency input by cloning the entire `visited` set at every node of the recursive plan traversal (`hash_planner.rs`: `Box::new((**visited).clone())` per input). The set grows toward transitive-closure size, so on large workspaces this is an O(set) allocation and rehash per input per traversal node, across all rayon planning threads. A memory bisect isolated this at +91 MiB peak RSS per process on a 500k-file workspace (cold path; planning runs every invocation, so warm too). ## Expected Behavior Same per-input scoping, zero clones: a `VisitedTracker` records each input's insertions in an undo log and rolls exactly those back when the input finishes. Every `visited` check during an input's traversal sees precisely what it saw before (entry state plus that input's own visits), the single-input fast path still persists visits, and the multi-input loop still leaves the caller's set untouched — so emitted instructions, and therefore hashes, are byte-for-byte identical. `visit()` also collapses the previous contains-then-insert double lookup into one, and the needless `Box` around the set is gone. The sibling-input regression tests added by #35071 (`planner.spec.ts`, `native-task-hasher-impl.spec.ts`) pass unchanged, plus new unit tests for the tracker's scope/rollback behavior. ## Related Issue(s) Fixes NXC-4605 Third of three cold-path regressions behind #36152: #34971 +218 MiB (#36244), #35248 +122 MiB (#36247), #35071 +91 MiB (this PR). <!-- polygraph-session-start --> --- [View session information ↗](https://app.trypolygraph.com/orgs/6a061dcb561c062131116eca/sessions/NXC-4603-Workspace-Fileset-Cache-Memory-Fix-69f28e06) <!-- polygraph-session-end --> (cherry picked from commit d4856f5)
FrozenPandaz
added a commit
that referenced
this pull request
Jul 9, 2026
…36249) ## Current Behavior Hash plans are `HashMap<task, Vec<HashInstruction>>` — every task owns deep copies of every instruction in its dependency closure. The distinct-instruction population is only a few thousand values (a couple per project × input), but plans store them O(tasks × closure) times: ~1.1M copies × ~200 bytes on a 1,110-task benchmark with deep closures. This is the structure that #35071's sibling-inputs correctness fix legitimately inflated (the real mechanism behind NXC-4605's +91 MiB), and it exists in every parallel DTE agent process (#36152). ## Expected Behavior Plans store `u32` ids into an `InstructionPool` interner and travel with it as `HashPlans` through the planner→hasher `External`: - The interner's entry()-serialized id allocation guarantees value-equal ⇒ id-equal, so integer sort+dedup ≡ value-level dedup. - Dependency subtrees are memoized per (project, propagated input) as id lists and spliced into plans as integer memcpy — zero materialization on the O(tasks × closure) path. Deps-outputs subtrees and cyclic graphs use the existing per-task traversal, interned at the boundary. - `task_hasher` / `hash_plan_inspector` resolve ids in place; the string-returning `getPlans` API materializes and Ord-sorts, keeping observable behavior identical. **Measured** (densified bench: 1,110 tasks, avg closure ~555, 3 runs): planning 373 ms → 144 ms (~2.6×), peak process RSS 947 MB → 658 MB (−31%), `hash_plans` unchanged. Task hashes byte-for-byte identical — 72 TS hasher+planner tests and all Rust tests pass. Negative control: the same memo returning materialized instructions measured 2.8× slower; the representation change is the entire win. > [!NOTE] > **Stacked on #36248** (uses its `VisitedTracker`); retarget to `master` once that merges. Draft pending: full e2e sweep, a committed `bench:plan` variant (the stock benchmarks workspace has no dependency inputs and cannot exercise this path), scoped clone of the pool `Ref` in the Runtime hash arm, and a proper opaque d.ts type name. ## Related Issue(s) Fixes NXC-4607 <!-- polygraph-session-start --> --- [View session information ↗](https://app.trypolygraph.com/orgs/6a061dcb561c062131116eca/sessions/NXC-4603-Workspace-Fileset-Cache-Memory-Fix-69f28e06) <!-- polygraph-session-end --> --------- Co-authored-by: nx-cloud[bot] <71083854+nx-cloud[bot]@users.noreply.github.com> (cherry picked from commit d6e9534)
AgentEnder
pushed a commit
that referenced
this pull request
Sep 10, 2026
…hash planner (#36248) ## Current Behavior The sibling-inputs correctness fix in #35071 scopes cycle detection per dependency input by cloning the entire `visited` set at every node of the recursive plan traversal (`hash_planner.rs`: `Box::new((**visited).clone())` per input). The set grows toward transitive-closure size, so on large workspaces this is an O(set) allocation and rehash per input per traversal node, across all rayon planning threads. A memory bisect isolated this at +91 MiB peak RSS per process on a 500k-file workspace (cold path; planning runs every invocation, so warm too). ## Expected Behavior Same per-input scoping, zero clones: a `VisitedTracker` records each input's insertions in an undo log and rolls exactly those back when the input finishes. Every `visited` check during an input's traversal sees precisely what it saw before (entry state plus that input's own visits), the single-input fast path still persists visits, and the multi-input loop still leaves the caller's set untouched — so emitted instructions, and therefore hashes, are byte-for-byte identical. `visit()` also collapses the previous contains-then-insert double lookup into one, and the needless `Box` around the set is gone. The sibling-input regression tests added by #35071 (`planner.spec.ts`, `native-task-hasher-impl.spec.ts`) pass unchanged, plus new unit tests for the tracker's scope/rollback behavior. ## Related Issue(s) Fixes NXC-4605 Third of three cold-path regressions behind #36152: #34971 +218 MiB (#36244), #35248 +122 MiB (#36247), #35071 +91 MiB (this PR). <!-- polygraph-session-start --> --- [View session information ↗](https://app.trypolygraph.com/orgs/6a061dcb561c062131116eca/sessions/NXC-4603-Workspace-Fileset-Cache-Memory-Fix-69f28e06) <!-- polygraph-session-end --> Backport note: 22.7.x had already reshaped `external_deps_mapped` to a borrowed map, which collides textually with this change to `visited` in the same four parameter lists. The two are orthogonal, so each resolution keeps 22.7.x's `external_deps_mapped` and takes this commit's `VisitedTracker`.
AgentEnder
pushed a commit
that referenced
this pull request
Sep 10, 2026
…36249) ## Current Behavior Hash plans are `HashMap<task, Vec<HashInstruction>>` — every task owns deep copies of every instruction in its dependency closure. The distinct-instruction population is only a few thousand values (a couple per project × input), but plans store them O(tasks × closure) times: ~1.1M copies × ~200 bytes on a 1,110-task benchmark with deep closures. This is the structure that #35071's sibling-inputs correctness fix legitimately inflated (the real mechanism behind NXC-4605's +91 MiB), and it exists in every parallel DTE agent process (#36152). ## Expected Behavior Plans store `u32` ids into an `InstructionPool` interner and travel with it as `HashPlans` through the planner→hasher `External`: - The interner's entry()-serialized id allocation guarantees value-equal ⇒ id-equal, so integer sort+dedup ≡ value-level dedup. - Dependency subtrees are memoized per (project, propagated input) as id lists and spliced into plans as integer memcpy — zero materialization on the O(tasks × closure) path. Deps-outputs subtrees and cyclic graphs use the existing per-task traversal, interned at the boundary. - `task_hasher` / `hash_plan_inspector` resolve ids in place; the string-returning `getPlans` API materializes and Ord-sorts, keeping observable behavior identical. **Measured** (densified bench: 1,110 tasks, avg closure ~555, 3 runs): planning 373 ms → 144 ms (~2.6×), peak process RSS 947 MB → 658 MB (−31%), `hash_plans` unchanged. Task hashes byte-for-byte identical — 72 TS hasher+planner tests and all Rust tests pass. Negative control: the same memo returning materialized instructions measured 2.8× slower; the representation change is the entire win. > [!NOTE] > **Stacked on #36248** (uses its `VisitedTracker`); retarget to `master` once that merges. Draft pending: full e2e sweep, a committed `bench:plan` variant (the stock benchmarks workspace has no dependency inputs and cannot exercise this path), scoped clone of the pool `Ref` in the Runtime hash arm, and a proper opaque d.ts type name. ## Related Issue(s) Fixes NXC-4607 <!-- polygraph-session-start --> --- [View session information ↗](https://app.trypolygraph.com/orgs/6a061dcb561c062131116eca/sessions/NXC-4603-Workspace-Fileset-Cache-Memory-Fix-69f28e06) <!-- polygraph-session-end --> --------- Co-authored-by: nx-cloud[bot] <71083854+nx-cloud[bot]@users.noreply.github.com> Backport notes: - 22.7.x builds `external_deps_mapped` per get_plans call rather than memoizing it on the planner, so the struct keeps no such field here. The subtree memo is still safe: its values derive only from the project graph and nx_json, which are immutable either way. - `memoized_dep_subtree` and `compute_dep_subtree` take 22.7.x borrowed `hashbrown::HashMap<&String, Vec<&String>>` instead of the owned map. - `OnceCache` comes from #36244, which is otherwise skipped: 22.7.x has no workspace fileset cache for that PR to fix. Only the type is taken.
AgentEnder
pushed a commit
that referenced
this pull request
Sep 10, 2026
…36249) Hash plans are `HashMap<task, Vec<HashInstruction>>` — every task owns deep copies of every instruction in its dependency closure. The distinct-instruction population is only a few thousand values (a couple per project × input), but plans store them O(tasks × closure) times: ~1.1M copies × ~200 bytes on a 1,110-task benchmark with deep closures. This is the structure that #35071's sibling-inputs correctness fix legitimately inflated (the real mechanism behind NXC-4605's +91 MiB), and it exists in every parallel DTE agent process (#36152). Plans store `u32` ids into an `InstructionPool` interner and travel with it as `HashPlans` through the planner→hasher `External`: - The interner's entry()-serialized id allocation guarantees value-equal ⇒ id-equal, so integer sort+dedup ≡ value-level dedup. - Dependency subtrees are memoized per (project, propagated input) as id lists and spliced into plans as integer memcpy — zero materialization on the O(tasks × closure) path. Deps-outputs subtrees and cyclic graphs use the existing per-task traversal, interned at the boundary. - `task_hasher` / `hash_plan_inspector` resolve ids in place; the string-returning `getPlans` API materializes and Ord-sorts, keeping observable behavior identical. **Measured** (densified bench: 1,110 tasks, avg closure ~555, 3 runs): planning 373 ms → 144 ms (~2.6×), peak process RSS 947 MB → 658 MB (−31%), `hash_plans` unchanged. Task hashes byte-for-byte identical — 72 TS hasher+planner tests and all Rust tests pass. Negative control: the same memo returning materialized instructions measured 2.8× slower; the representation change is the entire win. > [!NOTE] > **Stacked on #36248** (uses its `VisitedTracker`); retarget to `master` once that merges. Draft pending: full e2e sweep, a committed `bench:plan` variant (the stock benchmarks workspace has no dependency inputs and cannot exercise this path), scoped clone of the pool `Ref` in the Runtime hash arm, and a proper opaque d.ts type name. Fixes NXC-4607 <!-- polygraph-session-start --> --- [View session information ↗](https://app.trypolygraph.com/orgs/6a061dcb561c062131116eca/sessions/NXC-4603-Workspace-Fileset-Cache-Memory-Fix-69f28e06) <!-- polygraph-session-end --> --------- Co-authored-by: nx-cloud[bot] <71083854+nx-cloud[bot]@users.noreply.github.com> Backport notes: - 22.7.x builds `external_deps_mapped` per get_plans call rather than memoizing it on the planner, so the struct keeps no such field here. The subtree memo is still safe: its values derive only from the project graph and nx_json, which are immutable either way. - `memoized_dep_subtree` and `compute_dep_subtree` take 22.7.x borrowed `hashbrown::HashMap<&String, Vec<&String>>` instead of the owned map. - `OnceCache` comes from #36244, which is otherwise skipped: 22.7.x has no workspace fileset cache for that PR to fix. Only the type is taken.
FrozenPandaz
added a commit
that referenced
this pull request
Sep 10, 2026
…hash planner (#36248) ## Current Behavior The sibling-inputs correctness fix in #35071 scopes cycle detection per dependency input by cloning the entire `visited` set at every node of the recursive plan traversal (`hash_planner.rs`: `Box::new((**visited).clone())` per input). The set grows toward transitive-closure size, so on large workspaces this is an O(set) allocation and rehash per input per traversal node, across all rayon planning threads. A memory bisect isolated this at +91 MiB peak RSS per process on a 500k-file workspace (cold path; planning runs every invocation, so warm too). ## Expected Behavior Same per-input scoping, zero clones: a `VisitedTracker` records each input's insertions in an undo log and rolls exactly those back when the input finishes. Every `visited` check during an input's traversal sees precisely what it saw before (entry state plus that input's own visits), the single-input fast path still persists visits, and the multi-input loop still leaves the caller's set untouched — so emitted instructions, and therefore hashes, are byte-for-byte identical. `visit()` also collapses the previous contains-then-insert double lookup into one, and the needless `Box` around the set is gone. The sibling-input regression tests added by #35071 (`planner.spec.ts`, `native-task-hasher-impl.spec.ts`) pass unchanged, plus new unit tests for the tracker's scope/rollback behavior. ## Related Issue(s) Fixes NXC-4605 Third of three cold-path regressions behind #36152: #34971 +218 MiB (#36244), #35248 +122 MiB (#36247), #35071 +91 MiB (this PR). <!-- polygraph-session-start --> --- [View session information ↗](https://app.trypolygraph.com/orgs/6a061dcb561c062131116eca/sessions/NXC-4603-Workspace-Fileset-Cache-Memory-Fix-69f28e06) <!-- polygraph-session-end --> Backport note: 22.7.x had already reshaped `external_deps_mapped` to a borrowed map, which collides textually with this change to `visited` in the same four parameter lists. The two are orthogonal, so each resolution keeps 22.7.x's `external_deps_mapped` and takes this commit's `VisitedTracker`.
FrozenPandaz
added a commit
that referenced
this pull request
Sep 10, 2026
…36249) Hash plans are `HashMap<task, Vec<HashInstruction>>` — every task owns deep copies of every instruction in its dependency closure. The distinct-instruction population is only a few thousand values (a couple per project × input), but plans store them O(tasks × closure) times: ~1.1M copies × ~200 bytes on a 1,110-task benchmark with deep closures. This is the structure that #35071's sibling-inputs correctness fix legitimately inflated (the real mechanism behind NXC-4605's +91 MiB), and it exists in every parallel DTE agent process (#36152). Plans store `u32` ids into an `InstructionPool` interner and travel with it as `HashPlans` through the planner→hasher `External`: - The interner's entry()-serialized id allocation guarantees value-equal ⇒ id-equal, so integer sort+dedup ≡ value-level dedup. - Dependency subtrees are memoized per (project, propagated input) as id lists and spliced into plans as integer memcpy — zero materialization on the O(tasks × closure) path. Deps-outputs subtrees and cyclic graphs use the existing per-task traversal, interned at the boundary. - `task_hasher` / `hash_plan_inspector` resolve ids in place; the string-returning `getPlans` API materializes and Ord-sorts, keeping observable behavior identical. **Measured** (densified bench: 1,110 tasks, avg closure ~555, 3 runs): planning 373 ms → 144 ms (~2.6×), peak process RSS 947 MB → 658 MB (−31%), `hash_plans` unchanged. Task hashes byte-for-byte identical — 72 TS hasher+planner tests and all Rust tests pass. Negative control: the same memo returning materialized instructions measured 2.8× slower; the representation change is the entire win. > [!NOTE] > **Stacked on #36248** (uses its `VisitedTracker`); retarget to `master` once that merges. Draft pending: full e2e sweep, a committed `bench:plan` variant (the stock benchmarks workspace has no dependency inputs and cannot exercise this path), scoped clone of the pool `Ref` in the Runtime hash arm, and a proper opaque d.ts type name. Fixes NXC-4607 <!-- polygraph-session-start --> --- [View session information ↗](https://app.trypolygraph.com/orgs/6a061dcb561c062131116eca/sessions/NXC-4603-Workspace-Fileset-Cache-Memory-Fix-69f28e06) <!-- polygraph-session-end --> --------- Co-authored-by: nx-cloud[bot] <71083854+nx-cloud[bot]@users.noreply.github.com> Backport notes: - 22.7.x builds `external_deps_mapped` per get_plans call rather than memoizing it on the planner, so the struct keeps no such field here. The subtree memo is still safe: its values derive only from the project graph and nx_json, which are immutable either way. - `memoized_dep_subtree` and `compute_dep_subtree` take 22.7.x borrowed `hashbrown::HashMap<&String, Vec<&String>>` instead of the owned map. - `OnceCache` comes from #36244, which is otherwise skipped: 22.7.x has no workspace fileset cache for that PR to fix. Only the type is taken.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Current Behavior
The sibling-inputs correctness fix in #35071 scopes cycle detection per dependency input by cloning the entire
visitedset at every node of the recursive plan traversal (hash_planner.rs:Box::new((**visited).clone())per input). The set grows toward transitive-closure size, so on large workspaces this is an O(set) allocation and rehash per input per traversal node, across all rayon planning threads. A memory bisect isolated this at +91 MiB peak RSS per process on a 500k-file workspace (cold path; planning runs every invocation, so warm too).Expected Behavior
Same per-input scoping, zero clones: a
VisitedTrackerrecords each input's insertions in an undo log and rolls exactly those back when the input finishes. Everyvisitedcheck during an input's traversal sees precisely what it saw before (entry state plus that input's own visits), the single-input fast path still persists visits, and the multi-input loop still leaves the caller's set untouched — so emitted instructions, and therefore hashes, are byte-for-byte identical.visit()also collapses the previous contains-then-insert double lookup into one, and the needlessBoxaround the set is gone.The sibling-input regression tests added by #35071 (
planner.spec.ts,native-task-hasher-impl.spec.ts) pass unchanged, plus new unit tests for the tracker's scope/rollback behavior.Related Issue(s)
Fixes NXC-4605
Third of three cold-path regressions behind #36152: #34971 +218 MiB (#36244), #35248 +122 MiB (#36247), #35071 +91 MiB (this PR).
View session information ↗