perf: collapse 10 active-override Refs into single Ref[FunctionRealmProtos?] (#369)#369
Merged
Conversation
Replace the 10 double-indirect Ref reads in Part 1 of realm_fast_path_allowed with a single Bool field (has_active_override) maintained by apply_active_realm_protos. The Bool is set to true when any active override is Some, false when all are None. Ablation established that the 10 reads + HashMap lookup consumed 14-22% of per-call time (PR #367/#368 session). This change eliminates Part 1 (10 x 2 pointer dereferences) at the cost of 1 direct field read. Measured gain on JS target (5-run median): call_frame 6.66 ms → 6.25 ms (−6.2%, CV 4.7% → 0.9%) method_call 7.66 ms → 7.01 ms (−8.5%) runtime_helpers 9.49 ms → 8.79 ms (−7.4%) local_access (control): noise only All 7 raw direct-write sites in wbtest files updated to maintain the invariant (has_active_override == OR of all 10 active_*_override Refs). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…_realm_wbtest
Three direct writes to active_{map,set,promise}_prototype_override.val
in has_property_realm_wbtest.mbt were missing the companion
has_active_override = true update. All 10 direct raw write sites in
the source tree are now audited and consistent with the invariant:
has_active_override == (any active_*_prototype_override.val is Some).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a Changeshas_active_override consolidation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b14ccac369
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Contributor
Benchmark ResultsRun: https://github.com/dowdiness/js_engine/actions/runs/27628615012
Stage summary
Focused bytecode base-vs-head comparison
Base-vs-head comparison
Mean-time chart (log scale)
Closure-conversion comparison
|
PR #369 introduced a stale-cache hazard: the 10 active_*_prototype_override Ref fields remained publicly writable (RealmState is pub(all)), so external code writing any Ref directly would bypass has_active_override and allow call_value to take the fast path incorrectly. Fix: collapse the 10 individual Refs + Bool into a single Ref[FunctionRealmProtos?] on RealmState. - None = no cross-realm context active - Some(protos) = at least one override set The "any active?" check in realm_fast_path_allowed Part 1 is now `active_overrides.val is None` — one Ref read. This is structurally correct regardless of any external write, because writing to the single combined Ref atomically updates both the proto data AND the "any active?" check. No separate Bool cache exists to become stale. Public API changes: - Remove: 10 active_*_prototype_override fields, has_active_override Bool - Add: active_overrides : Ref[FunctionRealmProtos?] - Promote to pub: apply_active_realm_protos, FunctionRealmProtos struct + constructor All wbtest direct-write sites updated to use apply_active_realm_protos. apply_active_realm_protos is now simpler: 10 Ref writes → 1 Option write. active_realm_protos is now simpler: 10 .val reads → unwrap one Option. 2096/2096 tests green. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dowdiness
changed the title
2 tasks
dowdiness
added a commit
that referenced
this pull request
Jun 16, 2026
…overrides (#370) Covers the clearing path: set an override, call apply_active_realm_protos with FunctionRealmProtos() (all None), confirm fast path is re-allowed and proto getter falls back to the base proto. Follow-up to #369 — identified as a gap during review. Co-authored-by: Claude Sonnet 4.6 <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
1 participant
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
Replaces Part 1 of
realm_fast_path_allowed— originally 10 double-indirectRef[Value?]reads — with a singleRef[FunctionRealmProtos?]onRealmState.The initial approach (Bool cache + 10 individual Refs) had a correctness flaw:
RealmStateispub(all), so any external code writingactive_*_prototype_override.valdirectly would bypass the Bool without updating it, causingcall_valueto take the fast path erroneously when a cross-realm override was active.New design: collapses all 10 override Refs and the Bool into one field:
realm_state.active_overrides.val is None— one Ref readapply_active_realm_protos: 10 Ref writes → 1 Option writeactive_realm_protos: 10.valreads → unwrap one OptionPublic API changes (interpreter/runtime):
active_*_prototype_override : Ref[Value?]fields,has_active_override : Boolactive_overrides : Ref[FunctionRealmProtos?]pub:apply_active_realm_protos,FunctionRealmProtosstruct + constructorAll wbtest direct-write sites updated to use the now-public
apply_active_realm_protos.Motivation
An ablation (
return trueat top ofrealm_fast_path_allowed) showed 17–22% total savings from skipping both Part 1 and Part 2. Part 1 alone accounts for ~6–9% of per-call cost. Measured gain on JS target (5-run median, post-#368 baseline):call_framemethod_callruntime_helpersCV on
call_framedropped 4.7% → 0.9%, confirming the Ref reads were adding timing variance.Test plan
moon check --deny-warncleanmoon test: 2096/2096 passedmoon info && moon fmtclean🤖 Generated with Claude Code