Conversation
Every module load copies its transpiled source into a WTF::StringImpl owned by the Zig::SourceProvider. That string is only freed once the GC drops the executables and code blocks referencing the provider and, because every parse also registers the provider in VM::sourceProviderCacheMap, once a full collection has run. JSC was never told about those bytes, so a loop of require() + delete require.cache over a module whose JS footprint is tiny compared to its source (one big literal, an array of numbers, ...) never requested a collection and kept one copy of the source per load: about 1 MB of RSS per load of a 1 MB module, with no bound. Report the source's cost() through Heap::deprecatedReportExtraMemory when the provider is created, the same accounting a JSString applies to its StringImpl. The reported bytes count toward JSC's allocation budget, so a burst of loads requests an eden collection every ~8 MB of source, and since the reported bytes stay in extraMemorySize() until a full collection, the eden collection's old-generation ratio schedules the full collection that clears the provider cache and frees the strings. Builtin modules are skipped because they live as long as the VM. With this the same loop settles at a few KB of RSS per load (3 KB/load over 300 loads of a 1 MB module, was 1023 KB/load), import() + delete require.cache behaves the same way (59 KB/load, was 1024 KB/load), and loading a 1000 module graph or 20 MB of data modules takes the same time as before.
|
Warning Review limit reached
Next review available in: 7 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 (2)
Comment |
|
Status: ready for review. Reproduced on main with a 1 MB string literal module in a The three new tests in |
![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. The native change is small and follows the existing deprecatedReportExtraMemory pattern (napi, sqlite, JSC__VM__reportExtraMemory), and the tests are JSC-level rather than RSS-based so they should hold under ASAN. Because it changes GC scheduling on every non-builtin module load and hinges on a specific choice of JSC accounting API (deprecatedReportExtraMemory vs reportExtraMemoryAllocated) to force full collections, a human sign-off on the approach would still be worthwhile.
What was reviewed
provider->m_sourceis aRef<StringImpl>initialized in the ctor before the report call, so the deref is safe;cost()is one-shot and 0 for static strings, so plugin/shared strings aren't double-counted.deprecatedReportExtraMemoryonly bumps a counter and callsdidAllocate— no synchronous collection at this call site, so no rooting concern for locals increate().- Test fixtures drain stdout/stderr/exited concurrently, use
tempDir/bunEnv/Buffer.allocper harness conventions, and assert onextraMemorySize/objectTypeCounts.Modulerather than RSS thresholds.
Extended reasoning...
Overview
The PR adds one guarded call in Zig::SourceProvider::create (src/jsc/bindings/ZigSourceProvider.cpp) that reports each non-builtin module's source StringImpl::cost() to Heap::deprecatedReportExtraMemory, plus a new describe block with three tests in test/cli/run/require-cache.test.ts. The goal is to make module source bytes count toward JSC's allocation budget so that a synchronous require/delete require.cache loop over a large-source, low-JS-heap module actually triggers collections instead of piling up one source copy per iteration.
Security risks
None. This is GC accounting only; no user input parsing, no new surface, no auth/crypto/permissions code.
Level of scrutiny
Higher than the diff size suggests. SourceProvider::create runs on every module load, so this changes GC cadence process-wide. The PR description is unusually thorough — it explains why deprecatedReportExtraMemory (which persists in extraMemorySize() until a full collection and thus drives the old-gen ratio check that schedules the full collection needed to clear VM::sourceProviderCacheMap) is the right lever versus cell-tied reportExtraMemoryAllocated, cites perf runs on a 1000-module graph and load-same-js-file-a-lot.test.ts as unchanged within noise, and reports a BUN_JSC_gcMaxHeapSize=8192 stress run over 379 module-loading tests with no crashes. That's exactly the analysis a reviewer would ask for, but the choice of accounting API and the global effect on collection frequency are the kind of design call the review guide flags for maintainer confirmation.
Other factors
- The pattern matches four existing call sites in
src/jsc/bindings/(napinapi_adjust_external_memory, sqlite,JSC__VM__reportExtraMemory), so it is not novel in this codebase. m_sourceis aRef<WTF::StringImpl>set in the ctor, soprovider->m_source->cost()is safe immediately aftergetProvider(); access is from a static member function so visibility is fine.deprecatedReportExtraMemoryincrements a counter and feedsdidAllocate; it does not collect synchronously, so there is no GC-at-unsafe-point concern for the locals increate().- Tests look correct and non-flaky by design: fresh subprocesses, JSC-level counters (
process.memoryUsage().external,heapStats().objectTypeCounts.Module) instead of RSS,Buffer.alloc(n, fill)for the 1 MB literal, stdout/stderr/exited drained concurrently,// @bunprebuilt module used for the 32-iteration loop to keep debug builds fast. TheafterLoop <= LOADS/2bound (16) against a stated observed value of ~6 leaves headroom. - No prior human reviews or outstanding comments on the timeline.
Given the process-wide GC-cadence impact and the deliberate API choice, deferring rather than auto-approving.
|
One correction to the automated review above, for whoever signs off on the approach: |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Neither of the two suggested issues is fixed by this PR, so I am not adding the
This PR only changes whether loading modules counts toward JSC's collection budget; it does not make anything collectable that was not already. |
|
Updated 9:35 AM PT - Aug 13th, 2026
❌ @robobun, your commit 255af16 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38135That installs a local version of the PR into your bun-38135 --bun |
This branch has not been deployed
1 participant
Problem
require(file); delete require.cache[file]in a loop grows RSS by about the module's source size on every load, without bound: 1023 KB per load for a module holding a 1 MB string literal, still linear after 600 loads.await import(file); delete require.cache[file]does the same (1024 KB per load). The JS heap stays flat the whole time.WTF::StringImplowned by the module'sZig::SourceProvider(src/jsc/bindings/ZigSourceProvider.cpp,SourceProvider::create), andBun.gc(true)frees all of the copies (the live allocations of a 50 load loop drop from 54 x 1 MB to the baseline). The copies pile up because no collection ever runs: JSC was never told those bytes exist, and evaluating such a module allocates about 200 bytes on the JS heap (BUN_JSC_logGC:normal bytes: 168), so its 8 MB allocation budget is never reached. Bun's own GC timer does not run inside a synchronous loop either, and in theimport()case it asks for an unscoped collection once a second, which JSC runs as an eden collection since nothing has scheduled a full one, and eden collections do not release the strings (see Background).varstatements) allocates enough JS heap per load to trigger collections on its own, which is also why the existing fixtures inrequire-cache.test.ts(10000 exports or 20000 calls per module) never saw this.Fix
SourceProvider::createreports the source'sStringImpl::cost()throughHeap::deprecatedReportExtraMemoryfor non builtin modules.JSStringapplies to its ownStringImpl(JSString::finishCreationreportscost()); a module's source is the same kind of object, native bytes whose lifetime ends when the GC drops the cells referencing them, so it has to count toward the allocation budget the same way.deprecatedReportExtraMemoryis JSC's API for memory that is not owned by a single cell (it is whatJSReportExtraMemoryCostand WebCore's image and document wrappers use); there is no cell here whose lifetime matches the provider's.didAllocate, so a burst of loads requests an eden collection about every 8 MB of source, and they stay inextraMemorySize()until a full collection, so after that eden collection the old generation ratio check (minEdenToOldGenerationRatio) schedules the full collection that clearssourceProviderCacheMapand frees the strings. A plainreportExtraMemoryAllocatedwith no cell would only ever produce eden collections, which free nothing here. WithBUN_JSC_logGC=1, a 40 load loop now shows 3 eden and 2 full collections, each requested atoversized bytes: 7340900or so, i.e. by the reported sources.cost()reports a givenStringImplonce and is 0 for static strings, so a provider built over a string that already went through aJSString(plugins) or a shared string is not counted twice. Builtins are skipped because they live as long as the VM, so reporting them would be pressure with nothing to reclaim;process.memoryUsage().externalafter requiring 7 builtins is unchanged (834 bytes before and after).import()+delete require.cacheloop from 1024 to 59 KB per load; live allocations after a 50 load loop with no manual GC: 5 x 1 MB instead of 54. Loading a 1000 module graph (12 MB of source) and 200 modules of 100 KB string data (20 MB, the shape that gains the most collections: 3 instead of 1) take the same wall time as before within noise (medians 245 vs 239 ms and 39 vs 41 ms);test/js/bun/resolve/load-same-js-file-a-lot.test.ts(10000 imports) is unchanged within noise.test/cli/run/require-cache.test.ts, describemodule source text is reported to the GC. Two fixtures check thatprocess.memoryUsage().external(which isextraMemorySize()) grows by at least the literal's size after loading a transpiled and a// @bunprebuilt module, viarequire()and viaimport(); before this change they report 0 bytes (1571 for the ESM record). The third runs the synchronous loop over a prebuilt 1 MB module and readsheapStats().objectTypeCounts.Modulewithout forcing a GC: before this change exactly 32 of 32 Module objects are still alive, after it about 6 (the last budget's worth; the bound is 16). The fixtures are JSC level rather than RSS based so they hold under ASAN, where freed strings sit in the quarantine, and they run in fresh processes so the loads are the only possible trigger. The prebuilt form is used for the loop because it skips transpiling 1 MB per iteration in debug builds; it reaches the sameSourceProvider::create.bun bd test) and release; the whole file passes in release (the 7 other leak fixtures in it time out under debug builds with or without this change, as noted in their own timeouts).gc-controller-cadence,crypto-extra-memoryandv8-moduletests pass. As a GC safety check for the new request point,BUN_JSC_gcMaxHeapSize=8192(a collection requested at nearly everycreate()) overtest/cli/runmodule loading tests andtest/js/bun/resolveran 379 tests with no crashes.Background
Zig::SourceProvideris Bun'sJSC::SourceProvider: it owns the module's source text as aWTF::StringImpland is reference counted from theSourceCodeobjects held by JSC's executables and code blocks, so it is destroyed by GC when those cells die. For CommonJS it is created inJSCommonJSModule.cpp, for ESM inModuleLoader.cpp; both go throughSourceProvider::create.VM::sourceProviderCacheMapis a JSC parser cache keyed byRefPtr<SourceProvider>. EveryParserconstruction adds its provider to it, and it is cleared inHeap::deleteSourceProviderCaches, which only does so after a full collection. This is why module sources are released by full collections specifically, and why an eden collection (what Bun's timer or a plain allocation trigger produces) does not help.reportExtraMemoryAllocated(cell, bytes)/reportExtraMemoryVisitedtie native bytes to a cell;deprecatedReportExtraMemory(bytes)adds them to a counter that is reset at the next full collection. Both feedHeap::didAllocate, and a collection is requested once the bytes allocated in the current cycle exceed the budget (Bun sets the initial budget,largeHeapSize, to 8 MB). Reports of 64 KB or more count as oversized and are ignored while the last one is more than a third of the cycle's allocation, which with equal sized modules means a collection is requested by the third load at the latest.process.memoryUsage().externalandbun:jsc'sheapStats().extraMemorySizeboth exposeextraMemorySize().StringImpl::cost()returns the string's byte size the first time it is called on an impl and 0 afterwards (and always 0 for static strings); it exists for exactly this kind of one time report.// @bun/// @bun @bun-cjsis the pragmabun build --target=bunputs at the top of its output; the runtime loader passes such files to JSC without transpiling them.