Blob: stop reporting a shared store's bytes to the GC as newly allocated for every view - #38562
Blob: stop reporting a shared store's bytes to the GC as newly allocated for every view#38562robobun wants to merge 4 commits into
Conversation
…ted per view Every JSBlob wrapper passed Blob__estimatedSize to reportExtraMemoryAllocated when it was created. For blobs that share their store (slice(), dupe(), new Blob([blob]), FormData entries, Response(blob).blob()) that number is the whole payload, so N views of a 64 MiB blob told JSC N x 64 MiB had just been allocated and it collected every few views. Classes can now set newlyAllocatedSize in their .classes.ts definition, in which case the generated constructor and create paths report newly_allocated_size() instead, while visitChildren and memoryCost keep using estimated_size(). Blob implements it as the full estimate when it is the only holder of its store and just its own struct otherwise. What each view reports as retained on every GC is unchanged.
|
Warning Review limit reached
Next review available in: 15 seconds 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 (10)
Comment |
|
Status: fixed and ready for a maintainer; CI for the final head is green apart from unrelated lane timeouts.
|
![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 introduces a new codegen opt-in (newlyAllocatedSize) and changes what Blob wrappers report to JSC's GC scheduler, a human look at the design would still be worthwhile.
Checked: the reportExtraMemoryAllocated helper is byte-equivalent to the six inlined blocks it replaces when newlyAllocatedSize is unset; estimate_in_memory_size(blob, true) matches the old calculate_estimated_byte_size body (blob.store() and self.store.get() both project Option<&StoreRef>); Store::has_one_ref() reads the thread-safe refcount and is already used the same way at Blob.rs:3039 and Store.rs:116. Also confirmed Response::estimated_size returns 0 for a Value::Blob body, so the new Response(blob).blob() test row does not over-report through the intermediate Response.
Extended reasoning...
Overview
Seven files: a new newlyAllocatedSize?: boolean field on ClassDefinition (class-definitions.ts), a reportExtraMemoryAllocated helper in generate-classes.ts that replaces six identical inline blocks and picks ${T}__newlyAllocatedSize over ${T}__estimatedSize when the flag is set (plus the extern decl and Rust thunk), newlyAllocatedSize: true on Blob in response.classes.ts, and in Blob.rs a refactor of calculate_estimated_byte_size into estimate_in_memory_size(&Blob, include_store: bool) plus a one-line newly_allocated_size() that passes store.has_one_ref() for include_store. The rest is a bench case, a heapStats()-based test suite, and a SKILL.md doc update.
Security risks
None. The change only alters the size value passed to Heap::reportExtraMemoryAllocated, which feeds GC scheduling heuristics — it does not affect what is marked, freed, or rooted. No user input reaches new code paths; newly_allocated_size() reads the store refcount on the JS thread right after wrapper creation, matching existing has_one_ref() call sites.
Level of scrutiny
Medium-high. The codegen refactor itself is mechanical and the PR states it verified byte-identical output for every class other than Blob, which I can believe from reading the helper. But two things merit maintainer eyes: (1) newlyAllocatedSize is a new cross-cutting codegen abstraction (REVIEW.md flags those for maintainer agreement), and (2) the choice of has_one_ref() as the discriminator means the first wrapper of a store reports the bytes and later ones don't — the PR's argument for why this is the right accounting under JSC's allocated/visited contract is convincing, but it's a GC-behavior design call.
Other factors
The test relies on JSC's 8 MiB largeHeapSize allowance and heapStats().objectTypeCounts staying stable across 256 unretained allocations after a full GC — that mechanism is well-explained in the PR and the negative cases (new Blob([bytes]), new Response(bytes).blob()) confirm owned bytes still trigger collection. I checked that Response's own estimated_size returns 0 for a Value::Blob body (Body.rs:749), so the new Response(blob).blob() row cannot over-report via the intermediate Response wrapper. No CODEOWNERS cover the touched paths. No prior reviewer comments to address.
|
Nothing to change from the review above. The two points it asks a human to weigh are the ones the description argues under Fix: the opt-in exists because the generated creation sites and |
|
Related: #38626 makes |
…ry wrapper creation site The six generated creation sites and the three hand-written ones (JSBunRequest, JSBakeResponse, ShellBindings) each inlined a call to <T>__estimatedSize followed by Heap::reportExtraMemoryAllocated. They now call one generated member per class, so the class definition alone decides whether creation reports estimated_size() or newly_allocated_size(), and hand-written wrapper sites (JSDOMFile next) have the same thing to call.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Revised after review (7c75a75, e0b7840); description updated to match.
|
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. Because it introduces a new codegen opt-in (newlyAllocatedSize) that other PRs will build on and changes what Blob wrappers report to JSC's allocation-since-last-GC counter, a maintainer look at the abstraction and the has_one_ref() heuristic would still be worthwhile.
What was reviewed:
- Codegen refactor: the six inlined
reportExtraMemoryAllocated(estimatedSize)sites collapse to one generated member; for every class withoutnewlyAllocatedSizethe emitted C++ is behaviorally identical, and the three hand-written call sites now go through the same member (net deletion of externs). calculate_estimated_byte_size→estimate_in_memory_size(blob, true)is a line-for-line move; the visited-side number andestimateShallowMemoryUsageOfare unchanged.has_one_ref()is the same sole-owner test already used inStore::to_any_bloband the transfer path; it runs on the JS thread right after wrapper creation, so reading the store there doesn't race with marking.- Test design: full GC first, then 256 wrappers well under the 8 MiB allowance, with negative controls (
new Blob([bytes]),Response(bytes).blob()) confirming owned bytes still trigger collections.
Extended reasoning...
Overview
This PR fixes over-reporting of allocated memory to JSC when a Blob wrapper is created that shares an existing Store (slice(), new Blob([blob]), formData.get(), Response(blob).blob()). It does so by (1) generating a JS<T>::reportExtraMemoryAllocated(vm) member for every estimatedSize class and routing all nine creation-time report sites through it, and (2) adding an opt-in newlyAllocatedSize hook that lets Blob report only its own struct when it is not the sole holder of its store. Ten files change: codegen (class-definitions.ts, generate-classes.ts), three hand-written C++ wrapper sites (JSBunRequest.cpp, JSBakeResponse.cpp, ShellBindings.cpp), Blob.rs, response.classes.ts, a new test block in blob.test.ts, a bench case, and the GC skill doc.
Security risks
None identified. This is GC allocation-budget accounting; no user input parsing, no auth/crypto, no resource limits. The worst failure mode is a performance regression (too few or too many collections), not a safety issue.
Level of scrutiny
High. This is not a mechanical fix: it introduces a new codegen abstraction that four related open PRs (#37667, #37697, #37656, #36001, plus follow-up work for #38626 and Request/Response string bodies) are expected to build on, and it changes what a core type reports to JSC's bytes-allocated-this-cycle counter. The has_one_ref() heuristic is a design choice — it means "no other holder currently shares these bytes" at the instant the wrapper is created, which is correct for the cases enumerated, but a maintainer should confirm it's the boundary they want (e.g., a natively-held second ref that never gets a JS wrapper still suppresses the report). The visited-side estimate is deliberately left unchanged so heap snapshots and estimateShallowMemoryUsageOf keep showing the payload; the FormData heap-snapshot test guards that.
Other factors
- The codegen change was traced: for classes without the new flag, the generated member body is exactly
vm.heap.reportExtraMemoryAllocated(this, T__estimatedSize(m_ctx)), and every prior inlined site now emitsinstance->reportExtraMemoryAllocated(vm)afterm_ctxis set — same call, same timing. - The
calculate_estimated_byte_sizebody was moved verbatim intoestimate_in_memory_size(blob, true); I diffed the two and the only change isself.store.get()→blob.store(), which is the existing accessor used throughout the file. - The new tests use
heapStats().objectTypeCountsafter a forced full GC to observe whether any collection ran during 256 iterations, with both positive rows (shared store → all 256 survive) and negative controls (owned bytes → fewer survive). The 8 MiB allowance the test relies on is set inZigGlobalObject.cppand 256 × sizeof(Blob) is far below it, so the exact-count assertion should be stable; thetoBeLessThan(VIEWS)control is intentionally loose. - All comment-cop threads are resolved (comments trimmed in e0b7840); no outstanding human review comments.
- Given the cross-PR design surface and the GC-semantics nature of the change, deferring to a maintainer rather than auto-approving.
|
On the one design point in the review above (a second ref held natively, with no wrapper of its own, also suppresses the creation-time report): that is intended, and it is bounded. The bytes are still in that holder's retained-side number, so they are counted at the next collection and keep feeding the heap size from then on; all that is lost is the immediate bump to the allocation budget, once per buffer. Today the concrete case is a multipart upload parsed by formData(): the entries are dupes, so the first wrapper from get() reports only its struct, and the payload shows up through FormData's memoryCost on the next GC (#37697 is what makes that entry number non-zero). Reporting the payload from the first wrapper instead would have to be sticky per store to avoid re-reporting from the second wrapper, which is the variant described in the Fix section that broke the FormData size test, so the refcount check at creation is the simplest rule that is never worse than the next GC. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds a new codegen hook (newlyAllocatedSize) and changes GC allocation-time accounting for Blob — with an acknowledged edge case where a native holder suppresses the first wrapper's report — a human look at the design would still be worthwhile.
What was reviewed:
- The six generated
reportExtraMemoryAllocatedsites collapse into one member; verified the substitution is behavior-preserving for classes withoutnewlyAllocatedSize. - The three hand-written call sites (
JSBunRequest,JSBakeResponse,ShellBindings) — the removed externs match the generated declarations, andm_ctxis set before each call. estimate_in_memory_sizeis a pure extraction ofcalculate_estimated_byte_size;estimated_size()and the visited-side number are unchanged.- The new tests cover both directions (shared store survives, owned bytes still collect) and pin down the
new File([blob])path for #37667.
Extended reasoning...
Overview
This PR fixes over-reporting of GC extra memory when creating Blob views that share an existing store (slice(), new Blob([blob]), formData.get(), Response(blob).blob()). It does so by (a) generating a JS<T>::reportExtraMemoryAllocated(vm) member on every estimatedSize class and routing all nine creation sites (six generated, three hand-written) through it, and (b) adding an opt-in newlyAllocatedSize flag in .classes.ts so Blob can report only its struct when its store is shared (has_one_ref() is false). The retained-side number reported from visitChildren is unchanged. Ten files touched: codegen (generate-classes.ts, class-definitions.ts), three C++ hand-written wrapper sites, Blob.rs, response.classes.ts, tests, a bench snippet, and the GC skill doc.
Security risks
None. This is GC scheduling heuristics — no user input parsing, no auth, no resource-limit enforcement. The worst failure mode is under- or over-collection, which the tests bound in both directions.
Level of scrutiny
High. The change is small in line count but cross-cutting: generate-classes.ts affects every generated class with estimatedSize, and the new newlyAllocatedSize hook is a codegen abstraction that will be reused (Request/Response string bodies, #36001, #38626 are named as follow-ups). REVIEW.md flags new cross-cutting abstractions as needing maintainer agreement. The has_one_ref() heuristic has an acknowledged edge case (a store held by a native container like FormData, so the first JS wrapper reports only its struct and the payload is not counted until the next GC's visited pass) — the author argues this is bounded and simpler than a per-store sticky flag, which is reasonable, but it is a design tradeoff a maintainer should sign off on.
Other factors
The PR is exceptionally well-documented and tested. The generated-code refactor is mechanically verified to be behavior-preserving for classes that do not opt in (same estimatedSize symbol, same call site ordering after m_ctx is set). The Rust refactor of calculate_estimated_byte_size into estimate_in_memory_size(blob, include_store) preserves the exact computation for the include_store=true path. The new tests assert both that shared-store views survive (all 256) and that owned-bytes Blobs still trigger collection (toBeLessThan(VIEWS)), which guards against regressing #31422-style under-reporting. The comment-cop threads were all addressed in e0b7840. No human reviewer has weighed in yet; deferring so one can confirm the codegen hook shape and the has_one_ref() tradeoff before it becomes the pattern for the follow-ups.
|
Updated 5:52 PM PT - Aug 14th, 2026
❌ @robobun, your commit 8afc84d has 3 failures in
🧪 To try this PR locally: bunx bun-pr 38562That installs a local version of the PR into your bun-38562 --bun |
1 participant
Problem
blob.slice(),new Blob([blob]),formData.get(),new Response(blob).blob()) makes JSC run a collection every few Blobs when the source is large:BUN_JSC_logGC=1shows 892 collections for the 6000slice()calls in the repro below, andslice()of an 8 MiB blob takes 26.75 us against 180 ns for a tiny blob inbench/snippets/blob.mjs, although both are zero-copy. Reproduces on 1.3.14 and on main.Blob__estimatedSize(ptr)toHeap::reportExtraMemoryAllocated. For a Blob that only took a reference to an existing store that number is the whole payload (calculate_estimated_byte_size,src/runtime/webcore/Blob.rs), so N views of a 64 MiB blob tell JSC N x 64 MiB were just allocated.src/codegen/generate-classes.ts, plusJSBunRequest.cpp,JSBakeResponse.cppandShellBindings.cppby hand), and the two hand-written Blob wrapper sites (JSDOMFile.cpp,JSS3File.cpp, see Report File and S3File sizes to the GC when their wrappers are created #37667) had nothing to call, so there was no single place to fix what gets reported.Fix
estimatedSizegets a generatedJS<T>::reportExtraMemoryAllocated(vm); the six generated creation sites and the three hand-written ones call it instead of inlining the report (net deletion of three hand-declared externs). The generated.cppchanges by exactly that mechanical substitution for every class (diffed the codegen output).newlyAllocatedSizein.classes.tsmakes that member reportnewly_allocated_size()instead ofestimated_size();visitChildrenandmemoryCostkeep usingestimated_size(). Blob is the only class that sets it.Blob::newly_allocated_sizereturns the full estimate when the Blob is the only holder of its store (Store::has_one_ref, the same testStore::memory_costalready uses) andsize_of::<Blob>()plus its own content type and name otherwise. It runs on the JS thread right after the wrapper is created, so reading the store there is fine; what GC marking threads read is still the cached field.reportExtraMemoryAllocatedfeeds the bytes-allocated-this-cycle counter that schedules collections, so it has to describe what the new wrapper allocated. A view allocated its struct and nothing else; the payload is accounted for through whoever else holds the store. A sole holder (new Blob([bytes]),structuredClone,Response(bytes).blob()) did just allocate the bytes and still reports them.estimated_size, also whatestimateShallowMemoryUsageOfand heap snapshots show) is deliberately unchanged: a FormData entry or anew File([blob])has to keep reporting the bytes so they stay visible to the GC after the original Blob dies (heap-snapshot.test.ts"FormData" asserts this; a first attempt that deduplicated that side as well failed it). That number is read from marking threads off a cache and cannot consult the refcount anyway.test/js/web/fetch/blob.test.ts, describe "a Blob sharing another Blob's bytes does not report them to the GC as newly allocated". After a full GC, 256 wrappers that report only their struct cannot reach JSC's 8 MiB allowance, soheapStats()still counts all 256; before the fix only 8 to 41 survive forslice(),new Blob([blob]),formData.get()andResponse(blob).blob()(1.3.14 and an unfixed build of main).new File([blob], name)passes before and after: it is the one Blob creation site outside the generated ones and reports nothing today, and the row pins down what it may report once it does.new Blob([bytes])andResponse(bytes).blob()check that owned bytes still trigger collections (6 to 10 of 256 survive with the fix).blob.test.ts(82 pass),heap-snapshot.test.ts(FormData passes; its URL and Headers tests exceed the 5 s timeout under debug+ASAN with or without this change, see test: keep the URL and Headers heap-snapshot tests under the default timeout on debug builds #37835),blob-cow,blob-array-fast-path,structured-clone-blob-file,html/FormData,fetch/body,fetch/request,fetch/response, and for the converted hand-written siteshttp/serve-request-extra-memory(Bun.serve: restore per-request GC memory accounting to fix elevated RSS under HTTP load #31422),http/bun-serve-routes,http/bun-serve-cookies,bake/dev/response-to-bake-response,bake/dev/react-response,shell/lazy,shell/bunshell-instance, pluszlib-estimated-size-gcandnet/blocklist-gcfor two other generated classes.cargo fmt, clang-format and prettier are clean.blob.slice()68.6 ns,blob.slice() (8 MiB blob)72.1 ns (new bench case), 1 collection during the repro. Unfixed release build of main on the same machine: 180 ns and 26.75 us, 892 collections.Scope and related PRs
Request/Responsehave the same shape for string bodies:new Response(sharedString)refs the string'sStringImpl(Body.rsValue::WTFStringImpl), which JSC already accounted for on the JSString, butResponse::estimated_sizereports its byte length as allocated per wrapper (measured with the recipe above: 4 of 256 survive fornew Response(str), 19 forclone(), 12 fornew Request(url, {body});new Response(blob)is fine becauseValue::Blobalready counts as 0). Fixing that needs a decision in Body.rs about natively created strings and a re-check of the Bun.serve RSS plateau from Bun.serve: restore per-request GC memory accounting to fix elevated RSS under HTTP load #31422, so it is filed separately; with the generated member in place it is aBody::Valuemethod plus the flag on two class definitions.new File()/ S3 creation) should call the generated member; thenew File([blob], name)row fails if it reports the estimate instead. blob: report the size of Blobs created by the native bindings to the GC #37697 and Blob: wrap blobs for JS through JsClass::to_js only #37656 fill in the cached estimate for natively created Blobs and only touch the retained-side number, so either landing order works; Blob: wrap blobs for JS through JsClass::to_js only #37656 movescalculate_estimated_byte_size, andnewly_allocated_size/estimate_in_memory_sizemove with it (noted on that PR). webcore(Blob): share Blob-typed parts via a rope store instead of eager memcpy #36001 (shared parts) will want its ownnewly_allocated_size. Each of these PRs has a comment pointing here.Background
reportExtraMemoryAllocatedwhen the wrapper is created, which counts toward the budget that triggers the next collection (and may start one on the spot), andreportExtraMemoryVisitedfromvisitChildrenon every GC, which measures what is still retained and sizes the next budget. For codegen classes both numbers came from the oneestimated_size()Rust method; this PR lets a class supply a different number for the first.offset,size) onto a refcountedStorethat owns the bytes.slice(),dupe()and the single-Blob-part constructor fast path create a new Blob pointing at the same store; FormData,MessageEventand Response bodies hold their owndupe().Store::has_one_ref()is therefore "no other Blob or holder currently shares these bytes".Blob.reported_estimated_sizeis computed before the Blob is handed to a wrapper becauseBlob__estimatedSizeis also called from GC marking threads, where touching the store would race with the JS thread detaching it.heapStats().objectTypeCounts.Blob(bun:jsc) counts live JSBlob cells without collecting, which is how the test observes whether any collection ran. Bun configures the main heap with an 8 MiB minimum allowance per cycle (largeHeapSizeinZigGlobalObject.cpp).Repro and measurements
Unfixed release build of main: 498 / 532 / 608 ms CPU (119 / 124 / 183 ms wall);
BUN_JSC_logGC=1logs 892 collections, each requested with "bytes allocated this cycle ... oversized bytes" where every oversized entry is one 1, 8 or 64 MiB view. 2000slice()calls of a 64-byte blob take 1 ms. This branch: 0 to 1 ms for every size, 1 collection.Survivors out of 256 (the new test's measurement), unfixed main release build / bun 1.3.14 / this branch:
blob.slice()new Blob([blob])formData.get()new Response(blob).blob()new File([blob], name)(reports nothing at creation today)new Blob([bytes])(owns its bytes)new Response(bytes).blob()(owns its bytes)estimateShallowMemoryUsageOffor every producer in the table is identical before and after (the retained-side number is untouched), andsize_ofofBlobandStoreare unchanged; this PR adds no fields.The first revision of this PR switched the symbol at the six generated sites only; the generated member and the conversion of the three hand-written sites were added after review so hand-written wrapper sites report through the same definition.
no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/blob.test.ts