Repository navigation
Skip unneeded work in occur checks and primitive specialization - #9193
Merged
Merged
Conversation
JonoPrest
added a commit
that referenced
this pull request
Nov 3, 2026
The entry names #9193. Signed-off-by: Jono Prest <jjprest@gmail.com>
Contributor
Author
|
@codex review |
Changelog💅 Polish
Semantic Diff4 commits · Full page Try a cheap bounded walk before the full occur checks0dd61d0 · JonoPrest · Nov 03, 2026, 09:27 UTC
Both checks now first walk the type with no visited set and no marks, giving up after 256 nodes (which also stops cycles and heavy sharing); when the walk gives up, the unchanged full walk runs. - Review
Skip scraping a primitive's type when no specialization reads it184ee39 · JonoPrest · Nov 03, 2026, 09:27 UTC
For a function type whose primitive is in neither table, it now calls Review
Build the occur walk's closure once per call6c4bc78 · JonoPrest · Nov 09, 2026, 13:10 UTC
Measured with scripts/compilerBenchmarks.js against the previous commit: allocated words -1.58% in total, stress/TypeAliasChain.res -53%, Belt and runtime files -2% to -2.7%. On a 552 module React app (clean build, one domain) the build allocates 0.05% fewer words. A version that returns a bool instead of raising allocated more than this one (-1.27% in total), because Walk the occur fast paths without allocating679fbb9 · JonoPrest · Nov 09, 2026, 14:33 UTC
Measured with scripts/compilerBenchmarks.js against the previous commit: allocated words -1.03% in total (-1.14% without the project builds), stress/TypeAliasChain.res -8.3%, Belt map and set modules -2.7% to -3.4%. On a 552 module React app (clean build, one domain) the build allocates 0.99% fewer words. Keeping the closures and only skipping the walk for a leaf type (a variable or a constructor without arguments) gave -0.57% in total and -0.54% on the app. 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. 🚀 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9193 +/- ##
=======================================
Coverage 81.97% 81.97%
=======================================
Files 456 456
Lines 65613 65671 +58
=======================================
+ Hits 53783 53836 +53
- Misses 11830 11835 +5
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
JonoPrest
marked this pull request as ready for review
November 3, 2026 09:46
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-9193 |
JonoPrest
force-pushed
the
jono/occur-check-fast-path
branch
4 times, most recently
from
November 9, 2026 07:18
4eb8cf5 to
ece2955
Compare
🎉 Compiler benchmarksAllocated wordsIn total: 683,552,659 → 640,795,881 (-6.26%). 🎉 Improvement: 152 benchmarks allocate more than 1% less.
233 more changes
InstructionsIn total: 9,664,672,172 → 9,211,217,327 (-4.69%). 🎉 Improvement: 12 benchmarks execute more than 1% fewer instructions.
13 more changes
Output sizeIn total: 25,569,598 → 25,570,980 (+0.01%). None of the 13 benchmarks changed by 0.1% or more. Pull request 679fbb9 compared with base b82d944 in this run. |
JonoPrest
force-pushed
the
jono/occur-check-fast-path
branch
6 times, most recently
from
November 9, 2026 22:04
679fbb9 to
2c6629c
Compare
Changelog💅 Polish
Semantic Diff4 commits · Full page Try a cheap bounded walk before the full occur checksaa25c9e · JonoPrest · Nov 03, 2026, 09:27 UTC
Both checks now first walk the type with no visited set and no marks, giving up after 256 nodes (which also stops cycles and heavy sharing); when the walk gives up, the unchanged full walk runs. - Review
Skip scraping a primitive's type when no specialization reads itf987a7d · JonoPrest · Nov 03, 2026, 09:27 UTC
For a function type whose primitive is in neither table, it now calls Review
Build the occur walk's closure once per call2962050 · JonoPrest · Nov 09, 2026, 13:10 UTC
Measured with scripts/compilerBenchmarks.js against the previous commit: allocated words -1.58% in total, stress/TypeAliasChain.res -53%, Belt and runtime files -2% to -2.7%. On a 552 module React app (clean build, one domain) the build allocates 0.05% fewer words. A version that returns a bool instead of raising allocated more than this one (-1.27% in total), because Walk the occur fast paths without allocating86cf87a · JonoPrest · Nov 09, 2026, 14:33 UTC
Measured with scripts/compilerBenchmarks.js against the previous commit: allocated words -1.03% in total (-1.14% without the project builds), stress/TypeAliasChain.res -8.3%, Belt map and set modules -2.7% to -3.4%. On a 552 module React app (clean build, one domain) the build allocates 0.99% fewer words. Keeping the closures and only skipping the walk for a leaf type (a variable or a constructor without arguments) gave -0.57% in total and -0.54% on the app. Review
|
🎉 Compiler benchmarksAllocated wordsIn total: 683,220,137 → 640,767,501 (-6.21%). Generated projects: 923,206,562 → 920,854,584 (-0.25%). 🎉 Improvement: 152 benchmarks allocate more than 1% less.
236 more changes
InstructionsIn total: 9,666,123,646 → 9,211,172,800 (-4.71%). 🎉 Improvement: 12 benchmarks execute more than 1% fewer instructions.
13 more changes
Output sizeIn total: 25,841,785 → 25,843,198 (+0.01%). None of the 13 benchmarks changed by 0.1% or more. Pull request 86cf87a compared with base dcedd5d in this run. |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-9193 |
JonoPrest
force-pushed
the
jono/occur-check-fast-path
branch
5 times, most recently
from
November 10, 2026 14:44
130e13c to
6b84b88
Compare
Member
|
Thanks! Looks good to me and improves performance noticeably. But as it affects the type checker, maybe @cristianoc could also have a look. |
JonoPrest
force-pushed
the
jono/occur-check-fast-path
branch
3 times, most recently
from
November 10, 2026 17:24
95ab303 to
98c65e7
Compare
cristianoc
approved these changes
Nov 10, 2026
cristianoc
left a comment
cristianoc
left a comment
Collaborator
There was a problem hiding this comment.
Likely there's semantic diff to provide clarity as the PR description conflates the problem, the solution, and the justification in the same text.
So there's a fast approx path for occurs check. If it says OK, it means the occur check would agree.
If it says don't know then the normal occurs check runs.
So typical lucky path optimisation with backtracking.
The rest is speedup refactoring to avoid allocations and similar things.
Why:
Every variable link in unification runs `occur`, and the Tvar/Tconstr
cases also run `deep_occur`. `occur` builds an AVL set of visited
nodes along every path and `deep_occur` marks every node and walks the
type again to unmark it, although almost all of these walks see a
handful of nodes. On a 552 module React app they make 943k and 494k
calls, about 0.5 s of 14 s single domain CPU.
Both checks now first walk the type with no visited set and no marks,
giving up after 256 nodes (which also stops cycles and heavy sharing);
when the walk gives up, the unchanged full walk runs.
- `occur` in expression mode: a walk over the edges `occur_rec` follows
that finishes without meeting the variable proves `occur_rec` would
neither raise nor expand anything. Pattern mode, which looks types up,
always runs the full walk.
- `deep_occur`: the walk only answers "absent". It gives up on meeting
the variable and on a node an enclosing walk already marked, so the
marks left behind are the same as the full walk's.
Review: Can an answer, a diagnostic or an artifact change? No: each fast
walk either proves the full walk would do nothing more than it did, or
gives up and the full walk runs from scratch. The new fixtures pin the
errors, including one past the budget, and the corpus (5,398 artifacts
and 1,147 .cmt/.cmti) is byte identical to master.
Signed-off-by: Jono Prest <jjprest@gmail.com>
Why:
`specialize_primitive` scraped the primitive's whole function type (a
deep copy plus head expansion) on every use, but only unified
operators and comparisons look at it. For the other 95% of uses (on a
552 module React app, 21,582 of 22,770) the result was thrown away
and `find_builtin` answered.
For a function type whose primitive is in neither table, it now calls
`find_builtin` directly.
Review: Could the skipped scrape have had an effect? Scraping a function
type looks nothing up: only a Tconstr head is expanded. What is left
is a copy with fresh type ids, made during Lambda translation after
the .cmi and .cmt are written, whose result nothing read. The corpus
artifacts and .cmt files are byte identical to master.
Signed-off-by: Jono Prest <jjprest@gmail.com>
Why:
`occur_reaches` passed `occur_reaches budget ty0` to `iter_type_expr`
at every node, so the fast walk allocated a partial application per
node it visited.
`occur_reaches` now defines its walk once per call, capturing the budget
and the variable, as `deep_occur_fast_absent` already does. The edges it
follows and the exceptions it raises are unchanged.
Measured with scripts/compilerBenchmarks.js against the previous commit:
allocated words -1.58% in total, stress/TypeAliasChain.res -53%, Belt
and runtime files -2% to -2.7%. On a 552 module React app (clean build,
one domain) the build allocates 0.05% fewer words. A version that
returns a bool instead of raising allocated more than this one (-1.27%
in total), because `List.for_all` takes a closure per list.
Signed-off-by: Jono Prest <jjprest@gmail.com>
Why:
The bounded walks that `occur` and `deep_occur` try first allocated
a ref for their budget and a closure for the walk on every call, and
`iter_type_expr` allocated another closure at each arrow type they
visited. Unification runs `occur` for every variable it links, mostly
on small types, so this allocation was a large share of what the
fast paths cost.
`occur_reaches` and `deep_occur_fast_absent` now walk the type with
mutually recursive functions that take the budget left and return what
remains, following the edges of `iter_type_expr` written out by
constructor. The match on `type_desc` lists every constructor, so a new
one is a compile error here rather than a missed edge. Only
`deep_occur_fast_absent` at a `Tvariant` still goes through
`iter_type_expr` with a closure, for the row's fields. The nodes visited,
the order, the budget and the exceptions raised are unchanged.
Measured with scripts/compilerBenchmarks.js against the previous commit:
allocated words -1.03% in total (-1.14% without the project builds),
stress/TypeAliasChain.res -8.3%, Belt map and set modules -2.7% to
-3.4%. On a 552 module React app (clean build, one domain) the build
allocates 0.99% fewer words. Keeping the closures and only skipping the
walk for a leaf type (a variable or a constructor without arguments)
gave -0.57% in total and -0.54% on the app.
Review: Can a result change? No: a walk that finishes visited exactly
the nodes the closure version visited, within the same budget; one
that gives up still falls back to the full check.
Signed-off-by: Jono Prest <jjprest@gmail.com>
JonoPrest
force-pushed
the
jono/occur-check-fast-path
branch
from
November 10, 2026 18:14
98c65e7 to
86cf87a
Compare
cknitt
approved these changes
Nov 10, 2026
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.
Problem. Every variable link in unification runs
occur, and the Tvar/Tconstr cases also rundeep_occur.occurkeeps a set of visited nodes along every path, anddeep_occurmarks every node and then walks the type again to unmark it, although almost every call sees only a few nodes: on a 552 module React app they make 943k and 494k calls, about 0.5 s of 14 s single domain CPU. Separately,specialize_primitivecopied the primitive's whole function type on every use, though only unified operators and comparisons read it (95% of uses don't).Change.
iter_type_exprare written out with an exhaustive match, so a new type constructor is a compile error rather than a missed edge.specialize_primitivecallsfind_builtindirectly for a function type whose primitive is neither a unified operator nor a comparison.Why it is safe.
occur, expression mode only: a fast walk that finishes without meeting the variable provesoccur_recwould neither raise nor expand anything. Pattern mode looks types up, so it always takes the full walk.deep_occur: the fast walk only answers "absent". It gives up on meeting the variable and on nodes an enclosing walk already marked, so the marks left behind are the same as the full walk's..cmiand.cmtare written.Results.
stress/TypeAliasChain.res, allocated wordsJS,
.cmi,.cmj,.ast,.cmtand.cmtiare byte identical to master across three production codebases and their dependencies, at 1 and the default number of domains. Newsuper_errorsfixtures cover occur check errors through a function type, through a type constructor argument, and past the 256 node budget; their output matches master.make testandmake test-rewatchpass.Checklist
changes/README.md), or no release notes are needed and a maintainer should applychangelog:skip.CONTRIBUTING.md).git commit -s).