Repository navigation
Compile character range patterns in linear time - #8868
Conversation
|
@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. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8868 +/- ##
==========================================
+ Coverage 80.99% 81.02% +0.02%
==========================================
Files 453 453
Lines 62989 63030 +41
==========================================
+ Hits 51020 51070 +50
+ Misses 11969 11960 -9
🚀 New features to boost your workflow:
|
Skip the no-op alpha renaming of variable-free or-patterns, and find pattern groups, default matrix minima and switch cells by hashing. Fixes #8866. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDBKAdTcAxA3HsjtVESRCr Signed-off-by: Christoph Knittel <ck@cca.io>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDBKAdTcAxA3HsjtVESRCr Signed-off-by: Christoph Knittel <ck@cca.io>
d61ce0b to
4bb9898
Compare
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@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-8868 |
cristianoc
left a comment
There was a problem hiding this comment.
TIL this was not done in linear time earlier
Fixes #8866.
A character range pattern is expanded into one or-pattern alternative per character. Three steps were quadratic in the number of alternatives:
Typecore,Ppat_or): each level of the nested or-pattern ranalpha_patover the entire right subtree, copying it even when there are no variables to rename. This caused the memory blowup. Whenalpha_envis empty the copy is now skipped.enter_orpat_variableshas already checked that both sides bind the same (here: no) variables, so it would be a no-op.Parmatch.filter_all): every row did a linear search for its group. For int, char and string constants and for polymorphic variants,simple_matchis equality, so their groups are now looked up by hashing. Group order and row order stay the same.Parmatch.get_mins le_pats, used byMatching.as_matrix,check_unused,check_partial): compared all pairs of rows. The newget_mins_patsonly compares a row starting with an int/char/string constant with rows starting with the same constant or with a different kind of pattern. No other row can cover it, so the result is identical.Matching.dividefor constants): the cell for each key was found withList.find; it now uses a hash table for int/char/string keys. Floats and bigints keep the linear search, since they compare semantically (1.0=1.).The hash tables are created lazily, because most calls are on small matrices.
'\u{D7FE}' .. '\u{E001}'(~2k chars)'\u{100}' .. '\u{1100}'(~4k chars)'\u{4E00}' .. '\u{9FFF}'(CJK, ~21k chars)'\u{0}' .. '\u{FFFF}'(65k chars)Generated JavaScript and warnings are byte-identical to master across
tests/tests/src, thesuper_errorsfixtures, the runtime, and a stress file. That file has overlapping ranges, duplicate int/string/float/bigint/polyvariant cases, tuples with mixed heads, guards, constructors and arrays, and exercises warnings 8, 11 and 12. On ordinary code (tests/tests + Belt), CPU time is unchanged within noise and allocation is up about 0.5%.Big int/string switches are not fully linear yet. The remaining cost is
check_unusedchecking each row against the previous ones.New test:
tests/tests/src/char_range_test.rescovers the CJK block range and several adjacent ranges, checking boundaries at runtime. Before this change the compiler ran out of memory on it (killed after 39 s at 15 GB).🤖 Generated with Claude Code
https://claude.ai/code/session_01KDBKAdTcAxA3HsjtVESRCr