Repository navigation
Split let rec groups in linear time without the ordered hash map stack - #9153
Conversation
Changelog💅 Polish
Semantic Diff3 commits · Full page Split let rec groups with plain arrays instead of an ordered hash map2e7b96b · cknitt + Claude · Oct 31, 2026, 10:16 UTC Lambda_scc.preprocess_deps was the only user of Ordered_hash_map and Hash_set_ident_mask (468 lines with their test files). It now numbers the bindings with an array and a Hash_ident table, and collects each body's references in a bool array, still stopping the walk once every binding is hit. Edges come out in binding order; the generated JavaScript is unchanged across the test suites. Find the edges of a let rec group in time linear in its bodies707e268 · cknitt + Claude · Oct 31, 2026, 10:44 UTC Each body allocated and scanned a bool array as long as the group, so a group of n bindings cost O(n^2) on top of walking the bodies. One shared array now records which body last referred to each binding, and each body's edges are collected as found and sorted. A generated group of 16,000 bindings compiles in 2.7 s instead of 4.3 s (5.3 s on master); the generated JavaScript is unchanged. ✨ Test how let rec groups are split into componentsb23f519 · cknitt + Claude · Nov 01, 2026, 06:55 UTC · new Codex review: the rewrite of preprocess_deps removed the old modules' tests and added none. ounit_lambda_scc_tests checks bind_rec's output: an edge to a binding an earlier body also referred to survives the shared seen array (a mutation that leaks it between bodies fails the test), a mixed group keeps cycles, self-recursion and plain lets apart in dependency order, and a body referring to every binding stops early correctly. let_rec_components_test runs such a group end to end. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9153 +/- ##
==========================================
- Coverage 81.74% 81.73% -0.02%
==========================================
Files 460 455 -5
Lines 64944 64793 -151
==========================================
- Hits 53091 52957 -134
+ Misses 11853 11836 -17
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-9153 |
Lambda_scc.preprocess_deps was the only user of Ordered_hash_map and Hash_set_ident_mask (468 lines with their test files). It now numbers the bindings with an array and a Hash_ident table, and collects each body's references in a bool array, still stopping the walk once every binding is hit. Edges come out in binding order; the generated JavaScript is unchanged across the test suites. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gKyiYMf54wPRxFdUWe Signed-off-by: Christoph Knittel <ck@cca.io>
Each body allocated and scanned a bool array as long as the group, so a group of n bindings cost O(n^2) on top of walking the bodies. One shared array now records which body last referred to each binding, and each body's edges are collected as found and sorted. A generated group of 16,000 bindings compiles in 2.7 s instead of 4.3 s (5.3 s on master); the generated JavaScript is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gKyiYMf54wPRxFdUWe Signed-off-by: Christoph Knittel <ck@cca.io>
0dc558d to
707e268
Compare
cknitt
changed the title
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 707e2680c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review: the rewrite of preprocess_deps removed the old modules' tests and added none. ounit_lambda_scc_tests checks bind_rec's output: an edge to a binding an earlier body also referred to survives the shared seen array (a mutation that leaks it between bodies fails the test), a mixed group keeps cycles, self-recursion and plain lets apart in dependency order, and a body referring to every binding stops early correctly. let_rec_components_test runs such a group end to end. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gKyiYMf54wPRxFdUWe Signed-off-by: Christoph Knittel <ck@cca.io>
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Splitting a
let recgroup into its recursive components was the only user ofOrdered_hash_map{,_gen,_local_ident},Hash_set_ident_maskandExt_util.power_2_above. Together with their two ounit test files that's 468 lines. It was also O(n²) in the size of the group. (The@derivingregistry removal that was here is now #9157.)Commit 1: plain arrays.
preprocess_depsnumbers the bindings with an array and aHash_identtable, and records which bindings each body refers to, still stopping the walk once a body has referred to every binding. Edges now come out in binding order rather than the mask's hash order.Commit 2: linear. Each body allocated and scanned a
boolarray as long as the group. One shared array now records which body last referred to each binding, so finding the edges costs the size of the bodies.The generated JavaScript is unchanged across the test suites, and also for a hand-written group of 9 bindings that splits into several components.
Tests (third commit):
ounit_lambda_scc_tests.mlchecksbind_rec's output: an edge to a binding that an earlier body also referred to survives the shared array (a mutation that leaks it between bodies makes this fail); a mixed group keeps cycles, self-recursion and plain lets apart, in dependency order; and a body referring to every binding hits the early stop correctly.let_rec_components_test.resruns such a group end to end.Performance
Measured against master
bsc(user time, min of 5 or 9 runs; JS identical in every case).Large generated groups, each binding calling the next one and the first:
Normal code: 2,000 groups of each size. Time is unchanged within noise, and allocation goes up by about 0.1–0.15%, roughly 30 words for a 2-binding group:
Peak memory is unchanged. What remains superlinear for huge groups is in the type checker:
Typecore.enter_variablechecks each pattern variable for a duplicate name by scanning all variables bound so far in the group, worth about another 25% at 16,000 bindings. That's for a separate PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01Kbf3gKyiYMf54wPRxFdUWe