fix: bound JSICache growth by compacting dead weak slots - #1469
Open
jslok wants to merge 3 commits into
Open
Conversation
…l-release lock() checked isDeleted and then unconditionally incremented the strong count, but ~BorrowingReference decrements the count BEFORE calling forceDestroyValue() and holds no mutex while doing so. A lock() landing in that window handed out a strong reference to a value whose final release was already in flight, and the resurrected reference's destructor then ran a second forceDestroyValue() concurrently with the releaser's. lock() now claims the strong count with an increment-if-not-zero compare-exchange loop, exactly like weak_ptr::lock(), and returns null if the count already reached zero. The private WeakReference -> BorrowingReference lock-constructor no longer increments, since lock() has already claimed the count. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both BorrowingReference::maybeDestroyState() and WeakReference::maybeDestroy() freed the state when strongRefCount == 0 && weakRefCount == 0. Those are two independent atomics read as a unit, so a last-strong releaser and a last-weak releaser running concurrently could BOTH observe (0, 0) - a double delete of the state - or one could read a counter out of a state the other had already freed. ReferenceState now uses shared_ptr's control-block scheme: weakRefCount starts at 1, representing one implicit weak reference collectively owned by the strong cohort, released by whichever strong reference performs the final strong release (after it destroyed the value). The state is freed by whoever brings weakRefCount to zero, decided by fetch_sub's return value alone - exactly one thread ever sees the 1 -> 0 transition, and weakRefCount cannot reach zero before the final strong release has completed. Depends on the lock() increment-if-not-zero fix: a lock() resurrecting a value mid final-release would run a second final strong release, releasing the implicit weak reference twice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every JSICacheReference::makeShared() pushed a weak slot into one of JSICache's six vectors, and those slots were only ever freed wholesale in ~JSICache. Nothing removed a slot when its value died, so a caller that converts values to JS at a high rate (e.g. a Frame Processor boxing HybridObjects per frame) grew the lists without bound for the lifetime of the Runtime - visible in heap profiles as steady native growth under JSICacheReference::makeShared. The lists are now a small WeakCache<T> that compacts as it grows: compact() erases only slots whose value is definitively deleted, probed via a new non-resurrecting WeakReference::isDeleted() (an atomic flag read - lock() could resurrect a value whose final release is mid-flight on another thread). A doubling watermark (min 64) keeps compaction amortized O(1) per push. ~JSICache walks .references() and is otherwise unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 3 of 3 - #1464 split into atomic PRs as requested. Stacked on #1467 and #1468, which must merge first (in that order) - this branch contains their commits, so only the last commit is new here. I'll rebase as they land. The compaction probe is only safe given both: it relies on the non-resurrecting
lock()semantics (#1467) and onReferenceStatesurviving concurrent releases (#1468).The leak
This is the memory leak we originally chased: steady, unreclaimable native-heap growth in a production React Native app, on a hot path that converts HybridObjects to JS every frame. Heap profiling attributed the growth to
JSICacheReference::makeShared- in our case the caches grew by roughly 2,700 permanently-retained slots per minute for the lifetime of the Runtime, long after the values themselves had been collected.The cause: every
makeShared(..)does_xCache.push_back(owning.weak()), and those sixstd::vector<WeakReference<T>>s are only ever freed wholesale in~JSICache. Nothing removes a slot when its value dies. Invisible for a handful of long-lived callbacks; unbounded for high-rate converters (Frame Processors being the flagship case).The fix
The lists are now a small
WeakCache<T>that compacts as it grows:compact()erases only slots whose value is definitively deleted, via a new non-resurrectingWeakReference::isDeleted()(an atomic flag read). It deliberately does not uselock():lock()materializes a strong reference, and doing that from the hotmakeSharedpath while a value's final release runs on another thread is exactly the race fix: preventWeakReference::lock()from resurrecting a value mid final-release #1467 closes.isDeleted()is one-sided-safe - a racing release may keep the slot until the next compaction, never the reverse - so~JSICachecan never lose a slot it still needs to force-destroy._compactAtwatermark (min 64) keeps compaction amortized O(1) per push; a cache made mostly of long-lived values, where compaction reclaims nothing, does not re-scan on every insert.~JSICachewalks.references()and is otherwise unchanged.Testing
Verified on-device (Android, release build) against the per-frame conversion path that surfaced the leak, with all Nitro-consuming modules rebuilt: the retained-slot count stops ratcheting and the native-heap growth attributed to
makeShareddisappears from heap profiles. Formatted to matchconfig/.clang-formatoutput in the surrounding code.🤖 Generated with Claude Code