Hermes reference tracking #563

brocollie08 · 2024-12-18T20:33:44Z

No longer overloading the global JNI table, tracking references in C++ instead to avoid over-usage crash

Change Type (required)

Indicate the type of change your pull request is:

patch
minor
major
N/A

Does your PR have any documentation updates?

Updated docs
No Update needed
Unable to update docs

📦 Published PR as canary version: 0.10.1--canary.563.18922

Try this version out locally by upgrading relevant packages to 0.10.1--canary.563.18922

codecov · 2024-12-18T21:00:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.70%. Comparing base (94d37fc) to head (e65146c).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #563      +/-   ##
==========================================
- Coverage   89.70%   89.70%   -0.01%     
==========================================
  Files         331      331              
  Lines       19794    19794              
  Branches     1949     1949              
==========================================
- Hits        17757    17756       -1     
- Misses       2023     2024       +1     
  Partials       14       14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…racked

brocollie08 · 2024-12-20T21:48:42Z

/canary

sugarmanz · 2024-12-23T21:56:36Z

jvm/hermes/src/main/jni/JHermesRuntime.h

+        auto ref = newObjectCxxArgs();
+        return ref;


Nit: unnecessary local var (applies many places)

sugarmanz · 2024-12-23T21:57:27Z

jvm/hermes/src/main/jni/JHermesRuntime.h

+        for (auto& ptr : *runtimeScope_->valueScope) {
+            ptr.second.reset();
+        }
+        for (auto& ptr : *runtimeScope_->objectScope) {
+            ptr.second.reset();
+        }
+        for (auto& ptr : *runtimeScope_->functionScope) {
+            ptr.second.reset();
+        }
+        for (auto& ptr : *runtimeScope_->arrayScope) {
+            ptr.second.reset();
+        }
+        for (auto& ptr : *runtimeScope_->symbolScope) {
+            ptr.second.reset();


This should all be private implementation details of RuntimeScope::release.

Secondarily, this should also probably clear the keys too - as we discovered earlier.

sugarmanz · 2024-12-23T21:57:46Z

jvm/hermes/src/main/jni/JHermesRuntime.h

@@ -107,12 +106,12 @@ class JHermesRuntime : public HybridClass<JHermesRuntime, JJSIRuntime> {
 private:
    friend HybridBase;
    std::unique_ptr<HermesRuntime> runtime_;
-    global_ref<JHermesConfig::jhybridobject> jConfig_;
-    std::vector<global_ref<JHybridClass::jhybridobject>> scope_;
+    ::global_ref<JHermesConfig::jhybridobject> jConfig_;


sugarmanz · 2024-12-23T22:02:10Z

jvm/hermes/src/main/jni/JHermesRuntime.h

-        // TODO: From my current understanding, holding a global prevents the class from being
-        //       GC'd when it goes out of scope in Java land, which would prevent the JSI Value reference
-        //       from being reset. I originally wanted to keep a weak_ref, but was getting intermittent
-        //       segfaults from untracked (or incorrectly tracked) references that were then not being
-        //       released because the weak_ref was out of scope. The assumption there was that if the
-        //       weak_ref was out of scope, it was already released. But this doesn't seem to be true,
-        //       otherwise we wouldn't be getting segfaults. Making them global does prevent the segfaults,
-        //       reaffirming the above. Since all the wrapper classes are holding are pointers
-        //       this probably isn't a huge deal, but is certainly an inefficiency we should look
-        //       to remove. I'd like to just create weak_ptrs on the JSI values themselves,
-        //       which requires the holders to use shared_ptrs and expose a getter for accessing.
-        //       Then we'd also need a way to hold arbitrary weak_ptrs. We could also maybe try
-        //       to get weak refs to _just_ the CXX parts of the hybrid class to avoid leaking things
-        //       on the JVM side?
-        // TODO: Maybe we could do a periodic pruning of the vector to remove obsolete refs?


Beautiful - removing longstanding paragraphs of TODOs are always great to see.

sugarmanz · 2024-12-23T22:04:43Z

jvm/hermes/src/main/jni/JJSIValue.cpp

@@ -39,8 +39,7 @@ void JJSIPreparedJavaScript::registerNatives() {
 }


Can't put a comment on a diff that isn't modified - comment at the top of this file can now be removed as well:

player/jvm/hermes/src/main/jni/JJSIValue.cpp

Lines 6 to 15 in dc3c3ab

/**

* IMPORTANT: Our current scoping strategy is to ensure all JHybridClass constructs (JSI Value wrappers)

* are tracked as refs with the runtime that they're created in. This allows the runtime to explicitly

* release any lingering runtime pointer values before releasing the runtime itself. This prevents dangling

* pointers that could cause undefined behavior (likely segfault - or assert if in debug mode).

*

* Currently, this requires careful consideration for every newObjectCxxArgs call. If it creates a wrapper

* around a runtime PointerValue, and we don't have a guarantee it won't go out of scope, then we need to

* make sure it's tracked. TODO: Make it more foolproof, maybe inside the ctors for our wrappers

*/

sugarmanz · 2024-12-23T22:16:14Z

jvm/hermes/src/main/jni/JJSIValue.h

+    explicit JJSIValue(std::shared_ptr<RuntimeScope> scope, Value&& value) : HybridClass(), scope_(scope) {
+        // internally creates unique ptr
+        scope->trackValue(this, std::move(value));
+    }


If/when there is a singular API to track arbitrary runtime types, we should try to capture this as a common super constructor for JJSIValueHybridClass to enforce for any subclass (there are some new ones that may exist in the future).

sugarmanz · 2024-12-23T22:16:58Z

jvm/hermes/src/main/jni/JJSIValue.h

+    ~JJSIValue() override {
+        release();
+    }


Is this still necessary?

sugarmanz · 2024-12-23T22:18:16Z

jvm/hermes/src/main/jni/JJSIValue.h

+        if (!tracked && value_) value_.reset();
+        if (scope_) scope_->clearRef(this);


Ah, I get why this is necessary - but maybe the !tracked && value_ case could just make it's own RuntimeScope so that all the APIs still go through a scope and you don't need explicit logic to handle both cases.

sugarmanz · 2024-12-23T22:20:42Z

jvm/hermes/src/main/jni/JJSIValue.h

+            if (auto ref = scope_->getValue((void *)this)) {
+                return *ref;
+            } else {
+                throw std::runtime_error("Value reference cleaned up and is no longer in RuntimeScope");


This should probably just go through throwNativeHandleReleasedException("Value"); from below

If you don't go through the helper function, you need to tell the JNI to expect an exception, like throwNativeHandleReleasedException does (and should also probably wrap in a PlayerException - like throwNativeHandleReleasedException does)

sugarmanz · 2024-12-23T22:21:37Z

jvm/hermes/src/main/jni/RuntimeScope.cpp

+    if (it != valueScope->end()) {
+        return valueScope->at(ptr).get();
+    }
+    throw std::runtime_error("Value was not found in the RuntimeScope");


Why throw here? The return type is nullable, it should just return nullptr.

Otherwise, change return type to Value& to be more accurate (although, my preference would be keep nullable return type to match standard map APIs and let the caller handle nullptr explicitly.

this would apply to all the pointers, right?

sugarmanz and others added 11 commits December 5, 2024 13:56

explicit jhybrid class deconstructors

b64145d

move ownership of values to runtime

0f567a9

fix isReleased

f4ed9c3

runtime approach

a4743e0

referenceTracker

9f1be9c

rename to RuntimeScope

2e94b7c

.

0015ae8

somewhat working

11ee70e

debugging + trackRef

dc9a7df

sort of working again

7a36a7c

ok

29e8e6c

brocollie08 added the patch Increment the patch version when merged label Dec 18, 2024

brocollie08 added 2 commits December 18, 2024 16:10

do not include jni by default

865c498

remove all the debug logs

a19f15d

brocollie08 changed the title ~~Explicit deconstructors~~ Hermes reference tracking Dec 20, 2024

brocollie08 added 2 commits December 20, 2024 16:23

working without passing runtime for undefined/null

6c9cf9d

reverted null/undefined now that runtime isn't needed and no longer t…

ce1d647

…racked

brocollie08 marked this pull request as ready for review December 20, 2024 21:45

brocollie08 requested a review from sugarmanz as a code owner December 20, 2024 21:45

sugarmanz reviewed Dec 23, 2024

View reviewed changes

brocollie08 added 6 commits December 23, 2024 17:47

address some PR comments

5808d9f

addressed error handling and null ptr

e4b19ea

revert primitive tracking back

a21a795

clear the scope maps after release

93b714a

filter tracking in scoped Value constructor too

ab68c1a

object deconstructor

e65146c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hermes reference tracking #563

Hermes reference tracking #563

brocollie08 commented Dec 18, 2024 •

edited by intuit-svc

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading

brocollie08 commented Dec 20, 2024

sugarmanz Dec 23, 2024

sugarmanz Dec 23, 2024

sugarmanz Dec 23, 2024

sugarmanz Dec 23, 2024

sugarmanz Dec 23, 2024

sugarmanz Dec 23, 2024

sugarmanz Dec 23, 2024

sugarmanz Dec 23, 2024

sugarmanz Dec 23, 2024

sugarmanz Dec 23, 2024

sugarmanz Dec 23, 2024

brocollie08 Dec 23, 2024

		@@ -39,8 +39,7 @@ void JJSIPreparedJavaScript::registerNatives() {
		}

	/**
	* IMPORTANT: Our current scoping strategy is to ensure all JHybridClass constructs (JSI Value wrappers)
	* are tracked as refs with the runtime that they're created in. This allows the runtime to explicitly
	* release any lingering runtime pointer values before releasing the runtime itself. This prevents dangling
	* pointers that could cause undefined behavior (likely segfault - or assert if in debug mode).
	*
	* Currently, this requires careful consideration for every newObjectCxxArgs call. If it creates a wrapper
	* around a runtime PointerValue, and we don't have a guarantee it won't go out of scope, then we need to
	* make sure it's tracked. TODO: Make it more foolproof, maybe inside the ctors for our wrappers
	*/

		if (!tracked && value_) value_.reset();
		if (scope_) scope_->clearRef(this);

Hermes reference tracking #563

Are you sure you want to change the base?

Hermes reference tracking #563

Conversation

brocollie08 commented Dec 18, 2024 • edited by intuit-svc Loading

Change Type (required)

Does your PR have any documentation updates?

codecov bot commented Dec 18, 2024 • edited Loading

Codecov Report

brocollie08 commented Dec 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brocollie08 commented Dec 18, 2024 •

edited by intuit-svc

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading