Slot fast path for compound assignment to non-number bindings#2566
Merged
Conversation
EvalExecutionBenchmarks isolates the dromaeo-core-eval execution shape (hot/fresh eval, new Function, plain-function reference floor, parse-only bound); TimeProbe (--profile-time) splits dromaeo scripts per test() with Stopwatch, complementing MemoryProbe's allocation split. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Strict eval created a fresh dictionary-backed environment per call, so every identifier access in the eval body paid a dictionary probe and the per-AST-node slot caches (identifier reads, unboxed i++) could never engage - while the identical body under new Function() runs on fixed slots. dromaeo-core-eval spends about half of each op inside one such body. The eval compile-cache entry now precomputes the environment slot layout (deduplicated var + function names initialized to undefined, let/const as TDZ templates) plus the hoisting scope; PerformEval instantiates the strict-eval environment from the templates and EvalDeclarationInstantiation skips the var/lexical passes via a new bindingsPreInitialized flag. Bodies that cannot capture the environment (no closures, nested eval or with) pool it across calls, keeping the shared statement tree per-node caches valid. The layout is built only when it can amortize: a loop in the body, or a source seen twice (promotion). Function bookkeeping in EvalDeclarationInstantiation is now lazily allocated, and ShadowRealm reuses CachedHoistingScope for prepared scripts instead of re-walking the AST per eval. Same-window stash A/B: EvalHotReusedEngine -18.6%, EvalFreshEngine -17.5%, EvalSameSource -24.7% time / -47.9% alloc, EvalDistinctSources flat (miss guard), ShadowRealm_ParsedScript -9.2%, dromaeo-core-eval driver -8.5% source / -5.7% prepared; new Function and plain-function reference rows unchanged. Jint.Tests 3159/3097, PublicInterface 79/79, CommonScripts 28/28, Test262 99260/0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code-review findings on the slot-backed eval work, all reproduced before
fixing:
The identifier read fast path's cached-slot walk hopped over intermediate
declarative environments by reference comparison only, so a node whose
cache pointed at an outer environment read a stale binding when executed
under a chain that shadows the name - reachable through the eval
compilation cache (the same body under a shadowing block or a nested
eval of the same source) and through sloppy direct eval injecting a var
into an enclosing function environment. The walk now probes each hopped
declarative environment with HasBinding (pure; the common hop-0 hit pays
nothing) and falls back to full resolution when an intermediate
environment owns the name.
EvalDeclarationInstantiation validated eval-declared var names with
CanDeclareGlobalFunction where the spec requires CanDeclareGlobalVar
(sec-evaldeclarationinstantiation step 8.a.ii.1), so eval("var undefined;")
and redeclaration of any existing non-configurable global threw a
spurious TypeError; pre-existing, but the lines live in this change.
TryPoolEvalEnvironment parked environments with the completed call's
binding values still in the slots, keeping arbitrary object graphs
reachable from the eval cache for the engine's lifetime; the slots are
now reset to templates (and the dictionary and dispose capability
cleared) at park time, and the rent path no longer needs per-rent
cleanup.
EvalScriptAnalyzer no longer lets loops inside arrow functions mark the
eval body as loop-bearing (arrows still descend for arguments/new.target
detection); those loops run in the arrow's own environment and cannot
amortize the eval body's slot layout.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Compound assignment to a slot-stored non-number binding (the s += "x" loop shape) fell off the fast path on both lanes: the discard lane bailed for any non-number left value, and completion-value positions (eval and script bodies, where expression statements must produce values) always took the materialized lane - an uncached environment walk plus a Reference rent/return per iteration. TryCompoundUnboxed keeps its numeric lane unchanged and completes initialized mutable reference values through a shared TryCompoundSlotValue (slot read, right-hand side exactly once, ComputeCompound, SetMutableBinding write-back unless the rope concat mutated in place - the materialized lane exact skip). EvaluateInternal gets the value-producing twin behind the same gates (no suspendable frame, no operator overloading, logical/nullish forms excluded), which is what reaches eval bodies. Const and TDZ bail before any evaluation so the slow path keeps exact error ordering. SlotLocationCache.ResolveAndPopulate now probes only declarative environments (their sealed HasBinding is pure) and disables without touching object or global environments: test262 set-mutable-binding-idref-compound-assign-with-proxy-env.js caught the new value lane firing an extra has/Symbol.unscopables proxy trap while probing a with environment; the same latent hazard existed in the pre-existing discard lanes and is fixed for all users of the cache. Same-window stash A/B against the slot-backed-eval baseline: EvalHotReusedEngine -12.9% (945 us, stable within 0.2% across runs), EvalFreshEngine -10.0%, NewFunctionHotReusedEngine -18.8%, PlainFunctionLoopReference -17.1%, NewFunctionFreshEngine -12.3%. Numeric guard micros vary +/-4-6% between identical binaries with different rows flagging each run (including untouched plain-assign code) - documented run-to-run variance, stopwatch driver stable within 1%. Ten pinning tests (CompoundAssignmentTests) verified to pass identically before and after the change, including the in-place rope-append interleaved-write case in both discard and completion-value positions. Jint.Tests 3169/3107, PublicInterface 79/79, CommonScripts 28/28, Test262 99260/0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code-review findings on the compound-assignment slot lanes, all reproduced before fixing: SlotLocationCache.TryResolve validated a cached hit by reference-walking to the cached environment without checking whether an intermediate declarative environment shadows the name, so the new write lanes stored compound-assignment results into a stale outer binding (silently corrupting the wrong variable, and swallowing the TypeError for a shadowing const) when the same node ran under a different chain - an eval-cache-shared body under a shadowing block, a nested eval of the same source, or a sloppy direct eval injecting the name into an enclosing function environment. The hit walk now probes each hopped declarative environment with HasBinding, mirroring the identifier read walk fix; the common hop-0 hit pays nothing. The class remarks now state the real invariant instead of the per-function-lifetime claim. TryCompoundSlotValue takes the slot ref through the same null/bounds guard TryGetNumberSlot documents for stale or torn caches instead of indexing unguarded. EvaluateAndDiscard's fallback now goes straight to the materialized path (extracted as EvaluateMaterialized) instead of re-running the fast-lane gates and slot resolution that TryCompoundUnboxed just failed, which also leaves the gate list in exactly one place per lane. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Adversarial multi-agent review of the stack surfaced issues; fixed in 1722d13 (branch rebased on the updated #2565):
Perf note, measured with interleaved A/B/A/B driver runs: the shadow probe costs ~2% on the stopwatch script (closure-captured variables resolve at hop 1, paying one pure Full suites + Test262 99,260/0 re-verified on the fixed stack. 🤖 Generated with Claude Code |
lahma
enabled auto-merge (squash)
July 4, 2026 08:47
This was referenced Jul 6, 2026
This was referenced Jul 13, 2026
This was referenced Jul 20, 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.
Stacked on #2565 (the first two commits; review the last commit only).
Compound assignment to a slot-stored non-number binding (the
s += "x"loop shape) fell off the fast path on both execution lanes: the discard lane'sTryCompoundUnboxedbailed for any non-number left value, and completion-value positions (eval/script bodies, where expression statements must produce values) always took the materialized lane — an uncached environment-chain walk plus a Reference rent/return per iteration.What this does
TryCompoundUnboxedkeeps its numeric lane byte-for-byte and, when the resolved slot holds an initialized mutable reference value, completes through a sharedTryCompoundSlotValue: slot read → right-hand side exactly once →ComputeCompound→ write-back viaSetMutableBindingunless the rope concat mutated the value in place (the materialized lane's exact skip).EvaluateInternalgets the value-producing twin behind the same gates (no suspendable frame, no operator overloading, logical/nullish forms excluded) — this is what reaches eval bodies, whose completion-value semantics keep their statements off the discard lane entirely.SlotLocationCache.ResolveAndPopulatenow probes only declarative environments (their sealedHasBindingis pure) and disables without touching anything else once a non-declarative environment is reached. Test262'sset-mutable-binding-idref-compound-assign-with-proxy-env.jscaught the new value lane firing an extrahas/Symbol.unscopablesproxy trap while probing awithenvironment; the same latent hazard existed in the pre-existing discard lanes (i++/n += 1insidewith) and is fixed for all users of the cache.Measurements (same-window stash A/B against the slot-backed-eval baseline, BenchmarkDotNet default job)
Numeric guards (
FunctionLocalNumberLoopBenchmark,PlainNumericAssignBenchmark): the numeric lane is unchanged; flagged rows varied ±4–5% with different rows flagging in different runs, including untouched plain-assignment code, and a same-binary triple run spans the same range (e.g. AccumulatorWithCallArg 31.5–34.0 ms on one binary) — run-to-run variance, not a code effect. The stopwatch driver, the stable canary, reads ±0.8% across all shapes.Shape note: an earlier iteration marked the shared helper
[MethodImpl(NoInlining)]to protect the numeric lane's code size; that taxed the per-iteration discard call +3..7% on the string loops and was dropped after measurement.Combined effect with the slot-backed-eval PR this stacks on — EngineComparison rows (same-runtime worktree A/B against the common base):
Allocation unchanged on all rows.
Gates
CompoundAssignmentTests, 10 cases) verified to pass identically on the code before and after this change via stash cycles — includings += (s = 'z', 'a')(in-place rope append skips the write-back, so the interleaved write wins) in both discard and completion-value positions.Jint.Tests3169/3107 (net10/net472),PublicInterface79/79,CommonScripts28/28, Test262 99,260 passed / 0 failed, all-TFM build.🤖 Generated with Claude Code