Conversation
…runs in The constructor used to checkSyntax() and throw the parse away, and every runInContext()/runInThisContext() went back through JSC's CodeCache, so a script larger than the cache's working set was re-parsed per context and each context got its own copy of the unlinked function tree. The Script now parses once through the CodeCache and keeps the UnlinkedProgramCodeBlock; its SourceProvider hands that block back to the CodeCache on every miss (oven-sh/WebKit#418). Accepted cachedData is pinned the same way instead of being decoded and then ignored.
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: closed, superseded by #38040 (with oven-sh/WebKit#420). The constructor compile-once part continues in #37998. Reproduced with |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it depends on an unmerged WebKit change (WEBKIT_VERSION points at a preview build that the description says should be replaced when oven-sh/WebKit#418 lands) and adds GC-lifecycle-sensitive machinery — a ref-counted SourceProvider holding a raw back-pointer into a GC cell, cleared in the destructor, plus a new WriteBarrier field — a human look is still warranted before landing.
What was reviewed
- GC rooting:
m_pinnedUnlinkedCodeis aWriteBarriervisited invisitChildrenImpl; the provider'sm_scriptback-pointer is nulled in~NodeVMScriptso a provider outliving the Script (via executables/stack traces) can't reach a freed cell. - Constructor flow reordering:
checkSyntax→compile()preserves the SyntaxError/decorateParseErrorStack path;filename/filenameProvided/lineOffsetare copied out beforeWTF::move(options). cachedData+produceCachedDatarestructuring: the newcachedData.isEmpty() && produceCachedDatagate matches the oldelse ifsemantics.- Tests spawn subprocesses with
bunEnv, drain stdout/stderr concurrently, and theBUN_JSC_useCodeCache=0case would fail on the unfixed build per the description.
Extended reasoning...
Overview
This PR makes vm.Script parse its source once and reuse the resulting UnlinkedProgramCodeBlock across every context it runs in. It touches four files: scripts/build/deps/webkit.ts (points WEBKIT_VERSION at a preview build of oven-sh/WebKit#418), src/jsc/bindings/NodeVMScript.{h,cpp} (new NodeVMScriptSourceProvider subclassing StringSourceProvider with the new pinnedUnlinkedCode/pinUnlinkedCode virtual hooks; NodeVMScript gains a WriteBarrier<UnlinkedProgramCodeBlock> + key-hash field, a destructor that nulls the provider's back-pointer, and a compile() method replacing checkSyntax), and test/js/node/vm/vm.test.ts (four new tests under a describe.concurrent block).
Security risks
None identified. This is a compilation-caching optimization inside node:vm; it does not change what code runs or in which realm, and the run paths (JSC::evaluate → per-context ProgramExecutable) are unchanged. The pin is keyed by SourceCodeKey::hash() so a lookup under different parse flags misses.
Level of scrutiny
High. Three factors put this outside auto-approval territory: (1) it is a coordinated cross-repo change — the WebKit side (oven-sh/WebKit#418) is not yet merged and WEBKIT_VERSION is set to a preview tag that the PR description says should be replaced with the real bump; a human needs to sequence the two landings. (2) It adds GC-lifecycle-sensitive state: a ref-counted SourceProvider holds a raw NodeVMScript* back-pointer to a GC cell, relying on the cell's destructor to null it before the cell's storage is reused; and a new WriteBarrier field must be visited. Both look correct (the field is visited, the destructor nulls the pointer, NodeVMScript is already JSDestructibleObject with a destroy hook), but this class of change is exactly what the repo's review guidelines flag for careful human review. (3) The new virtual-method contract with JSC's CodeCache is defined in the paired WebKit PR, so correctness depends on code not visible in this diff.
Other factors
The bug-hunting pass found nothing. The constructor reordering was checked for use-after-move (filename, filenameProvided, lineOffset are copied to locals before WTF::move(options)); the produceCachedData branch was checked against the old else if semantics (now cachedData.isEmpty() && produceCachedData, equivalent because the old branches were mutually exclusive on cachedData.isEmpty()). Tests follow harness conventions (subprocess spawn, {...bunEnv, ...}, concurrent pipe drain, describe.concurrent) and per the description the BUN_JSC_useCodeCache=0 case fails on the unfixed build (5 blocks added) and passes here (0). Given the cross-repo dependency and GC-sensitive surface, deferring to a human is the right call.
|
Updated 7:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit 99d435d has some failures in 🧪 To try this PR locally: bunx bun-pr 37950That installs a local version of the PR into your bun-37950 --bun |
|
On hold. A JSC maintainer is replacing the SourceProvider hook in oven-sh/WebKit#418 with a first-class entry point for executing an already-parsed UnlinkedProgramCodeBlock (and the eval/function equivalents) against a given global; this PR will be rebased onto that API once it is posted (the Script will keep the block from its single parse and call the new entry point per run instead of going through the CodeCache). Not iterating further against the #418 preview in the meantime. The part that does not depend on any JSC change, compiling once in the constructor instead of checkSyntax() plus a second parse on the first run (or on produceCachedData), is being split out into its own PR; link to follow here. |
|
The constructor compile-once part is #37998. |
|
Superseded by #38040 together with oven-sh/WebKit#420, which is the first-class JSC entry point this PR was on hold for: the Script keeps the The constructor-only part, which needs no WebKit change, continues in #37998 until #38040 lands. Closing this one. |
…text it runs in (#38040) ### What does this PR do? `new vm.Script(src)` compiles `src` once, and every `runInContext` / `runInThisContext` / `runInNewContext` links that one `UnlinkedProgramCodeBlock` into the target context, instead of going back to JSC's `CodeCache` for each run. The cache keeps 16 MB of source in total, so for anything large (or once enough other scripts have gone through) every context used to re-parse and regenerate the whole script and get its own copy of the unlinked cells; jest-style workloads (one big script, one context per test file) hit exactly this. - Uses the entry point added in oven-sh/WebKit#420 (`JSC::evaluate(globalObject, source, block, …)`), which is included in the WebKit build main already pins; this PR no longer changes `WEBKIT_VERSION`. - The `Script` cell holds the block in a `WriteBarrier` and passes it to `evaluate()`. It is recompiled (and replaced) only if a context whose `CodeGenerationMode` differs — i.e. one with a debugger attached — runs it; JSC also ignores a block that does not match and falls back to the cache, so a mismatch can never run the wrong bytecode. Global declaration instantiation still happens per context inside JSC exactly as before. - The constructor's `checkSyntax` (a parse whose result was thrown away) is replaced by that compile, so construct + run parses once instead of twice; construction still throws the SyntaxError with the same decoration. `sourceMapURL` no longer needs its own re-parse either, since the constructor's compile populates the directive. - `decorateParseErrorStack` no longer fabricates a `<url>:-1` header for compile errors that have no position (parser stack overflow already did this before; codegen out-of-memory now surfaces at construction too). Those errors now look like Node's: a plain `RangeError`. - `cachedData` keeps its current meaning (accept / reject check; execution uses the compiled source), and `produceCachedData` / `createCachedData()` are unchanged — they now find the constructor's compile in the CodeCache instead of parsing again. Executing the decoded `cachedData` block directly, and serializing the held block for `createCachedData()`, are possible follow-ups; the former needs the Script to own the `cachedData` bytes for as long as functions decoded from them can run, so it is deliberately not part of this change. This replaces #37950 (+ oven-sh/WebKit#418), which hooked the `SourceProvider` into the CodeCache miss path: there a Script whose source is already cached under another provider never gets to pin anything, and pinning the block decoded from `cachedData` would have run bytecode that borrows the `cachedData` buffer. Here the Script simply owns what it compiled and hands it to JSC. Trade-off: the unlinked program block (and the `UnlinkedFunctionExecutable` shells under it) now live as long as the `Script` object, rather than as long as the cache happens to keep them; function bytecode inside is still dropped by `deleteAllUnlinkedCodeBlocks` / memory pressure as before, and dead Scripts are unaffected (`script-leak.test.ts` ends with the same 1942 live blocks before and after). Scripts that are constructed but never run now pay bytecode generation at construction (36 → 38 ms for the 2.7 MB script below), which is also what Node does. Numbers — local release builds on the same machine (`build:release:local`; "before" is main as of Aug 5 with unmodified WebKit), 2.7 MB script with 30k functions run in 3 fresh contexts, parse counts from `BUN_JSC_reportParseTimes=1` (a constant 3–4 unrelated small parses per phase, identical on both sides, are subtracted). `BUN_JSC_useCodeCache=0` stands in for the block having been evicted: | phase | before | after | | --- | --- | --- | | `new Script` (30k top-level functions) | 36 ms, 1 parse (checkSyntax, discarded) | 38 ms, 1 parse + bytecode, kept | | each further context, top-level functions, cache evicted | 31–44 ms, 1 full parse, +30k `UnlinkedFunctionExecutable` per context (90k after 3) | 6–7 ms, 0 parses, stays at 30k | | each further context, functions inside a wrapper, cache evicted | 38–59 ms, program + wrapper body re-parsed per context | 1–2.5 ms, 0 parses | | construct + first context, cache warm (wrapper case) | 33 + 40 ms | 39 + 25 ms | | 2000 × `new Script(90 KB) + runInThisContext()` | 32–43 ms (two parses each) | 18–19 ms (one parse each) | ### How did you verify your code works? - New tests in `test/js/node/vm/vm.test.ts`: with `BUN_JSC_useCodeCache=0`, running one Script / two Scripts with the same source / a Script constructed with accepted `cachedData` in 6 contexts adds no `UnlinkedProgramCodeBlock` (the current build adds 5 / 10 / 5); each context still gets its own `var` / function bindings; source positions are identical in every context; a position-less compile error gets no header. - `test/js/node/vm/` and Node's `test/parallel/test-vm-*` + `sequential/test-vm-*` (100 files) against a debug build with the WebKit change, plus `BUN_JSC_validateExceptionChecks=1` on `vm.test.ts`. A probe of the whole `vm.Script` surface (SyntaxError decoration, `sourceMapURL`, positions with `lineOffset` through all four run methods, thrown-error decoration in a second context, `cachedData` accept / reject, `produceCachedData`, `createCachedData`, `timeout`) prints byte-identical output on main and on this branch. - `script-leak.test.ts` passes on release; on a debug build with a locally built WebKit it hits the 5 s timeout, but so does unrelated `eval` in that configuration (18× slower than the prebuilt debug WebKit), so that is the local build configuration, not this change. Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
2 participants
Depends on oven-sh/WebKit#418 (this PR points
WEBKIT_VERSIONat that PR's preview build; it should land together with the real bump once that is merged).Problem
const s = new vm.Script(src)followed bys.runInContext(vm.createContext({}))for N contexts parsessrcmore than once. Both run paths callJSC::evaluate(globalObject, script->source(), ...)(src/jsc/bindings/NodeVMScript.cpp,runInContext/scriptRunInThisContext), which creates a freshProgramExecutableper run and gets its code through JSC'sCodeCache. That works while the cache holds the entry, butCodeCacheMapis sized for a 16 MB working set (workingSetMaxBytes, CodeCache.h) and prunes whenever more than that has been added since the last prune, so a ~20 MB bundle is re-parsed by whichever context comes after any other sizable compilation. Each re-parse also produces a fresh tree ofUnlinkedFunctionExecutables, so memory grows per context instead of being shared.JSC::checkSyntaxto throwSyntaxErroreagerly, threw the result away, and the first run parsed again (the comment in the constructor already named compile-once as the follow-up).cachedDatawas accepted (decoded and linked once to computecachedDataRejected) but the runs never used it; they still went to the CodeCache and parsed.(function(exports, require, module){...})source with 117,978 functions, parses counted withBUN_JSC_reportParseTimes=1:evaluate514-790 ms) plus a re-parse of the wrapper body on invoke, andUnlinkedFunctionExecutablegoes from 118k to 236k live cells (two copies of the bundle).cachedData: construction still parses once (checkSyntax), and the first run parses again.Fix
CodeCache: let a SourceProvider pin the unlinked code for its source WebKit#418 adds
SourceProvider::pinnedUnlinkedCode()/pinUnlinkedCode(): on a CodeCache miss JSC asks the provider before parsing, and hands the provider whatever it parses. Base implementations are no-ops.vm.Scriptnow creates itsSourceCodeover aNodeVMScriptSourceProvider(NodeVMScript.h) that forwards those two hooks to the owningNodeVMScript, which keeps theUnlinkedProgramCodeBlockin aWriteBarrierkeyed bySourceCodeKey::hash()(source plus code type / strictness / code generation mode, so a lookup under different flags misses and gets replaced). The Script is the GC root; the provider only holds a back-pointer, cleared in~NodeVMScriptbecause executables and stack traces from earlier runs can keep the provider alive longer than the Script.The constructor's
checkSyntaxis replaced byNodeVMScript::compile(), which goes throughCodeCache::getUnlinkedProgramCodeBlockand therefore pins the parse it does; syntax errors come out of the sameParserErrorand are decorated exactly as before. AcceptedcachedDatapins the decoded block instead, so such a Script never parses its source. The run paths are unchanged:JSC::evaluatestill creates aProgramExecutableper run (global declaration instantiation has to happen per context), butinitializeGlobalPropertiesnow links the pinned block.Net effect: one parse per Script (zero with accepted cachedData), regardless of how many contexts it runs in, what else goes through the CodeCache, or
useCodeCache; the nested unlinked function executables / code blocks are shared by every context, linked code stays per context as before.Equivalents for the other two entry points (not done here):
ShadowRealm.prototype.evaluate(string)builds a newSourceCodeper call (evalInRealm, ShadowRealmPrototype.cpp) so there is nothing to pin to; the WebKit hook already covers eval code blocks, so a Bun-side handle (for example avm.Scriptmethod that evaluates the pinned source in a given realm viaIndirectEvalExecutable+ the realm'swrapRemoteValue) would give ShadowRealms the same one-parse behavior.vm.compileFunctionreturns a bare function per call and already goes throughgetUnlinkedProgramCodeBlockon a per-callStringSourceProvider(NodeVM.cppconstructAnonymousFunction), so it too would only need a handle object owning a pinning provider. Until then,new vm.Script("(function (exports, require, module) {...})")+runInContextis the way to instantiate one compiled function per context with a single parse.Verified:
test/js/node/vm/vm.test.ts, newdescribe"Script compiles its source once for every context it runs in": a fixture keeps everything 6 context runs produce alive and reports how manyUnlinkedProgramCodeBlocks later runs added. WithBUN_JSC_useCodeCache=0(which makes every run miss the cache, i.e. the big-source behavior in miniature) and with acceptedcachedData, the unfixed build reports 5 and this build 0; the default-cache case and a per-context global declaration check pass on both.Parse counts (
BUN_JSC_reportParseTimes=1, debug builds of the same revision with and without this change, a 5 MB source with ~30k functions run in 3 contexts; the probe is in the details block):new Script(src)runInContext(evaluate)new Script(src, { cachedData })test/js/node/vm/(231 pass) and all 97 nodetest/parallel/test-vm-*files pass against the preview WebKit.script-leak.test.tstimes out on this (slow, ASAN) machine with and without the change; its 10,000-script loop measured directly ends at 34 MB RSS growth with this change versus 138 MB without, because the construction-time compile turns every run into a CodeCache hit and the cache prunes its now-dead entries sooner.Related: node:vm: reject invalid cachedData instead of crashing #32839 rewrites the
cachedDatadecode block this change adds one line to (textual overlap only); test: add a regression test for JSC CodeCache key collisions #35778 is about CodeCache key collisions between different sources, which this per-Script pin does not interact with (a pin only ever answers lookups for its own provider).Background
UnlinkedProgramCodeBlockfor the top level, with anUnlinkedFunctionExecutableper function in it, each of which lazily gets anUnlinkedFunctionCodeBlockthe first time it is called. Running the code in a particular global object "links" those into per-realmCodeBlocks /FunctionExecutables. Only the unlinked half is what a second context can share, and sharing it is exactly what theCodeCachedoes when it hits; this change just makes the hit unconditional for avm.Script.ProgramExecutable::initializeGlobalPropertiesis the only place program code enters execution: it fetches the unlinked block from theCodeCacheand performs global declaration instantiation (hoistingvars and functions onto the context's global) against it. Since the executable is created insideInterpreter::executeProgram, there is no way to pass it a block directly, which is why the hook lives in the cache lookup and is expressed through theSourceProvider(the object JSC already consults there for disk-cached bytecode).Bun.gc()callsHeap::deleteAllUnlinkedCodeBlocks, which drops every function's cached bytecode; a probe that callsBun.gc(true)between contexts therefore sees function bodies re-parsed on the next call even with this change (and even in the main realm). The test only collects after all runs, and counts program blocks, which that call does not touch.Raw probe output (parse counts per phase; "sibling" = a 16.5 MB script compiled between context 0 and 1)
The probe constructs one
vm.Script, runs it in N fresh contexts (calling the returned wrapper each time and keeping the results alive), and counts theParsed ...lines JSC prints withBUN_JSC_reportParseTimes=1between markers it writes to stderr.