diff --git a/packages/react-native-nitro-modules/cpp/jsi/JSICache.cpp b/packages/react-native-nitro-modules/cpp/jsi/JSICache.cpp index 45739e3e8..9a13ccc3c 100644 --- a/packages/react-native-nitro-modules/cpp/jsi/JSICache.cpp +++ b/packages/react-native-nitro-modules/cpp/jsi/JSICache.cpp @@ -27,12 +27,12 @@ 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); + 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..b6f0afd4e 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,57 @@ 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. + * + * Slots were previously only ever appended and freed as a whole in `~JSICache`, so a caller that converts + * values 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 erases slots whose value is definitively deleted (`isDeleted()` - deliberately NOT `lock()`, + * which materializes a strong reference), so it can never drop a slot `~JSICache` still needs to destroy. + * The doubling `_compactAt` watermark keeps this amortized O(1) per push, so a cache made mostly of long-lived + * values 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 +131,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..3a892100e 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,14 @@ 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..96df23598 100644 --- a/packages/react-native-nitro-modules/cpp/utils/ReferenceState.hpp +++ b/packages/react-native-nitro-modules/cpp/utils/ReferenceState.hpp @@ -34,7 +34,28 @@ struct ReferenceState { return oldRefCount <= 1; } - explicit ReferenceState() : strongRefCount(1), weakRefCount(0), isDeleted(false) {} + /** + * Increments the strong ref count by one - unless it is already zero, and returns whether it did. + * + * A zero strong count means the final strong release is already under way (`~BorrowingReference` decrements + * the count BEFORE calling `forceDestroyValue()`), so handing out a strong reference in that window would + * resurrect a dying value. 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 - the same + // shape as `shared_ptr`'s control block. + 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..ac9b3ec8f 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 - the value is about to be destroyed, it just hasn't + // flagged `isDeleted` yet. + 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..c5a7e63dc 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,31 @@ 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. During such a race it may still report a dying value + * as alive ("maybe alive"), never the reverse. + */ + [[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: