Add resolve_and_get_from_scope (fused resolve_scope + get_from_scope) - #516
Add resolve_and_get_from_scope (fused resolve_scope + get_from_scope)#516Jarred-Sumner wants to merge 2 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // prediction, we'd otherwise think that it has to exit. Then when it did execute, we | ||
| // would recompile. But if we can fold it here, we avoid the exit. | ||
| if (JSValue value = m_graph.tryGetConstantClosureVar(scopeNode, ScopeOffset(inputs.operand))) | ||
| return weakJSConstant(value); | ||
|
|
||
| SpeculatedType prediction = SpecNone; | ||
| if (resolveType == ResolvedClosureVar) { | ||
| // ResolvedClosureVar is not used normally. It is very special internal ResolveType, mainly used for generators and private fields. | ||
| // In these variables, it can happen that we use JSEmpty as a result of op_get_from_scope (which becomes a TDZ error in normal ClosureVar). | ||
| // And this JSEmpty is still legit. The problem is that ValueProfile never tells about JSEmpty since it sees no value is stored when JSEmpty | ||
| // is stored. We workaround this very special internal use case by explicitly setting SpecEmpty when ValueProfile tells this is SpecNone. | ||
| prediction = getPredictionWithoutOSRExit(); | ||
| if (prediction == SpecNone) |
There was a problem hiding this comment.
🔴 The refactored parseGetFromScope() checks resolveType == ResolvedClosureVar where resolveType is derived from inputs.getPutInfo (populated from metadata.m_getPutInfo), but CodeBlock::finishCreation rewrites ResolvedClosureVar → ClosureVar in that metadata field for op_get_from_scope (CodeBlock.cpp:613-615), so this branch is now dead. The pre-refactor code deliberately checked bytecode.m_getPutInfo.resolveType() — the immutable bytecode operand, which stays ResolvedClosureVar — so generator/private-field reads that legitimately load JSEmpty now fall through to getPrediction() and insert ForceOSRExit. Fix: thread the bytecode's original resolve type (or an isResolvedClosureVar flag) through GetFromScopeInputs.
Extended reasoning...
What the bug is
The op_get_from_scope DFG lowering was extracted into parseGetFromScope() so it can be shared with the new fused opcode. Inside the ClosureVar case, the SpecEmpty workaround now reads:
ResolveType resolveType = getPutInfo.resolveType(); // getPutInfo = inputs.getPutInfo
...
if (resolveType == ResolvedClosureVar) {
prediction = getPredictionWithoutOSRExit();
if (prediction == SpecNone)
prediction = SpecEmpty;
} else
prediction = getPrediction();At the op_get_from_scope caller, inputs.getPutInfo is populated from metadata.m_getPutInfo. But CodeBlock::finishCreation rewrites the metadata for op_get_from_scope at link time (CodeBlock.cpp:613-615):
if (bytecode.m_getPutInfo.resolveType() == ResolvedClosureVar) {
metadata.m_getPutInfo = GetPutInfo(bytecode.m_getPutInfo.resolveMode(), ClosureVar, ...);
break;
}So by the time DFG parsing runs, metadata.m_getPutInfo.resolveType() is ClosureVar, never ResolvedClosureVar, and the SpecEmpty branch is unreachable.
What the pre-refactor code did
The removed code was subtle about which field it read: the outer switch used resolveType = metadata.m_getPutInfo.resolveType() (so it landed in the ClosureVar case), but the inner check read bytecode.m_getPutInfo.resolveType() — the immutable instruction operand, which is not rewritten and still says ResolvedClosureVar. The refactor collapsed both to the metadata-derived value, losing that distinction.
Step-by-step proof
- A generator's frame-environment slot (or a private-field brand slot) is emitted as
op_get_from_scopewithbytecode.m_getPutInfo.resolveType() == ResolvedClosureVar. CodeBlock::finishCreationsetsmetadata.m_getPutInfoto ClosureVar for that instruction.- The slot legitimately holds JSEmpty (e.g., before first write). LLInt/baseline execute it; the ValueProfile records nothing because JSEmpty encodes as 0 ("no value stored"), so the profile stays SpecNone.
- DFG compiles:
op_get_from_scopehandler setsinputs.getPutInfo = metadata.m_getPutInfo→ resolveType is ClosureVar. parseGetFromScope()enters the ClosureVar case;resolveType == ResolvedClosureVaris false; falls togetPrediction().getPrediction()sees SpecNone and insertsForceOSRExit— exactly what the comment says the workaround was added to avoid.
Before this PR, step 5 would have taken the ResolvedClosureVar branch, set prediction = SpecEmpty, and emitted a normal GetClosureVar with no forced exit.
Why nothing else prevents it
The fused opcode is unaffected (op_resolve_and_get_from_scope asserts resolveType != ResolvedClosureVar and canFuseResolveAndGet excludes scoped-offset variables), so the branch's only live consumer was the standalone op_get_from_scope — which is precisely the caller that now passes the rewritten value.
Impact
This is a performance regression, not a correctness bug: DFG-compiled generators and private-field reads whose profiled value is empty will hit ForceOSRExit and recompile, undoing a deliberate fix. The workaround comment is still in the code, describing behavior that no longer happens.
Fix
Add a field to GetFromScopeInputs carrying the bytecode's original resolve type (or a bool isResolvedClosureVar), populate it from bytecode.m_getPutInfo.resolveType() at the op_get_from_scope caller, and check that field instead of the metadata-derived resolveType in the SpecEmpty branch.
Preview Builds
|
94da99b to
04f1216
Compare
|
Warning Review limit reached
On-demand reviews are free for the next 26 days. After that, they cost $0.25 per reviewed file. Or wait 58 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 79 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (24)
Comment |
| let globalLet = 2; | ||
| globalThis.globalProp = 3; | ||
| function readGlobals() { return globalVar + globalLet + globalProp; } | ||
| for (let i = 0; i < 1e4; ++i) |
There was a problem hiding this comment.
🟡 Nit: this new stress test hardcodes 1e4 in 11 loop bounds (lines 25, 36, 43, 64, 72, 81, 91, 104, 117, 127, 135); JSTests/README.md rule 2 requires new tests to use testLoopCount so the harness can scale iterations per configuration. Replace 1e4 with testLoopCount.
Extended reasoning...
What the issue is
JSTests/stress/resolve-and-get-from-scope.js is a newly-added test file, and every one of its tier-up loops uses a hardcoded 1e4 bound:
for (let i = 0; i < 1e4; ++i)
shouldBe(readGlobals(), 6);This appears at 11 sites: lines 25, 36, 43, 64, 72, 81, 91, 104, 117, 127, and 135.
Why this violates a repository requirement
JSTests/README.md:20 (pulled into the directory-scoped instructions via JSTests/CLAUDE.md) states, under "New tests are required to adhere to the following rules":
- Use
testLoopCountorwasmTestLoopCountto control how many iterations a test runs. ThejscCLI sets these based on the configuration of the test, so tests iterate enough to tier up where that matters and exit early where it doesn't.
This is not a stylistic suggestion — it is listed as a required rule for newly-added test files. The convention is widely followed: ~2200+ files under JSTests/ reference testLoopCount.
Why nothing else prevents it
The test happens to work with 1e4 because that is roughly the default tier-up threshold, but the point of testLoopCount is that the jsc shell sets it per configuration: no-JIT / cloop configurations set it low so the test exits quickly instead of wasting 10 000 iterations that will never tier up, while eager-tier configurations may set it higher/lower as needed. A hardcoded 1e4 defeats that scaling in every configuration this file is run under.
Step-by-step
run-jsc-stress-tests JSTests/stressrunsresolve-and-get-from-scope.jsunder many configurations (e.g..no-llint,.no-cjit,.ftl-eager-no-cjit, cloop, etc.).- In each configuration, the shell sets the global
testLoopCountto the appropriate iteration count for that configuration. - This test ignores that global and always runs each loop
1e4times. - In configurations where JIT is disabled, all 11 loops × 10 000 iterations run in the interpreter for no benefit; in configurations with lowered thresholds,
1e4may be far more than needed to reach FTL. The test still passes — this is purely a harness-integration/convention violation, not a correctness bug.
Impact
No runtime correctness impact — the test produces the same pass/fail result either way. The impact is on test-suite hygiene: it violates a documented repository requirement for new tests and won't scale iteration count with the harness.
Fix
Replace each 1e4 with testLoopCount:
for (let i = 0; i < testLoopCount; ++i)
shouldBe(readGlobals(), 6);(applied to all 11 loop bounds listed above).
… instruction A non-local read that resolves statically (no enclosing with scope) no longer spends a resolve_scope and a temporary register on the resolved scope. The generator emits the fused op for ResolveNode, call callees, typeof, and tagged templates; a bare call through it passes undefined as this, which is what to_this made of the scope. LLInt: one fast path that resolves into a register and reads from it; either half that cannot be served inline goes to one slow path that redoes both. Baseline: the resolve_scope and get_from_scope emitters and thunks are templated on the opcode and run back to back (the fused metadata lays the two ops' metadata out in order); inline check failures redo the whole instruction in C++. DFG: the two lowerings are shared with the unfused ops and chained; a dynamic resolve feeds a GetDynamicVar, whose KnownCellUse edge means no exit can land between the halves. LOL treats it as a slow op.
… add a stress test - DFG: a dynamic resolve is one GetDynamicVar that resolves first (GetPutInfo::resolvesScopeFirstBit), instead of a ResolveScope node (which clobbers exit state) followed by a node that may exit; after a ForceOSRExit in the resolve half the get half is not parsed. - Baseline operations take the frame from the JIT operation itself; the shared helpers receive it. - Do not fuse in code lexically inside a with statement (TaintedByWithScopeLexicallyScopedFeature): its static-looking scope chain resolves through the with object at runtime, which must be this for a call through it. - Fuzzer agents know the fused op carries a value profile.
04f1216 to
87cc6cc
Compare
1 participant
Stacked on #515.
In bundled ESM, every read of a module-level binding from inside a function is
resolve_scope+get_from_scope: 43% of all instruction bytes in Claude Code's bytecode, and 97–99% ofresolve_scopeinstances are immediately consumed by aget_from_scopeon the resolved register. This addsresolve_and_get_from_scopeand emits it forResolveNode, call callees (f()),typeof x, and tagged templates when the resolve is static: nowithscope in the function's own chain, and nowithlexically enclosing the function (TaintedByWithScopeLexicallyScopedFeature). The fused instruction is the same size asget_from_scope(it already carries the resolve type ingetPutInfoandlocalScopeDepth), and the temporary register for the resolved scope goes away.Prior art: bugs.webkit.org #202675 (Yusuke) and #237836 / WebKit PR WebKit#1579 (Robin Morisset, Yijia Huang; implemented with checkpoints, closed unmerged in 2022).
Tiers:
slow_path_resolve_and_get_from_scope) that redoes both.resolve_scope/get_from_scopeemitters and thunks are templated on the opcode and run back to back. The fused metadata lays the two ops' metadata out in order, so the thunks read the same offsets. Inline check failures redo the whole instruction in C++; thunk slow paths stay per-half (operationResolveScopeHalfForBaseline,operationGetFromScopeHalf).parseResolveScope,parseGetFromScope) are shared with the unfused ops and chained, so the graph is identical to before for static resolves. A dynamic resolve (unresolved property after an exit, var injection) is oneGetDynamicVarthat resolves through the base scope first (GetPutInfo::resolvesScopeFirstBit), so no node that clobbers exit state is followed by one that may exit within the bytecode. After aForceOSRExitin the resolve half the get half is not parsed.A bare call through a fused resolve passes
undefinedasthisinstead of the resolved scope;JSValue::toThismaps both to the same value (undefined in strict, the global proxy in sloppy), and upstream later made the same change (ee167b). Code inside awithkeeps the two instructions and the scope asthis.Size on the 20 largest CC chunks (14.9 MB of minified JS): 39.9 MB → 37.9 MB of bytecode on top of #515 (46.7 MB on main). Claude Code binary with #515 + #516: 220.4 MB → 203.1 MB; startup and footprint unchanged.
Testing:
JSTests/stress/resolve-and-get-from-scope.js(global var/lexical/property, late-defined and deleted globals,typeofundeclared, TDZ, closure chains,thisfor sloppy/strict callees and tagged templates,with, a function nested insidewith, sloppy direct eval var injection) passes in the jsc shell under default,--useJIT=false,--useDFGJIT=false,--jitPolicyScale=0 --useConcurrentJIT=false, and--useLLInt=false. Bun--bytecoderound-trips over a JSTests sample. Full JSTests/stress differential (2,535 standalone files, release Bun with this WebKit vs release Bun on main, exit code + stdout compared) on linux-x64: default 2535/2535 identical, eager tier-up (jitPolicyScale=0) 2535/2535, Baseline-only 2534/2535, LLInt-only 2532/2535. The four non-identical cases are deep-recursion / termination tests that sit at the 90 s timeout under a 24-way parallel run and flipped between SIGKILL and stack-overflow SIGSEGV in both directions; rerun alone, both builds give the same exit code and time on each of them.