UnlinkedMetadataTable: hold only the offset table while no CodeBlock is linked - #496
UnlinkedMetadataTable: hold only the offset table while no CodeBlock is linked#496Jarred-Sumner wants to merge 1 commit into
Conversation
…is linked finalize() (and the bytecode-cache decoder) allocated the full value-profile + LinkingData + metadata buffer up front and the UnlinkedCodeBlock kept it for its whole life, including after every CodeBlock linked from it had been destroyed, and for cache-decoded functions that were never linked at all. The unlinked table now holds just the offset table; link() allocates the full buffer (zeroed, as before) and unlink() of the owning MetadataTable shrinks back to the offset table. Decoding every function of a 75 MB bytecode cache (48k UnlinkedCodeBlocks, none linked): peak RSS 217 MB -> 175 MB.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 37 minutes Limit details: You’ve used the included review currently available. Your 63 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 28 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
![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 changes buffer-lifetime and layout invariants in UnlinkedMetadataTable — a structure every executed function's CodeBlock depends on — a human look would still be worthwhile.
What was reviewed:
prefixSize()gating ofbuffer()/offsetTable16/32()— all callers (totalSize,link,unlink,sizeInBytesForGC) read the correct region in both linked and unlinked states.link()sizing:zeroedMalloc(sizeof(LinkingData)+totalSize)withtotalSize = valueProfileSize + offsetTable[last]matches the old allocation; the offset-tablememcpylands at the same offset the old code used.unlink()freeing the shared buffer:MetadataTable::destroymoves out and destructsLinkingDatabefore callingunlink, so the free is safe; the compact copy preserves the zeroed 16-bit prefix for 32-bit tables via the fulloffsetTableSizememcpy.sizeInBytesForGC(MetadataTable&)still only compares pointers againstbuffer(); a torn read ofm_isLinked/m_rawBufferfrom the GC thread affects only the reported byte count, not correctness.
Extended reasoning...
Overview
The PR changes UnlinkedMetadataTable so that while no MetadataTable shares its buffer (!m_isLinked), m_rawBuffer holds only the offset table rather than the full valueProfiles + LinkingData + offsets + metadata layout. finalize() and the bytecode-cache constructor now allocate only the offset table; link() always allocates a fresh full-size zeroed buffer and copies the offset table in; unlink() of the owning MetadataTable frees the shared buffer and keeps a compact offset-table copy. buffer()/offsetTable16()/offsetTable32() are routed through a new prefixSize() that returns 0 when unlinked and valueProfileSize + sizeof(LinkingData) when linked. CachedMetadataTable::decode drops the now-unused lastOffset argument.
Security risks
No new external inputs or attack surface. The risk profile is memory-safety within the engine: use-after-free or buffer-layout confusion if the m_isLinked ↔ buffer-shape invariant were ever violated. I traced the state transitions (finalize → link → unlink → link …) and the invariant holds at each step; MetadataTable::destroy already destructs LinkingData before unlink() frees the underlying allocation, and the destructor's ASSERT(!m_isLinked) still holds because the owning MetadataTable keeps a Ref to the unlinked table. The 32-bit path's zeroed 16-bit prefix (used by MetadataTable::is32Bit()) is preserved through zeroedMalloc in finalize/link and the full-offsetTableSize memcpy in unlink.
Level of scrutiny
High. This is core bytecode infrastructure on the execution path of every JS function, and the change replaces a "buffer never moves after finalize" invariant with one where m_rawBuffer is freed and reallocated on both link() and owning-unlink(). The class is ThreadSafeRefCounted and sizeInBytesForGC(MetadataTable&) is called from GC visitation; its existing comment about m_rawBuffer being realloced concurrently now applies to more transitions. The PR description addresses this (pointer comparison only), and I agree, but the concurrency and lifetime reasoning here is subtle enough that it should not be merged on an automated review alone.
Other factors
The author reports JSTests/stress (default + disk-cache, 5,259 tests) passing identically to main, and a −42.6 MB RSS win on a 48k-function bytecode cache. The diff is small and focused, and I found no defects — deferring purely on criticality of the code path, not on any identified problem.
Preview Builds
|
|
Folded into #494. |
1 participant
Split out of the startup-snapshot branch (#397's description: "the
UnlinkedMetadataTablecompaction that used to be here is unrelated to snapshots and will be proposed separately"). Independent of #494/#495.What
UnlinkedMetadataTable::finalize()allocatedvalueProfiles + LinkingData + offset table + all metadata entriesas one buffer, and the bytecode-cache decoder (CachedMetadataTable::decode→ the 3-argument constructor) allocated the same full-size buffer for every function it decoded. TheUnlinkedCodeBlockthen owned that buffer for its lifetime: the firstlink()shared it, later ones copied, and when the sharingMetadataTablediedunlink()only clearedm_isLinked— the value-profile/metadata bytes stayed allocated with noCodeBlockusing them. For executables generated from a bytecode cache theUnlinkedCodeBlockedge is strong, so that is the life of the process.Now the unlinked state holds only the offset table (
s_offset16TableSize, plus the 32-bit table when needed).link()always allocates a fresh zeroedLinkingData + totalSizebuffer and copies the offset table in (the first link previously reused the finalize-time buffer and memset it; same cost).unlink()of the owning table frees it and keeps a compact copy of the offset table.buffer()/offsetTable16()/offsetTable32()go throughprefixSize(), which is 0 while unlinked. The cache decoder's constructor loses its now-unusedlastOffsetparameter.MetadataTable::destroyalready documents thatunlinkfrees the table's memory and destroysLinkingDatafirst;sizeInBytesForGC(MetadataTable&)still only compares pointers.Measured
jsc shell, x64 Linux, decoding every function of a 75 MB cache built from a 12 MB minified bundle (48,040
UnlinkedCodeBlocks, none linked): peak RSS 217.4 MB → 174.8 MB (−42.6 MB, ~890 B per unlinked code block). For a running program the same bytes come back whenever aCodeBlockis collected while itsUnlinkedCodeBlocksurvives (aging window,CodeCache, or cache-generated executables).JSTests/stress(default run + disk-cache run, 5,259 tests): identical pass/fail tomain.