Skip identifier walk through empty FunctionEnvironments#2419
Merged
Conversation
Closures with zero parameters and zero local declarations (a tight
inner method like sw.Start() in the stopwatch benchmark) end up with a
FunctionEnvironment whose _slots and _dictionary are both null — their
own bindings are nothing. Identifier lookups inside the body always
resolve outward through the closure chain, but the env-walk still pays
a virtual TryGetBinding/HasBinding per layer.
Skip those empty FunctionEnvironments outright in
TryGetIdentifierEnvironmentWithBinding{,Value}. Only FunctionEnvironment
is targeted (ModuleEnvironment may carry indirect-import bindings even
when slots/dict are null; DeclarativeEnvironment block scopes effectively
always have slots when they exist).
Hot-path benefit is small in isolation — Change 3's slot cache already
short-circuits this on cache hits — but it speeds up first-call resolve
and cache-miss fallback paths.
stopwatch benchmark: noise dominated this run (StdDev 49-76ms from
concurrent machine load). Allocations unchanged from Change 4, confirming
the change is correctness-preserving and the apparent regression is
contention, not a real cost.
98567 test262 + 2861 Jint.Tests pass; 0 regressions.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
Closures with zero parameters and zero local declarations — a tight inner method like
sw.Start()in the stopwatch benchmark — end up with aFunctionEnvironmentwhose_slotsand_dictionaryare both null. Their own bindings are nothing. Identifier reads inside the body always resolve outward through the closure chain, but the env-walk inJintEnvironment.TryGetIdentifierEnvironmentWithBinding{,Value}still pays a virtualTryGetBinding/HasBindingper layer.This PR skips those empty
FunctionEnvironmentlayers outright in the chain walk:Only
FunctionEnvironmentis targeted —ModuleEnvironmentmay carry indirect-import bindings even when slots/dict are null, andDeclarativeEnvironmentblock scopes effectively always have slots when they exist.The hot-path benefit is small in isolation — the slot-binding cache from #2416 already short-circuits this on cache hits — but it speeds up first-call resolve and cache-miss fallback paths.
Benchmark (stopwatch.js)
Ryzen 9 5950X / .NET 10.0.7. Baseline + PR taken back-to-back. Run-to-run StdDev was high on this session (4-17ms), so individual numbers should be read with caution; the directional pattern is consistent though.
Most of the gain is on the parse-each-iteration
Execute (var)path where the slot cache from #2416 starts cold each iteration; here the empty-env skip removes a virtual call per identifier read in the warmup phase. Allocations are unchanged — this is purely a control-flow tweak.Validation
Jint.Tests(net10.0): 2861 passed, 0 failed.Jint.Tests.Test262(net10.0): 98567 passed, 506 skipped, 0 failed (clean run; transient flakes under concurrent test load are unrelated).The skip is gated on
is FunctionEnvironmentplus_slots is null && _dictionary is null, so any env that could ever resolve a binding is preserved.record._outerEnv is not nullensures we still hit the global lookup at the chain end.Stacked work
Fifth of six related performance changes. The
perf/drop-interlockedbranch (originally #2418) was abandoned after CI revealed that pool fields onJintFunctionDefinition.Stateare genuinely shared across parallel test fixtures via the static Test262 harness script cache, so the atomic exchanges are required.Last in the queue is
perf/block-env-resetonlahma/jint— small block-env reset inlining. Will be PR'd after this one lands.🤖 Generated with Claude Code