Save and restore generator locals in bulk with dedicated bytecodes - #525
Open
sosukesuzuki wants to merge 1 commit into
Open
Save and restore generator locals in bulk with dedicated bytecodes#525sosukesuzuki wants to merge 1 commit into
sosukesuzuki wants to merge 1 commit into
Conversation
sosukesuzuki
force-pushed
the
claude/generator-bulk-save-restore-locals
branch
from
August 27, 2026 16:10
32f962c to
998a225
Compare
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 1 minute for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (17)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (17)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. WalkthroughChangesThe change adds bulk and per-local generator-local save/restore bytecodes, integrates them across JavaScriptCore execution and optimization tiers, and adds stress coverage for synchronous, asynchronous, delegated, TDZ, closure, and tiered generator execution. Generator local persistence
Merge Risk: ⚪ Minimal · up to The change introduces bulk generator-local save and restore bytecodes without any supplied evidence of a concrete correctness or merge-readiness issue. It is merge-ready after normal checks, with generated-artifact confirmation as a routine follow-up. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed rationale, implementation summary, measurements, and test plan. However, it does not follow the required template because it omits the bug title and Bugzilla link, the review status line, and the required changed-file and function list. Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Preview Builds
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
sosukesuzuki
force-pushed
the
claude/generator-bulk-save-restore-locals
branch
from
August 28, 2026 00:02
998a225 to
eea3372
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
robobun
added a commit
to oven-sh/bun
that referenced
this pull request
Aug 28, 2026
The new head renames the option to useGeneratorBulkSaveRestore and drops the option-off eager tier-up run from the stress test. The fixture and the bytecode round trip follow. The bytecode portability snapshot is unchanged.
Generatorification spills every live local with its own op_put_to_scope at every suspension point and reloads each with op_get_from_scope on resume, so generator and async-function bytecode grows as live locals x suspension points. Large async generators compile to bytecode that is mostly this save/restore code, plus Baseline machine code and linked metadata per local per suspension point. Replace the per-local sequences with two bytecodes, op_save_generator_locals and op_restore_generator_locals, which copy all live locals at once. Each op names a per-suspension-point liveness bit vector and the function-wide union of live locals (both stored in the existing UnlinkedCodeBlock rare-data bit vector table, deduplicated by content); a local's frame slot is its rank within the union, so slots stay stable across suspension points and the generator frame is sized by the number of ever-live locals. Locals no longer need per-local identifiers, SymbolTableEntries, watchpoint sets, or metadata. LLInt runs one C++ slow path per op with a single write barrier; the Baseline JIT emits an unrolled load/store sequence (restore also stores into the op's run of consecutive value profiles); the DFG parser expands the ops into the same PutClosureVar / GetClosureVar nodes as before, so optimized code is unchanged, with OSR-exit value feedback routed through lazy operand profiles keyed by the restored local. The new encoding is behind Options::useGeneratorBulkSaveRestore (default true). With the option off, generatorification emits the previous per-local op_put_to_scope / op_get_from_scope sequences. Every tier executes both encodings, so cached bytecode produced under either setting runs regardless of the current value.
robobun
force-pushed
the
claude/generator-bulk-save-restore-locals
branch
from
August 28, 2026 13:54
eea3372 to
c2fd836
Compare
Collaborator
|
Rebased onto main at
Everything else applied cleanly. oven-sh/bun#40667 re-pins to this preview once it is published. |
robobun
added a commit
to oven-sh/bun
that referenced
this pull request
Aug 28, 2026
…locals in bulk Generatorification spilled every live local with its own op_put_to_scope at every suspension point and reloaded each with op_get_from_scope on resume, so generator and async-function bytecode grew as live locals x suspension points. oven-sh/WebKit#525 replaces the per-local sequences with op_save_generator_locals and op_restore_generator_locals, behind Options::useGeneratorBulkSaveRestoreLocals (default on). This pins its preview build. Tests: - test/js/bun/jsc-stress/fixtures/generator-save-restore-locals.js is the stress test from that PR. The harness now runs a fixture once per `//@ run*` directive, as run-jsc-stress-tests does, so it runs under the default options, --useJIT=0, --useDFGJIT=0, eager tier-up, and with the option off (x2). - A `bun build --bytecode` round trip of the fixture: a cache built with the option on or off must load (Cache hit) and run with it on or off. - preload.js gains drainMicrotasks from bun:jsc.
robobun
added a commit
to oven-sh/bun
that referenced
this pull request
Aug 28, 2026
The updated head renames the option to useGeneratorBulkSaveRestore and drops the option-off eager tier-up run from the stress test. The fixture and the bytecode round trip follow.
robobun
added a commit
to oven-sh/bun
that referenced
this pull request
Aug 28, 2026
…estore ops Only the .jsc sizes and hashes move (26 entries), the source hashes do not. Regenerated against autobuild-preview-pr-525-c2fd8368, which is main's WebKit pin (ceb9f90fb774) plus the oven-sh/WebKit#525 commit.
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
2 participants
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
Generatorification currently spills every live local with its own
op_put_to_scopeat every suspension point and reloads each withop_get_from_scopeon resume, so generator / async-function bytecode grows as live locals × suspension points. Large async functions and async generators end up with bytecode that is mostly this save/restore code, plus a proportional amount of Baseline machine code and 48 bytes of linked metadata per local per suspension point.This PR replaces the per-local sequences with two bytecodes,
op_save_generator_localsandop_restore_generator_locals, which copy all live locals at once:UnlinkedCodeBlockrare-data bit vector table, deduplicated by content.SymbolTableEntrys, watchpoint sets, or metadata.PutClosureVar/GetClosureVarnodes as before, so optimized code is unchanged. OSR-exit value feedback is routed through lazy operand profiles keyed by the restored local (Graph::methodOfGettingAValueProfileFor).BytecodeUseDef) treats the save as using the live locals and the restore as defining them.Option
The new encoding is gated by
Options::useGeneratorBulkSaveRestore(defaulttrue). With--useGeneratorBulkSaveRestore=0, generatorification emits the previous per-localop_put_to_scope/op_get_from_scopesequences. Every tier executes both encodings regardless of the option, so bytecode cached under one setting still runs under the other (verified both directions with--diskCachePath+--forceDiskCache=1).Measurements
Same
RelWithDebInfojsc binary, option on vs off (arm64 macOS, best of 5, 2M resumes,N= locals live across each suspension point).Bytecode for an async function with 100 locals and 200
awaits:ns per
yield, LLInt only (--useJIT=0):ns per
yield, Baseline only (--useDFGJIT=0):All tiers enabled: flat within noise for both
yieldandawait(e.g. N=100: 40.6 → 40.2 ns/yield, 54.5 → 55.9 ns/await), as expected since the DFG graph is identical. SincebytecodeCostdrops accordingly, large async functions that previously sat above the DFG size limit can now tier up.Test plan
JSTests/stress/generator-save-restore-locals.js: mixed-type locals resumed withnext/throw, live sets of 3/9/70/300 locals (out-of-line bit vectors, wide operands), TDZ locals across a yield, locals sharing the frame with captured variables, nothing live, async function / async generator /yield*, and a tier-up-then-type-change case. Runs under default,--useJIT=0,--useDFGJIT=0, eager tier-up,--useGeneratorBulkSaveRestore=0, andrunBytecodeCache.--validateGraph=1and--useLOLJIT=1; the 174 existinggenerator*/async*/yield*/await*stress tests under default, eager tier-up,--useJIT=0, and--useGeneratorBulkSaveRestore=0— no failures.--forceDiskCache=1under the other — passes both ways.