diff --git a/packages/react-native-nitro-modules/cpp/jsi/JSICache.cpp b/packages/react-native-nitro-modules/cpp/jsi/JSICache.cpp index 45739e3e8..068691a44 100644 --- a/packages/react-native-nitro-modules/cpp/jsi/JSICache.cpp +++ b/packages/react-native-nitro-modules/cpp/jsi/JSICache.cpp @@ -27,12 +27,14 @@ JSICache::~JSICache() { Logger::log(LogLevel::Info, TAG, "Destroying JSICache..."); std::unique_lock lock(_mutex); - destroyReferences(_valueCache); - destroyReferences(_objectCache); - destroyReferences(_functionCache); - destroyReferences(_weakObjectCache); - destroyReferences(_propNameIDCache); - destroyReferences(_arrayBufferCache); + // `.references()` - the caches self-compact dead slots as they grow (see `JSICache::WeakCache`), so this only + // ever walks slots that were still live at the last compaction. + destroyReferences(_valueCache.references()); + destroyReferences(_objectCache.references()); + destroyReferences(_functionCache.references()); + destroyReferences(_weakObjectCache.references()); + destroyReferences(_propNameIDCache.references()); + destroyReferences(_arrayBufferCache.references()); } JSICacheReference JSICache::getOrCreateCache(jsi::Runtime& runtime) { diff --git a/packages/react-native-nitro-modules/cpp/jsi/JSICache.hpp b/packages/react-native-nitro-modules/cpp/jsi/JSICache.hpp index 75785796a..034e99f5a 100644 --- a/packages/react-native-nitro-modules/cpp/jsi/JSICache.hpp +++ b/packages/react-native-nitro-modules/cpp/jsi/JSICache.hpp @@ -10,6 +10,8 @@ #include "BorrowingReference.hpp" #include "NitroLogger.hpp" #include "WeakReference.hpp" +#include +#include #include #include #include @@ -57,14 +59,63 @@ class JSICache final : public jsi::NativeState { private: friend class JSICacheReference; +public: + /** + * A list of weakly-held cache slots that drops dead slots as it grows. + * + * The cache exists so that `~JSICache` can force-destroy every value it handed out when the Runtime goes away. + * Slots were previously only ever appended, and freed as a whole in `~JSICache` - so a caller that converts a + * HybridObject to JS at a high rate grew these lists without bound for the lifetime of the Runtime, long after + * the values themselves had been collected. + * + * `compact()` only ever erases slots whose value is definitively deleted (`isDeleted()`), so it can never drop + * a slot that `~JSICache` still needs to destroy - it just removes the bookkeeping for values that no longer + * exist. The probe deliberately does NOT use `lock()`, which could resurrect a value whose final release is + * mid-flight on another Thread (see `WeakReference::isDeleted`); reading the atomic flag is one-sided-safe, so + * a racing release merely keeps the slot until the next compaction. + * + * The `_compactAt` watermark keeps this amortized O(1) per push: after compacting we only try again once the + * list has doubled, so a cache made mostly of LONG-LIVED values (where compaction reclaims nothing) does not + * re-scan on every insert. + */ + template + class WeakCache final { + public: + void push(WeakReference&& reference) { + if (_references.size() >= _compactAt) [[unlikely]] { + compact(); + } + _references.push_back(std::move(reference)); + } + + [[nodiscard]] + const std::vector>& references() const { + return _references; + } + + private: + void compact() { + _references.erase( + std::remove_if(_references.begin(), _references.end(), [](const WeakReference& reference) { return reference.isDeleted(); }), + _references.end()); + _compactAt = std::max(kMinCompactSize, _references.size() * 2); + } + + private: + static inline constexpr size_t kMinCompactSize = 64; + + std::vector> _references; + size_t _compactAt{kMinCompactSize}; + }; + private: std::mutex _mutex; - std::vector> _valueCache; - std::vector> _objectCache; - std::vector> _functionCache; - std::vector> _weakObjectCache; - std::vector> _propNameIDCache; - std::vector> _arrayBufferCache; + WeakCache _valueCache; + WeakCache _objectCache; + WeakCache _functionCache; + WeakCache _weakObjectCache; + WeakCache _propNameIDCache; + WeakCache _arrayBufferCache; private: static inline std::unordered_map> _globalCache; @@ -86,32 +137,32 @@ class JSICacheReference final { public: BorrowingReference makeShared(jsi::Value&& value) { BorrowingReference owning(new jsi::Value(std::move(value))); - _strongCache->_valueCache.push_back(owning.weak()); + _strongCache->_valueCache.push(owning.weak()); return owning; } BorrowingReference makeShared(jsi::Object&& value) { BorrowingReference owning(new jsi::Object(std::move(value))); - _strongCache->_objectCache.push_back(owning.weak()); + _strongCache->_objectCache.push(owning.weak()); return owning; } BorrowingReference makeShared(jsi::Function&& value) { BorrowingReference owning(new jsi::Function(std::move(value))); - _strongCache->_functionCache.push_back(owning.weak()); + _strongCache->_functionCache.push(owning.weak()); return owning; } BorrowingReference makeShared(jsi::WeakObject&& value) { BorrowingReference owning(new jsi::WeakObject(std::move(value))); - _strongCache->_weakObjectCache.push_back(owning.weak()); + _strongCache->_weakObjectCache.push(owning.weak()); return owning; } BorrowingReference makeShared(jsi::PropNameID&& value) { BorrowingReference owning(new jsi::PropNameID(std::move(value))); - _strongCache->_propNameIDCache.push_back(owning.weak()); + _strongCache->_propNameIDCache.push(owning.weak()); return owning; } BorrowingReference makeShared(jsi::ArrayBuffer&& value) { BorrowingReference owning(new jsi::ArrayBuffer(std::move(value))); - _strongCache->_arrayBufferCache.push_back(owning.weak()); + _strongCache->_arrayBufferCache.push(owning.weak()); return owning; } diff --git a/packages/react-native-nitro-modules/cpp/utils/BorrowingReference.hpp b/packages/react-native-nitro-modules/cpp/utils/BorrowingReference.hpp index 2100469af..5f6a5985f 100644 --- a/packages/react-native-nitro-modules/cpp/utils/BorrowingReference.hpp +++ b/packages/react-native-nitro-modules/cpp/utils/BorrowingReference.hpp @@ -56,8 +56,8 @@ class BorrowingReference final { bool shouldDestroy = _state->decrementStrongRefCount(); if (shouldDestroy) { forceDestroyValue(); + releaseImplicitWeakRef(); } - maybeDestroyState(); } _value = ref._value; @@ -71,10 +71,10 @@ class BorrowingReference final { } private: - // WeakReference -> BorrowingReference Lock-constructor - explicit BorrowingReference(const WeakReference& ref) : _value(ref._value), _state(ref._state) { - _state->strongRefCount++; - } + // WeakReference -> BorrowingReference Lock-constructor. + // The caller (`WeakReference::lock()`) has already claimed the strong ref count via + // `tryIncrementStrongRefCount()`, so this must NOT increment it again. + explicit BorrowingReference(const WeakReference& ref) : _value(ref._value), _state(ref._state) {} private: // BorrowingReference -> BorrowingReference Cast-constructor @@ -97,8 +97,8 @@ class BorrowingReference final { bool shouldDestroy = _state->decrementStrongRefCount(); if (shouldDestroy) { forceDestroyValue(); + releaseImplicitWeakRef(); } - maybeDestroyState(); } public: @@ -192,12 +192,16 @@ class BorrowingReference final { } private: - void maybeDestroyState() { - if (_state->strongRefCount == 0 && _state->weakRefCount == 0) { - // free the full memory if there are no more references at all + /** + * Releases the strong cohort's implicit weak reference (see `ReferenceState`). Called exactly once per state, + * by whichever strong reference performed the final strong release - the value is already destroyed at this + * point. Frees the state if no `WeakReference` is left holding it either. + */ + void releaseImplicitWeakRef() { + if (_state->weakRefCount.fetch_sub(1) == 1) { delete _state; - _state = nullptr; } + _state = nullptr; } void forceDestroyValue() { diff --git a/packages/react-native-nitro-modules/cpp/utils/ReferenceState.hpp b/packages/react-native-nitro-modules/cpp/utils/ReferenceState.hpp index 7bc631694..1b3d6aeff 100644 --- a/packages/react-native-nitro-modules/cpp/utils/ReferenceState.hpp +++ b/packages/react-native-nitro-modules/cpp/utils/ReferenceState.hpp @@ -34,7 +34,34 @@ struct ReferenceState { return oldRefCount <= 1; } - explicit ReferenceState() : strongRefCount(1), weakRefCount(0), isDeleted(false) {} + /** + * Increments the strong ref count by one, but only if it is not already zero, and returns whether it did. + * + * A zero strong count means the final strong release is already under way, so the value is about to be + * destroyed even if `isDeleted` has not been set yet - `~BorrowingReference` decrements the count BEFORE + * calling `forceDestroyValue()`. Handing out a strong reference in that window would resurrect a dying value, + * and the resurrected reference would then run a second final release. This is `weak_ptr::lock()`'s + * increment-if-not-zero. + */ + inline bool tryIncrementStrongRefCount() { + size_t count = strongRefCount.load(); + while (count != 0) { + if (strongRefCount.compare_exchange_weak(count, count + 1)) { + return true; + } + } + return false; + } + + // `weakRefCount` starts at 1: the strong cohort collectively owns one implicit weak reference, 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. + // + // This is the same shape as `shared_ptr`'s control block, and it exists because the previous scheme ("delete + // the state when `strong == 0 && weak == 0`") read the two counters non-atomically as a unit: a last-strong + // releaser and a last-weak releaser running concurrently could BOTH observe (0, 0) and double-delete the + // state - or one could read a state the other had already freed. + explicit ReferenceState() : strongRefCount(1), weakRefCount(1), isDeleted(false) {} }; } // namespace margelo::nitro diff --git a/packages/react-native-nitro-modules/cpp/utils/WeakReference+Borrowing.hpp b/packages/react-native-nitro-modules/cpp/utils/WeakReference+Borrowing.hpp index a97d7be36..d4aab617b 100644 --- a/packages/react-native-nitro-modules/cpp/utils/WeakReference+Borrowing.hpp +++ b/packages/react-native-nitro-modules/cpp/utils/WeakReference+Borrowing.hpp @@ -26,6 +26,11 @@ BorrowingReference WeakReference::lock() const { // return nullptr return BorrowingReference(); } + if (!_state->tryIncrementStrongRefCount()) { + // The last strong reference is mid-release and this value is about to be destroyed - it just hasn't + // flagged `isDeleted` yet. Resurrecting it here would cause a second final release. + return BorrowingReference(); + } return BorrowingReference(*this); } diff --git a/packages/react-native-nitro-modules/cpp/utils/WeakReference.hpp b/packages/react-native-nitro-modules/cpp/utils/WeakReference.hpp index 22ca5c77f..d439d0381 100644 --- a/packages/react-native-nitro-modules/cpp/utils/WeakReference.hpp +++ b/packages/react-native-nitro-modules/cpp/utils/WeakReference.hpp @@ -49,8 +49,7 @@ class WeakReference final { return *this; if (_state != nullptr) { - _state->weakRefCount--; - maybeDestroy(); + releaseWeakRef(); } _value = ref._value; @@ -67,8 +66,7 @@ class WeakReference final { if (_state != nullptr) { // destroy previous pointer - _state->weakRefCount--; - maybeDestroy(); + releaseWeakRef(); } _value = ref._value; @@ -83,8 +81,7 @@ class WeakReference final { ~WeakReference() { if (_state != nullptr) { - _state->weakRefCount--; - maybeDestroy(); + releaseWeakRef(); } } @@ -94,21 +91,37 @@ class WeakReference final { [[nodiscard]] BorrowingReference lock() const; + /** + * Returns whether the referenced value has already been deleted. + * + * Unlike `lock()`, this never materializes a strong reference, so it is safe to call while the value's final + * release may be running concurrently on another Thread: `lock()` checks `isDeleted` and then increments the + * strong count, but `~BorrowingReference` decrements the count BEFORE setting `isDeleted` (and without holding + * the state mutex), so a `lock()` landing in that window resurrects the dying value - and the resurrected + * temporary's destructor then runs a second `forceDestroyValue()` concurrently with the releaser's. A plain + * read of the atomic flag cannot resurrect anything; during such a race it merely still reports the value as + * alive, which callers must treat as "maybe alive". + */ + [[nodiscard]] + bool isDeleted() const { + return _state == nullptr || _state->isDeleted.load(); + } + public: friend class BorrowingReference; private: - void maybeDestroy() { - if (_state->strongRefCount == 0 && _state->weakRefCount == 0) { - // free the full memory if there are no more references at all - if (!_state->isDeleted) [[unlikely]] { - std::string typeName = TypeInfo::getFriendlyTypename(true); - throw std::runtime_error("WeakReference<" + typeName + "> encountered a stale `_value` - BorrowingReference<" + typeName + - "> should've already deleted this!"); - } + /** + * Releases this weak reference's count on the state, freeing the state if it was the last reference overall. + * The strong cohort owns one implicit weak reference (see `ReferenceState`), so the count can only reach zero + * after the final strong release already destroyed the value, and `fetch_sub`'s return value alone decides + * who frees the state. + */ + void releaseWeakRef() { + if (_state->weakRefCount.fetch_sub(1) == 1) { delete _state; - _state = nullptr; } + _state = nullptr; } private: