Repository navigation
Conversation
… JIT code alive Under --isolate today, every test file gets a fresh JSGlobalObject. That means every file re-links every dependency JSModuleRecord and re-tiers every hot dependency function up through LLInt/Baseline/DFG/FTL from scratch. On a 128-file suite importing zod/date-fns/lodash this is ~12s of DFG/FTL worklist CPU per run (7.3s wall) vs ~0.3s on a shared global (2.7s wall). This adds an opt-in BUN_FEATURE_FLAG_EXPERIMENTAL_TEST_ISOLATE_REUSE_GLOBAL path that keeps the global between files and evicts only project-source module records (anything not under node_modules/ and not a node:/bun: builtin) from the ESM registry and require.cache. Dependency module records, and the FunctionExecutable/CodeBlock chain hanging off them, survive across files so optimized code is never recompiled. P0 spike only: no pristine-global fingerprint, no taint graph, no preload-once. Default behaviour unchanged.
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Not adding |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 2 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 (6)
WalkthroughChangesThe test runner adds experimental VM-global reuse for isolated test files. It evicts project modules, preserves dependencies, detects pollution, supports forced fresh globals, and adds serial, parallel, diagnostic, and benchmark coverage. Test isolation global reuse
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/test/app/setup.ts`:
- Around line 11-13: Make the fresh benchmark command explicitly unset
BUN_FEATURE_FLAG_EXPERIMENTAL_TEST_ISOLATE_REUSE_GLOBAL in
bench/test/app/setup.ts lines 11-13 and bench/test/README.md lines 45-48 before
invoking hyperfine; leave the reuse command explicitly setting the flag to 1.
- Around line 34-66: Update the workload setup in hot by defining descriptive
constants for the row count, name modulus, date modulus, chunk size, row-index
mask, and millisecond mask, then replace the repeated literals in row generation
and accumulation with those constants. Keep the row count and index-mask
relationship synchronized so rows[k & mask] remains within the generated rows.
- Around line 19-24: Validate the FILES and ITERS environment values before the
rmSync(root, ...) side effect, distinguishing unset, empty, and zero explicitly.
Reject NaN, non-finite, fractional, negative, and unsafe integer values, and
enforce an appropriate finite upper bound for each resource before continuing to
mkdirSync or generate tests; preserve the existing defaults for unset values.
- Around line 9-16: Update the benchmark commands in bench/test/app/setup.ts
lines 9-16 and bench/test/README.md lines 43-49 to build once with bun bd, then
run every hyperfine sample using build/debug/bun-debug without rebuilding inside
the measured commands. Ensure the fresh-global sample explicitly clears
BUN_FEATURE_FLAG_EXPERIMENTAL_TEST_ISOLATE_REUSE_GLOBAL, while the reuse-global
sample sets it.
In `@bench/test/package.json`:
- Around line 6-9: Pin the benchmark dependencies in bench/test/package.json by
removing the caret ranges from date-fns, lodash-es, vitest, and zod, or commit a
frozen lockfile that guarantees these exact resolved versions for reproducible
benchmark runs.
In `@bench/test/README.md`:
- Line 56: Update the wording in the README sentence so the compound modifier
uses a hyphen: change “dependency optimized code” to “dependency-optimized
code.”
In `@src/jsc/bindings/bindings.cpp`:
- Around line 2827-2842: Update the module eviction block to acquire
moduleLoader->cellLock() before iterating over moduleLoader->moduleMap(), and
keep that lock held through collecting identifiers and calling removeEntry.
Remove the narrower lock scope so the entire traversal and removal sequence is
synchronized.
- Around line 2853-2859: Update the requireMap iteration around keyValue and
keyString to detect non-string keys before calling toWTFString, explicitly
remove those entries from requireMap, and continue without conversion. Preserve
normal string-key handling while ensuring Symbol keys do not remain after the
isolation cleanup.
In `@src/jsc/VirtualMachine.rs`:
- Around line 4857-4859: Update cleanup_between_isolated_test_files and both
isolation transition methods, including reuse_global_for_test_isolation, to
propagate set_process_cwd restoration errors instead of continuing after
saved_cwd is consumed. Change the transition results and serial/parallel callers
so a cleanup failure aborts the file transition and is returned to the caller.
In `@test/cli/test/isolation.test.ts`:
- Around line 978-987: Add a deterministic parallel-worker test case near the
local run helper in isolation.test.ts by invoking the existing run flow with
--parallel=1 and multiple files. Assert both dependency retention and
project-module eviction within the reused worker, covering the parallel runner
branch while preserving the existing serial-path coverage.
- Around line 1111-1139: Strengthen the “leaked timer, server and subprocess are
still torn down between files” test by storing observable handles or state for
the interval and spawned process in `a.test.ts`, then asserting in `b.test.ts`
that the interval no longer executes and the child process has terminated,
alongside the existing server check. Ensure each assertion can fail and verifies
actual resource cleanup rather than only global-property persistence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e0fce00-0786-4e20-a6f7-fd03fdea2071
⛔ Files ignored due to path filters (1)
bench/test/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
bench/test/README.mdbench/test/app/setup.tsbench/test/package.jsonsrc/bun_core/env_var.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/bindings.cppsrc/runtime/cli/test/parallel/runner.rssrc/runtime/cli/test_command.rstest/cli/test/isolation.test.ts
…resh-global Bun__computeIsolationFingerprint digests globalThis + the fixed intrinsic set (ctor/prototype of Object/Function/Array/String/Number/Boolean/Symbol/ BigInt/Date/RegExp/Error/Promise/Map/Set/ArrayBuffer, plus Math/JSON/ Reflect/console/fetch/WeakMap/WeakSet/Error subclasses by slot value, plus process identity + listener presence, plus crypto/Request/Response/Headers/ Buffer identity). Each hashed object's lazy static properties are reified first so test-time reads of e.g. Object.keys don't look like mutation. prepare_global_for_next_test_file runs the shared cleanup then checks hard dirty triggers (mock.module, pending fetches) and the fingerprint. Match: evict project modules (plus anything not in the baseline preload-key snapshot) and reuse. Mismatch: swap to a fresh global, clear baseline + preload snapshot, and let the next load re-run preloads and re-baseline. Preloads run once per global: the module keys live in the registry at baseline time are snapshotted and exempted from eviction, and reload_entry_point_for_test_runner skips load_preloads while a baseline is captured. FirstLast.first tracks the per-global lifecycle so preload-level beforeAll fires once per global; reset_hook_scope_for_test_isolation only runs on the fresh-global path. --isolate now accepts an optional value; --isolate=fresh-global forces the fresh-global path regardless of the feature flag (forwarded to --parallel workers). BUN_DEBUG_test_isolate emits a reused/fresh summary line in release builds too so the isolation tests can assert it. Also: MarkedArgumentBuffer for the require.cache eviction collect, evict non-string require.cache keys instead of swallowing the conversion throw, clearExceptionExceptTermination at every nothrow boundary, and keep node_quic_callbacks.deinit on the fresh-global path only (its one-time initializer lives in a kept built-in module).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/bindings.cpp`:
- Around line 2848-2876: Check evict.hasOverflowed() after collecting entries
and before calling evict.size() or iterating with evict.at(i). If it has
overflowed, stop the CommonJS eviction path, clear or handle the pending state
consistently, and set evictedCjs to zero; otherwise preserve the existing
removal loop and count.
In `@src/runtime/cli/Arguments.rs`:
- Line 598: Update the --isolate help text in the CLI argument definitions to
document the optional fresh-global value, including what --isolate=fresh-global
does, while preserving the existing no-value behavior description and matching
the style used by --bail and --changed.
In `@src/runtime/cli/test/parallel/runner.rs`:
- Around line 556-563: Update the worker reuse transition around
prepare_global_for_next_test_file to match the serial path: match both
TestIsolationOutcome::Reused and TestIsolationOutcome::Fresh, and emit the
corresponding bun_output::scoped_log!(test_isolate, ...) diagnostics with
evicted ESM/CJS counts or the fallback reason. Preserve the existing Fresh
handling that updates next_is_first_on_global and resets the hook scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7bd8bd47-45da-48f0-bb21-2587c3385f17
📒 Files selected for processing (11)
bench/test/README.mdsrc/bun_core/env_var.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/BunPlugin.cppsrc/jsc/bindings/ZigGlobalObject.hsrc/jsc/bindings/bindings.cppsrc/options_types/context.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/cli/test_command.rstest/cli/test/isolation.test.ts
💤 Files with no reviewable changes (1)
- src/bun_core/env_var.rs
…the subprocess-dead poll default_timeout_override was reset to u32::MAX unconditionally after every file, but under reuse preloads don't re-run, so a preload's setDefaultTimeout() was lost for file 2+. Record the value when setDefaultTimeout is called during preload and restore it on the Reused outcome (both serial and parallel); reset to u32::MAX on Fresh / no reuse as before. Also bound the process.kill(pid, 0) assertion in the cleanup test with the same poll the sibling leaked-subprocesses test uses (auto_killer sends SIGTERM without waiting for reap).
…p node_modules on Windows hashObject now clears any exception left by reifyAllStaticProperties before continuing, matching the six other in-tree callers (the PropertyCallback chain on globalObject can open nested ThrowScopes that trip BUN_JSC_validateExceptionChecks otherwise). isKeptAcrossTestIsolation now matches /node_modules/ with either separator on each side on Windows, so a boundary-flip key like C:\proj\node_modules/zod/index.js is kept.
|
CI at 80b4518: the three red lanes are all unrelated to this diff.
|
There was a problem hiding this comment.
No new findings this pass; the earlier inline comments (subprocess-poll flake, preload setDefaultTimeout reset, reifyAllStaticProperties exception check, Windows mixed-separator node_modules) were all addressed in 8b4094f and 80b4518. Deferring to a human for the design/semantics questions the PR itself flags — this is ~1100 lines across the JSC module loader, global-object fingerprinting, and test-runner lifecycle, and the "dep module-level state shared across files" semantic is an explicit open question.
What was reviewed:
Bun__evictProjectModulesForTestIsolation/Bun__computeIsolationFingerprint: exception-scope handling,MarkedArgumentBufferoverflow,cellLockaroundremoveEntry,VMInquiryso no user getters run.prepare_global_for_next_test_file/cleanup_between_isolated_test_filesrefactor:node_quic_callbacks.deinit()correctly kept fresh-global-only;preload_module_keysare.deref()'d on the fresh path.- Serial vs
--parallelworker parity forFirstLast,default_timeout_overriderestore, andreset_hook_scopegating.
Extended reasoning...
Overview
Adds an opt-in BUN_FEATURE_FLAG_EXPERIMENTAL_TEST_ISOLATE_REUSE_GLOBAL path to bun test --isolate that keeps the JSGlobalObject alive across test files (evicting only project-source module records) so dependency JIT code survives. ~1100 lines across 15 files: ~280 lines of new C++ in bindings.cpp (module-registry eviction, intrinsic fingerprinting via structure/property-table walk, module-key iteration), ~190 lines in VirtualMachine.rs (state struct, prepare_global_for_next_test_file, refactored cleanup split), test-runner lifecycle threading in test_command.rs / runner.rs, a new --isolate=<value> CLI arg, and 373 lines of tests plus a benchmark fixture.
Security risks
None identified. The new code runs only under bun test --isolate with an experimental env flag set, operates on the runner's own module registry and global object, and uses VMInquiry/getPureResult so the fingerprint pass doesn't invoke user getters.
Level of scrutiny
High. This touches JSC module-loader internals (removeEntry under cellLock, JSMapIterator over requireMap), walks JSC Structure property tables, calls reifyAllStaticProperties on the global object and ~30 prototypes, and reshapes the per-file lifecycle (FirstLast, preload-once, hook-scope reset gating) across two entry points. The default path is unchanged and everything new is behind an experimental flag, which lowers blast radius — but the refactor of swap_global_for_test_isolation into cleanup_between_isolated_test_files + swap_global_for_test_isolation_inner affects the non-flag path too and warrants human eyes.
Other factors
Two prior automated review rounds surfaced five issues (test flake, setDefaultTimeout snapshot/restore, worker afterAll gap, reifyAllStaticProperties exception check, Windows node_modules separator handling), all of which the author addressed or explicitly scoped out in the PR description. The PR itself lists open design questions ("whether dep module-level state shared across files is the right default --isolate semantics") and known follow-ups (taint graph, spyOn restore, worker preload-afterAll) that a maintainer should weigh in on before this lands. Test coverage is thorough (15 cases including the negative flag-off arm and --parallel worker), and the ASAN/release gate output in the PR body shows both passing.
|
Closing in favor of #36871, which implements the same opt-in global reuse for |
Problem
Under
--isolate, every test file gets a freshJSGlobalObject, which means a freshJSModuleLoader: every file re-links every dependencyJSModuleRecordand re-creates everyModuleProgramExecutable/FunctionExecutable/CodeBlockchain. JSC then re-tiers every hot dependency function through LLInt/Baseline/DFG/FTL per file. On a 128-file suite that exercises zod/date-fns/lodash this is ~12s of DFG/FTL worklist CPU per run; the same suite on a shared global spends ~0.3s in JIT.Change
Adds an opt-in
BUN_FEATURE_FLAG_EXPERIMENTAL_TEST_ISOLATE_REUSE_GLOBALpath. Default behaviour is unchanged.Per-file boundary (
prepare_global_for_next_test_file):close_isolation_handles,auto_killer.kill(),cancel_all_timers, generation bump, entry-point reset), factored intocleanup_between_isolated_test_files.node_quic_callbacks.deinit()stays on the fresh-global path only since its once-only initializer lives in a kept built-in module.mock.modulewas called (per-global flag set inJSMock__jsModuleMock), or the loader has aStatus::Fetchingentry.Bun__computeIsolationFingerprint: FNV-1a over globalThis plus the constructor + prototype of Object/Function/Array/String/Number/Boolean/Symbol/BigInt/Date/RegExp/Error/Promise/Map/Set/ArrayBuffer, and Math/JSON/Reflect/console/fetch/WeakMap/WeakSet/the Error subclasses by slot value, plus crypto/Request/Response/Headers/Buffer/process identity, plusprocesslistener presence. Each hashed object's static-table properties are reified first so a test merely readingObject.keys/JSON.stringify/Number.isIntegerdoes not look like mutation; depth is the property table only, andVMInquiryis used so no getters run.node_modules/, not anode:/bun:/internal:builtin, and not in the baseline preload-key snapshot) from the ESM registry (removeEntryundercellLock) andrequire.cache(MarkedArgumentBuffercollect, non-string keys evicted outright). Mismatch or hard-dirty → fall back to the existing fresh-global swap, clear the baseline, and let the next load re-run preloads + re-baseline.Preloads run once per global.
reload_entry_point_for_test_runnerskipsload_preloadswhile a baseline is captured; the module keys present in the registry immediately after preloads run are snapshotted viaBun__forEachModuleKeyForTestIsolationand exempted from eviction.FirstLast.firsttracks the per-global lifecycle so preload-levelbeforeAllfires once per global, andreset_hook_scope_for_test_isolationonly runs on the fresh-global path (preloadbeforeEach/afterEachregistered inhook_scopepersist across reused files).Escape hatch:
--isolatenow accepts an optional value;--isolate=fresh-globalforces the fresh-global path regardless of the flag (forwarded to--parallelworkers).Diagnostics:
BUN_DEBUG_test_isolate=1emits areused/fresh (fingerprint/mock.module/pending-fetch)summary line on stderr in release builds too, which the tests assert against.Measurement
bench/test/app/setup.tsgenerates the 128-file zod/date-fns/lodash suite. Release build, 16 cores:--isolate--isolate(floor)--parallel(16 workers)Serial reuse reports
127 reused, 0 fresh. User-CPU 15.5s → 5.1s: ~10.4s of JIT-worklist CPU eliminated. Wall 1.65x faster; RSS down ~40%. The gap to the shared-global floor is (a) the project-sidehot.tsre-evaluating per file by design, and (b) the baseline reify making the core prototypes dictionary objects, which costs a little on inline caches; that second part is a known tuning target.Scope
Opt-in behind a feature flag; default behaviour unchanged. Still to come:
node_modulespackage that transitively imports a project module is currently kept.spyOnrestore.afterAllon a mid-run fresh-global fallback: the outgoing global's preloadafterAllis not run (serial runs fire it once at the end; workers never fire it).Open questions for before any default flip: whether dep module-level state shared across files is the right default
--isolatesemantics, and whether to also capnumberOfDFGCompilerThreads/numberOfFTLCompilerThreadsin--test-workerprocesses.Tests
test/cli/test/isolation.test.tsgains a 15-caseBUN_FEATURE_FLAG_EXPERIMENTAL_TEST_ISOLATE_REUSE_GLOBALdescribe:node_modulesrecord identity survives across files; project module re-evaluates fresh (serial and--parallel=2worker)node_moduleskept inrequire.cache, project.cjsevictedglobalThis.x,Array.prototype.foo,Math.random,console.log,process.on('exit', ...), each asserting exactly one fingerprint fallbackmock.modulein A forces a fresh global; B sees the real module--preloadruns once per global; runs again after a forced fallback--isolate=fresh-globaldisables reuse;--isolate=<unknown>is rejected[review] gate passed · iteration 1 · 15 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