Conversation
…f checkSyntax The constructor parsed the source with checkSyntax() to throw SyntaxError eagerly and threw the result away, so the first run (or produceCachedData) parsed it again. Compiling through the CodeCache keeps the eager error and leaves the code where JSC::evaluate and getBytecode look for it.
|
Warning Review limit reached
Next review available in: 4 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 (3)
Comment |
|
Status: ready for review; CI green at the current head (build 93978). No WebKit dependency. Relationship to #38040 (which replaced #37950 on top of oven-sh/WebKit#420): #38040 contains this same constructor change as part of holding the compiled block for every run. Either this lands first as the part that needs no WebKit bump (and #38040's constructor hunk becomes a small rebase), or this gets closed when #38040 lands; what is unique here is the parse-count tests and Reproduced by counting JSC's parse log ( |
|
Updated 12:05 AM PT - Aug 13th, 2026
❌ @robobun, your commit 1edc535 has some failures in 🧪 To try this PR locally: bunx bun-pr 37998That installs a local version of the PR into your bun-37998 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate in the sense of competing: this is the subset of #37950 that needs no JSC change, split out on request. #37950 is on hold until the replacement JSC API lands and will be rebased on top of this (it adds keeping the parsed block alive per Script; this PR only stops the constructor's parse from being thrown away). |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing the earlier parsesDuring nit — the fixture now surfaces stderr on failure and stdout is ignored. I re-reviewed and found no bugs, but since this swaps checkSyntax for a direct CodeCache::getUnlinkedProgramCodeBlock call in the constructor path every vm.Script goes through, a maintainer familiar with JSC's CodeCache keying should confirm.
What was reviewed:
- The new call passes
globalObject->defaultCodeGenerationMode()where the two existinggetUnlinkedProgramCodeBlocksites inNodeVM.cpppass{}; the parse-count test proves they match in the default case, so a mismatch under a non-default mode would only fall back to the old double-parse, not misbehave. - The throwaway
ProgramExecutable::createresult is stack-reachable through the call and neither existing call site treatscreateas throwing. - Only
parseError.isValid()is checked (return value discarded); a null-without-error return would just leave the cache unpopulated, same as before. - The existing SyntaxError-at-construction tests (arrow decoration, lineOffset, prepareStackTrace) cover the error path unchanged, and the per-context-globals test guards the run side.
Extended reasoning...
Overview
The functional change is one line in src/jsc/bindings/NodeVMScript.cpp: constructScript replaces JSC::checkSyntax(vm, source, parseError) with vm.codeCache()->getUnlinkedProgramCodeBlock(vm, JSC::ProgramExecutable::create(globalObject, source), source, globalObject->defaultCodeGenerationMode(), parseError), so the eager syntax check populates JSC's CodeCache instead of discarding its parse. The rest is a new describe.concurrent block in test/js/node/vm/vm.test.ts that counts Parsed lines under BUN_JSC_reportParseTimes=1, and a new bench snippet.
Security risks
None identified. This changes how a script is parsed at construction, not whether or what is parsed. The SyntaxError path (parseError.isValid() → toErrorObject → decorateParseErrorStack) is byte-identical to before, and the extensive existing tests for compile-time SyntaxError decoration remain green.
Level of scrutiny
High — this is native C++ in the JSC bindings, on a path every new vm.Script() executes. The change is small and uses a pattern that already appears twice in NodeVM.cpp (getBytecode and the compileFunction path both call getUnlinkedProgramCodeBlock), but confirming that the CodeCache key produced here matches what JSC::evaluate and getBytecode look up in all configurations (not just the default the test exercises) requires JSC-internal knowledge. The throwaway ProgramExecutable is a slightly unusual shape: it's created solely so the CodeCache call has an executable argument, then dropped — a maintainer should confirm nothing in the CodeCache retains a reference to it that would matter.
Other factors
My earlier inline comment on parsesDuring (undrained stdout pipe, exitCode asserted before stderr) was addressed in 6275679: stdout is now "ignore" and a non-zero exit or missing marker throws with the full stderr. The comment-cop bot fired on the constructor comment length and it's now one line. The new tests are well-designed (warmup to isolate the measured parse, per-context-globals guard, produceCachedData variant), and the author reports all 97 node test-vm-* files pass. Given the JSC-internals nature, a human sign-off is still appropriate.
|
#37950 is now closed in favor of #38040 (with oven-sh/WebKit#420). #38040 also replaces the constructor's This PR stays open as the interim version with no WebKit dependency; it can be closed once #38040 lands. |
|
#38040 landed on main (1b881a9), and it contains this constructor change as part of holding the compiled block for every run, which is also what the merge conflict here is: main replaced the same lines. Closing as agreed. If anyone wants the two pieces that were unique to this PR, the parse-count tests in test/js/node/vm/vm.test.ts and bench/snippets/node-vm-script-contexts.mjs, they are on the farm/b63639a2/vm-script-compile-once branch. |
2 participants
Split out of #37950 (on hold until the replacement JSC API lands); this part needs no WebKit change.
Problem
new vm.Script(src)parsedsrctwice: the constructor ranJSC::checkSyntaxto throwSyntaxErroreagerly and discarded the result (src/jsc/bindings/NodeVMScript.cpp,constructScript), then the firstrunInContext/runInThisContext(JSC::evaluate) orproduceCachedData(getBytecode) parsed it again, because neither finds anything in JSC'sCodeCache. For a 20 MB bundle that is ~0.4 s (release) of duplicated work per Script; the constructor comment already described compile-once as the follow-up.Fix
vm.codeCache()->getUnlinkedProgramCodeBlock()instead ofcheckSyntax(). Syntax errors come out of the sameParserErrorand are decorated exactly as before; the difference is that the parsed code is now in theCodeCacheunder the key the first run andgetBytecodelook up (same provider, same code generation mode, and the key is built before parsing so the strictness bit matches too), so both hit instead of parsing.script-leak.test.tsends at 34 MB RSS growth instead of 139 MB, because a lookup that hits lets the cache shrink and prune the dead entries sooner.test/js/node/vm/vm.test.ts, newdescribe"Script parses its source once": counts JSC'sParsedlines (BUN_JSC_reportParseTimes=1) around construct+run and around construct withproduceCachedData; both report 2 on the unfixed build (release and debug) and 1 here. A per-context global declaration check guards that runs still instantiate globals per context.test/js/node/vm/(230 pass;script-leak.test.tshits its 5 s timeout on this slow ASAN machine with and without the change) and all 97 nodetest/parallel/test-vm-*files pass.bench/snippets/node-vm-script-contexts.mjsis the measurement script used for vm.Script: parse once and reuse the unlinked code in every context it runs in #37950 (per-phase wall time and parser runs for one Script across N contexts, with--evictand--cached-datavariants); on a 1 MB source it shows the first context's evaluate going from 1 parser run to 0 with this change, everything else unchanged.Background
CodeCacheis JSC's in-memory map from source text (plus parse flags) to the parsed, realm-independentUnlinkedProgramCodeBlock.ProgramExecutable::initializeGlobalProperties, which everyJSC::evaluategoes through, asks it first and only parses on a miss;checkSyntaxnever touches it, which is why the old constructor's work could not be reused. What the still-parked vm.Script: parse once and reuse the unlinked code in every context it runs in #37950 adds on top is keeping the block alive per Script so later contexts do not depend on the cache keeping the entry.[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file