From e66f5d546c522f20e3815f69930a3ca2580acaac Mon Sep 17 00:00:00 2001 From: chrfalch Date: Sat, 28 Oct 2023 12:15:35 +0200 Subject: [PATCH 1/3] Added disposing/release mechanism to the system To make sure we free any underlying values due to the dreaded hermes GC issue where internally allocated memory is not calcualated when considering garbage collection we need to implement this ourselves. A shared value is now disposed and all resources allocated should be released. This is done through wrappers. When deleting argument wrappers we also release any resources. Fixes #129 --- cpp/WKTJsiWorklet.h | 2 +- cpp/base/WKTJsiHostObject.cpp | 7 +++++++ cpp/base/WKTJsiHostObject.h | 11 +++++++++++ cpp/sharedvalues/WKTJsiSharedValue.h | 13 +++++++++++++ cpp/wrappers/WKTArgumentsWrapper.h | 6 ++++++ cpp/wrappers/WKTJsiArrayWrapper.h | 14 ++++++++++++++ cpp/wrappers/WKTJsiObjectWrapper.h | 17 +++++++++++++++++ cpp/wrappers/WKTJsiPromiseWrapper.h | 21 ++++++++++++++++++++- cpp/wrappers/WKTJsiWrapper.h | 10 +++++++--- src/types.ts | 1 + 10 files changed, 97 insertions(+), 5 deletions(-) diff --git a/cpp/WKTJsiWorklet.h b/cpp/WKTJsiWorklet.h index ea82623..fd7b076 100644 --- a/cpp/WKTJsiWorklet.h +++ b/cpp/WKTJsiWorklet.h @@ -301,7 +301,7 @@ class JsiWorklet : public JsiHostObject, .asString(runtime) .utf8(runtime); } - + // Double-check if the code property is valid. bool isCodeEmpty = std::all_of(_code.begin(), _code.end(), std::isspace); if (isCodeEmpty) { diff --git a/cpp/base/WKTJsiHostObject.cpp b/cpp/base/WKTJsiHostObject.cpp index 49fcf9b..4739fc4 100644 --- a/cpp/base/WKTJsiHostObject.cpp +++ b/cpp/base/WKTJsiHostObject.cpp @@ -31,6 +31,13 @@ JsiHostObject::~JsiHostObject() { #endif } +void JsiHostObject::dispose() { + if (!_disposed) { + dispose(_disposed); + _disposed = true; + } +} + void JsiHostObject::set(jsi::Runtime &rt, const jsi::PropNameID &name, const jsi::Value &value) { diff --git a/cpp/base/WKTJsiHostObject.h b/cpp/base/WKTJsiHostObject.h index bab6776..01350a1 100644 --- a/cpp/base/WKTJsiHostObject.h +++ b/cpp/base/WKTJsiHostObject.h @@ -143,6 +143,11 @@ class JsiHostObject : public jsi::HostObject { JsiHostObject(); ~JsiHostObject(); + /** + Disposes and releases all used resources + */ + void dispose(); + protected: /** Override to return map of name/functions @@ -194,7 +199,13 @@ class JsiHostObject : public jsi::HostObject { */ std::vector getPropertyNames(jsi::Runtime &runtime) override; + /** + Override to dispose + */ + virtual void dispose(bool disposed) {} + private: std::map> _hostFunctionCache; + std::atomic _disposed = {false}; }; } // namespace RNWorklet diff --git a/cpp/sharedvalues/WKTJsiSharedValue.h b/cpp/sharedvalues/WKTJsiSharedValue.h index 429878a..a7ce5c9 100644 --- a/cpp/sharedvalues/WKTJsiSharedValue.h +++ b/cpp/sharedvalues/WKTJsiSharedValue.h @@ -30,6 +30,11 @@ class JsiSharedValue : public JsiHostObject { */ ~JsiSharedValue() { _valueWrapper = nullptr; } + JSI_HOST_FUNCTION(dispose) { + JsiHostObject::dispose(); + return jsi::Value::undefined(); + } + JSI_HOST_FUNCTION(toString) { return jsi::String::createFromUtf8(runtime, _valueWrapper->toString(runtime)); @@ -100,6 +105,7 @@ class JsiSharedValue : public JsiHostObject { } JSI_EXPORT_FUNCTIONS(JSI_EXPORT_FUNC(JsiSharedValue, toString), + JSI_EXPORT_FUNC(JsiSharedValue, dispose), JSI_EXPORT_FUNC(JsiSharedValue, addListener)) JSI_EXPORT_PROPERTY_GETTERS(JSI_EXPORT_PROP_GET(JsiSharedValue, value)) @@ -122,6 +128,13 @@ class JsiSharedValue : public JsiHostObject { _valueWrapper->removeListener(listenerId); } +protected: + void dispose(bool disposed) override { + if (!disposed) { + _valueWrapper->release_wrapped_resources(); + } + } + private: std::shared_ptr _valueWrapper; std::shared_ptr _context; diff --git a/cpp/wrappers/WKTArgumentsWrapper.h b/cpp/wrappers/WKTArgumentsWrapper.h index caa2346..f77d667 100644 --- a/cpp/wrappers/WKTArgumentsWrapper.h +++ b/cpp/wrappers/WKTArgumentsWrapper.h @@ -20,6 +20,12 @@ class ArgumentsWrapper { } } + ~ArgumentsWrapper() { + for (size_t i = 0; i < _arguments.size(); i++) { + _arguments[i]->release_wrapped_resources(); + } + } + size_t getCount() const { return _count; } std::vector getArguments(jsi::Runtime &runtime) const { diff --git a/cpp/wrappers/WKTJsiArrayWrapper.h b/cpp/wrappers/WKTJsiArrayWrapper.h index aea3ee9..04255cc 100644 --- a/cpp/wrappers/WKTJsiArrayWrapper.h +++ b/cpp/wrappers/WKTJsiArrayWrapper.h @@ -32,6 +32,8 @@ class JsiArrayWrapper : public JsiHostObject, JsiWrapper *parent) : JsiWrapper(runtime, value, parent, JsiWrapperType::Array) {} + ~JsiArrayWrapper() { JsiHostObject::dispose(); } + JSI_HOST_FUNCTION(toStringImpl) { return jsi::String::createFromUtf8(runtime, toString(runtime)); } @@ -413,6 +415,18 @@ class JsiArrayWrapper : public JsiHostObject, const std::vector> &getArray() { return _array; } + /** + Overridden dispose - release our array resources! + */ + void release_wrapped_resources() override { + for (size_t i = 0; i < _array.size(); i++) { + _array[i]->release_wrapped_resources(); + } + } + +protected: + void dispose(bool disposed) override { release_wrapped_resources(); } + private: /** Creates a proxy for the host object so that we can make the runtime trust diff --git a/cpp/wrappers/WKTJsiObjectWrapper.h b/cpp/wrappers/WKTJsiObjectWrapper.h index 123fc3f..9b84137 100644 --- a/cpp/wrappers/WKTJsiObjectWrapper.h +++ b/cpp/wrappers/WKTJsiObjectWrapper.h @@ -30,6 +30,8 @@ class JsiObjectWrapper : public JsiHostObject, JsiWrapper *parent) : JsiWrapper(runtime, value, parent) {} + ~JsiObjectWrapper() { JsiHostObject::dispose(); } + JSI_HOST_FUNCTION(toStringImpl) { return jsi::String::createFromUtf8(runtime, toString(runtime)); } @@ -156,7 +158,22 @@ class JsiObjectWrapper : public JsiHostObject, } } + /** + Overridden dispose - release our array resources! + */ + void release_wrapped_resources() override { + for (auto it = _properties.begin(); it != _properties.end(); it++) { + it->second->release_wrapped_resources(); + } + } + protected: + void dispose(bool disposed) override { + if (!disposed) { + release_wrapped_resources(); + } + } + jsi::Value getAsProxyOrValue(jsi::Runtime &runtime) override { if (getType() == JsiWrapperType::Object) { return getObjectAsProxy(runtime, shared_from_this()); diff --git a/cpp/wrappers/WKTJsiPromiseWrapper.h b/cpp/wrappers/WKTJsiPromiseWrapper.h index 0bbca70..88f6205 100644 --- a/cpp/wrappers/WKTJsiPromiseWrapper.h +++ b/cpp/wrappers/WKTJsiPromiseWrapper.h @@ -78,7 +78,20 @@ class JsiPromiseWrapper explicit JsiPromiseWrapper(jsi::Runtime &runtime); - ~JsiPromiseWrapper() {} + ~JsiPromiseWrapper() { JsiHostObject::dispose(); } + + /** + Overridden dispose - release our array resources! + */ + void release_wrapped_resources() override { + if (_reason != nullptr) { + _reason->release_wrapped_resources(); + } + if (_value != nullptr) { + _value->release_wrapped_resources(); + } + } + /** Returns true if the object is a thenable object - ie. an object with a then function. Which is basically what a promise is. @@ -138,6 +151,12 @@ class JsiPromiseWrapper } protected: + void dispose(bool disposed) override { + if (!disposed) { + release_wrapped_resources(); + } + } + jsi::Value then(jsi::Runtime &runtime, const jsi::Value &thisValue, const jsi::Value *thenFn, const jsi::Value *catchFn); diff --git a/cpp/wrappers/WKTJsiWrapper.h b/cpp/wrappers/WKTJsiWrapper.h index 4abc4d0..9739a24 100644 --- a/cpp/wrappers/WKTJsiWrapper.h +++ b/cpp/wrappers/WKTJsiWrapper.h @@ -137,6 +137,11 @@ class JsiWrapper { */ void removeListener(size_t listenerId) { _listeners.erase(listenerId); } + /** + Override to ensure releasing resources correctly + */ + virtual void release_wrapped_resources() {} + protected: /** * Returns a wrapper for the value @@ -185,7 +190,6 @@ class JsiWrapper { const jsi::Value &thisValue, const jsi::Value *arguments, size_t count); -protected: /** * Sets the value from a JS value * @param runtime runtime for the value @@ -253,10 +257,10 @@ class JsiWrapper { * @param parent Parent wrapper */ explicit JsiWrapper(JsiWrapper *parent) : _parent(parent) { - _readWriteMutex = new std::mutex(); + _readWriteMutex = std::make_shared(); } - std::mutex *_readWriteMutex; + std::shared_ptr _readWriteMutex; JsiWrapper *_parent; JsiWrapperType _type; diff --git a/src/types.ts b/src/types.ts index 8a07077..e6c1348 100644 --- a/src/types.ts +++ b/src/types.ts @@ -2,6 +2,7 @@ export interface ISharedValue { get value(): T; set value(v: T); addListener(listener: () => void): () => void; + dispose: () => void; } export interface IWorklet { From 77f2e1754b3629af6eaf6fd907534a907c669990 Mon Sep 17 00:00:00 2001 From: chrfalch Date: Sat, 28 Oct 2023 13:10:44 +0200 Subject: [PATCH 2/3] Added dispose to shared values and worklets - Added calling dispose from useSharedValue - Ran clang / cpp lint --- cpp/WKTJsRuntimeFactory.h | 6 +++--- cpp/WKTJsiWorklet.h | 26 ++++++++++++++++++++++---- cpp/WKTJsiWorkletApi.h | 7 ++++--- cpp/wrappers/WKTJsiArrayWrapper.h | 7 ++++++- cpp/wrappers/WKTJsiObjectWrapper.h | 1 + src/hooks/useSharedValue.ts | 7 ++++++- 6 files changed, 42 insertions(+), 12 deletions(-) diff --git a/cpp/WKTJsRuntimeFactory.h b/cpp/WKTJsRuntimeFactory.h index 4f8cda8..0ae3fc8 100644 --- a/cpp/WKTJsRuntimeFactory.h +++ b/cpp/WKTJsRuntimeFactory.h @@ -11,10 +11,10 @@ // Hermes #include #elif __has_include() - // JSC - #include +// JSC +#include #else - #include +#include #endif namespace RNWorklet { diff --git a/cpp/WKTJsiWorklet.h b/cpp/WKTJsiWorklet.h index fd7b076..18f12a3 100644 --- a/cpp/WKTJsiWorklet.h +++ b/cpp/WKTJsiWorklet.h @@ -81,6 +81,10 @@ class JsiWorklet : public JsiHostObject, JsiWorklet(jsi::Runtime &runtime, std::shared_ptr func) { createWorklet(runtime, func); } + + ~JsiWorklet() { + JsiHostObject::dispose(); + } JSI_HOST_FUNCTION(isWorklet) { return isWorklet(); } @@ -233,6 +237,15 @@ class JsiWorklet : public JsiHostObject, } } +protected: + void dispose(bool disposed) override { + if (!disposed) { + if (_closureWrapper != nullptr) { + _closureWrapper->release_wrapped_resources(); + } + } + } + private: /** Installs the worklet function into the worklet runtime @@ -305,10 +318,15 @@ class JsiWorklet : public JsiHostObject, // Double-check if the code property is valid. bool isCodeEmpty = std::all_of(_code.begin(), _code.end(), std::isspace); if (isCodeEmpty) { - std::string error = "Failed to create Worklet, the provided code is empty. Tips:\n" - "* Is the babel plugin correctly installed?\n" - "* If you are using react-native-reanimated, make sure the react-native-reanimated plugin does not override the react-native-worklets-core/plugin.\n" - "* Make sure the JS Worklet contains a \"" + std::string(PropNameWorkletInitDataCode) + "\" property with the function's code."; + std::string error = + "Failed to create Worklet, the provided code is empty. Tips:\n" + "* Is the babel plugin correctly installed?\n" + "* If you are using react-native-reanimated, make sure the " + "react-native-reanimated plugin does not override the " + "react-native-worklets-core/plugin.\n" + "* Make sure the JS Worklet contains a \"" + + std::string(PropNameWorkletInitDataCode) + + "\" property with the function's code."; throw jsi::JSError(runtime, error); } diff --git a/cpp/WKTJsiWorkletApi.h b/cpp/WKTJsiWorkletApi.h index 5a2b058..9699e30 100644 --- a/cpp/WKTJsiWorkletApi.h +++ b/cpp/WKTJsiWorkletApi.h @@ -132,9 +132,10 @@ class JsiWorkletApi : public JsiHostObject { JSI_PROPERTY_GET(currentContext) { auto current = JsiWorkletContext::getCurrent(runtime); - if (!current) return jsi::Value::undefined(); - return jsi::Object::createFromHostObject( - runtime, current->shared_from_this()); + if (!current) + return jsi::Value::undefined(); + return jsi::Object::createFromHostObject(runtime, + current->shared_from_this()); } JSI_EXPORT_PROPERTY_GETTERS(JSI_EXPORT_PROP_GET(JsiWorkletApi, diff --git a/cpp/wrappers/WKTJsiArrayWrapper.h b/cpp/wrappers/WKTJsiArrayWrapper.h index 04255cc..f051bad 100644 --- a/cpp/wrappers/WKTJsiArrayWrapper.h +++ b/cpp/wrappers/WKTJsiArrayWrapper.h @@ -425,7 +425,12 @@ class JsiArrayWrapper : public JsiHostObject, } protected: - void dispose(bool disposed) override { release_wrapped_resources(); } + // Release resources when the owning JS engine calls dispose on this object + void dispose(bool disposed) override { + if (!disposed) { + release_wrapped_resources(); + } + } private: /** diff --git a/cpp/wrappers/WKTJsiObjectWrapper.h b/cpp/wrappers/WKTJsiObjectWrapper.h index 9b84137..0d5b88a 100644 --- a/cpp/wrappers/WKTJsiObjectWrapper.h +++ b/cpp/wrappers/WKTJsiObjectWrapper.h @@ -168,6 +168,7 @@ class JsiObjectWrapper : public JsiHostObject, } protected: + // Release resources when the owning JS engine calls dispose on this object void dispose(bool disposed) override { if (!disposed) { release_wrapped_resources(); diff --git a/src/hooks/useSharedValue.ts b/src/hooks/useSharedValue.ts index 90fef6e..7d5f8e0 100644 --- a/src/hooks/useSharedValue.ts +++ b/src/hooks/useSharedValue.ts @@ -1,4 +1,4 @@ -import { useRef } from "react"; +import { useEffect, useRef } from "react"; import type { ISharedValue } from "../types"; /** @@ -11,5 +11,10 @@ export function useSharedValue(initialValue: T): ISharedValue { if (ref.current == null) { ref.current = Worklets.createSharedValue(initialValue); } + // Free on unmount + useEffect(() => { + return ref.current?.dispose(); + }, []); + return ref.current; } From 36ad64fabd0f9eeef8765aebbb0c99a3bfba9d85 Mon Sep 17 00:00:00 2001 From: chrfalch Date: Sun, 14 Apr 2024 10:06:58 +0200 Subject: [PATCH 3/3] fix: Fix after codereview --- src/hooks/useSharedValue.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hooks/useSharedValue.ts b/src/hooks/useSharedValue.ts index 7d5f8e0..d91f79d 100644 --- a/src/hooks/useSharedValue.ts +++ b/src/hooks/useSharedValue.ts @@ -13,7 +13,7 @@ export function useSharedValue(initialValue: T): ISharedValue { } // Free on unmount useEffect(() => { - return ref.current?.dispose(); + return ref.current?.dispose; }, []); return ref.current;