perf(core): tighten resolve() sorting to drop the quadratic hot paths#89
Conversation
resolve() re-sorted the Kahn queue on every pop (O(V^2 log V)) and rebuilt each depth level with a filter-and-sort over the full node array (O(V*D), quadratic on a linear chain). This is Candidate 3 of issue #46. The output (levels and order) is derived only from the depth map and a per-level alphabetical sort: the depth map is the longest path from a root and is order-invariant (a node is dequeued only after every predecessor is processed, so its depth is final when read), and each level is sorted alphabetically. The dequeue order therefore has no effect on output. So: walk the alphabetically-seeded queue with a FIFO index pointer instead of re-sorting on every pop, drop the per-node dependents sort, and build the levels in a single O(V) pass over the pre-sorted names array (which drops each node into its depth bucket already in alphabetical order) instead of filter-and-sort per level. Determinism is preserved exactly. A golden-master baseline captures the pre-optimisation resolve() output across linear chain, diamond, wide fan-out, multiple roots, disconnected components, cross-level depth, reverse-name ordering, and the cycle / self-dependency / missing-dependency error shapes; the optimised resolve() reproduces every snapshot byte-for-byte. The baseline was verified to fail under a perturbed level ordering, so it genuinely guards determinism rather than passing vacuously. Proof (npm run bench, resolve on a 1000-step linear chain, this machine): best 21.2ms before, 1.19ms after (~18x), widening the margin against the 800ms assertion from ~38x to ~670x. Thresholds left unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 15 minutes and 7 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
1 participant
Issue #46, Candidate 3 (PR 4 of 4, the final algorithmic fix). Follows the scaffold (#53), compiler (#87), and expression cache (#88).
Summary
resolve()(Kahn's topological sort) had two quadratic hot paths:queue.sort()on every pop, O(V^2 log V).sorted.filter(...).sort(), O(V*D), which is quadratic on a linear chain.The fix:
namesarray, which drops each node into its depth bucket already in alphabetical order, instead of filter-and-sort per level.Determinism (the hard constraint)
Output (
levels,order, and error shapes) is byte-identical. The output is a pure function of thedepthmap and the per-level alphabetical sort.depthis the longest path from a root and is dequeue-order-invariant: a node is enqueued only after every predecessor has been processed (the in-degree gate), so its depth is final when read, regardless of queue discipline. The dequeue order only ever affected the internalsortedtraversal array, which feeds nothing observable except theunreachableerror branch, and that branch is provably empty on the no-cycle path (every node insortedis reachable from a root).Determinism-baseline test approach
A golden-master baseline:
resolve()output was captured from the pre-optimisation implementation onmainacross linear chain, diamond, wide fan-out, multiple roots, disconnected components, cross-level depth, reverse-name ordering, an alphabetical-vs-topological conflict shape, and the cycle / self-dependency / missing-dependency error shapes. The optimisedresolve()reproduces every snapshot byte-for-byte (toEqualagainst embedded literals). The baseline was verified to FAIL under a perturbed level ordering, so it guards determinism rather than passing vacuously.Proof (npm run bench, Candidate-3 assertion: resolve on a 1000-step linear chain)
~18x faster; the assertion passes with a far wider margin. Thresholds left unchanged per the issue's calibration guidance.
Scope
packages/core/dag.ts+ its tests only.Test Plan
npm run build:core&&npm run buildnpm test(511 passed)npm run typechecknpm run lint(no new errors; 5 pre-existing out-of-scope errors unchanged)node spec/fixtures/run-fixtures.mjs(29 passed)npm run bench(3 passed, Candidate-3 margin widened)Closes #46 (final of the 4-PR sequence: #53 scaffold, #87 compiler, #88 expression cache, this).
🤖 Generated with Claude Code
Summary by cubic
Tightened
resolve()to remove two quadratic hot paths, delivering ~18x faster DAG sorting with byte-identical output and alphabetical per-level ordering. Final algorithmic fix for issue #46.queue.sort()on each pop) and drop per-node dependents sorting.namesinto depth slots, preserving alphabetical order within each level.Written for commit 6f7f665. Summary will update on new commits. Review in cubic