Repository navigation
Conversation
JonoPrest
added a commit
that referenced
this pull request
Nov 3, 2026
The entry names #9193. Signed-off-by: Jono Prest <[email protected]>
Contributor
Author
|
@codex review |
Changelog💅 Polish
Semantic Diff3 commits · Full page Try a cheap bounded walk before the full occur checks393d6cb · 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 it58959d9 · 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 calldfc6baa · JonoPrest · Nov 09, 2026, 13:10 UTC · edited
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 |
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.92% 81.93%
=======================================
Files 456 456
Lines 65469 65501 +32
=======================================
+ Hits 53635 53665 +30
- Misses 11834 11836 +2
🚀 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: 685,970,852 → 650,036,516 (-5.24%). 🎉 Improvement: 112 benchmarks allocate more than 1% less.
208 more changes
InstructionsIn total: 9,675,830,989 → 9,259,635,021 (-4.30%). 🎉 Improvement: 12 benchmarks execute more than 1% fewer instructions.
13 more changes
Output sizeIn total: 28,859,904 → 28,860,480 (±0.00%). None of the 13 benchmarks changed by 0.1% or more. Pull request dfc6baa compared with base e941d27 in this run. |
JonoPrest
force-pushed
the
jono/occur-check-fast-path
branch
2 times, most recently
from
November 9, 2026 12:42
699552f to
879a21d
Compare
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 <[email protected]>
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 <[email protected]>
JonoPrest
force-pushed
the
jono/occur-check-fast-path
branch
from
November 9, 2026 13:05
879a21d to
58959d9
Compare
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 <[email protected]>
JonoPrest
force-pushed
the
jono/occur-check-fast-path
branch
from
November 9, 2026 13:11
75351f6 to
dfc6baa
Compare
This branch has not been deployed
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.
Two small type checker hot spots that do work nobody uses.
Occur checks. 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, but almost all of these walks see only a few nodes. On a 552 module React app that is 943k and 494k calls, about 0.5 s of 14 s single domain CPU.Both now first walk the type without a visited set or marks, giving up after 256 nodes (which also stops cycles and heavy sharing). When it gives up, the existing walk runs unchanged.
occur, expression mode only: if the cheap walk finishes without meeting the variable,occur_recwould not have raised or expanded anything either. Pattern mode looks types up, so it always takes the full walk.deep_occur: the cheap walk only answers "absent". It gives up on meeting the variable and on nodes an enclosing walk already marked, so marks end up the same.Primitive specialization.
specialize_primitivescraped the primitive's function type (a copy of the whole type) on every use, but only unified operators and comparisons read it. For the other 95% of uses it now callsfind_builtindirectly. Scraping a function type looks nothing up, so the only work skipped is that copy, made during translation after.cmiand.cmtare written.Clean builds, 8 vCPU Linux VM, 1 domain, this PR / master, 5 runs:
Testing:
super_errorsfixtures for occur check errors: through a function type, through a type constructor argument, and past the 256 node budget. Their output matches master..cmi,.cmj,.ast,.cmtand.cmtiare byte identical to master across three production codebases and their dependencies, at 1 and the default number of domains.make testandmake test-rewatchpass.