Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: explicit invalidations for native and cpp #6850

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -844,4 +844,10 @@ void ReanimatedModuleProxy::unsubscribeFromKeyboardEvents(
unsubscribeFromKeyboardEventsFunction_(listenerId.asNumber());
}

void ReanimatedModuleProxy::invalidate() {
// Make sure to release WorkletsModuleProxy on invalidate to allow it
// to destroy its runtime during the invalidation stage.
workletsModuleProxy_.reset();
}

} // namespace reanimated
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ class ReanimatedModuleProxy : public ReanimatedModuleProxySpec {

~ReanimatedModuleProxy();

void invalidate();

jsi::Value registerEventHandler(
jsi::Runtime &rt,
const jsi::Value &worklet,
Expand Down Expand Up @@ -175,7 +177,7 @@ class ReanimatedModuleProxy : public ReanimatedModuleProxySpec {

const bool isBridgeless_;
const bool isReducedMotion_;
const std::shared_ptr<WorkletsModuleProxy> workletsModuleProxy_;
std::shared_ptr<WorkletsModuleProxy> workletsModuleProxy_;
const std::string valueUnpackerCode_;

std::unique_ptr<EventHandlerRegistry> eventHandlerRegistry_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,8 @@ SingleInstanceChecker<T>::SingleInstanceChecker() {
std::string className =
__cxxabiv1::__cxa_demangle(typeid(T).name(), nullptr, nullptr, &status);

// Only one instance should exist, but it is possible for two instances
// to co-exist during a reload.
assertWithMessage(
instanceCount_ <= 1,
instanceCount_ < 1,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When cleaning during invalidation stage, it shouldn't be possible for two instances to exist at the same time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just instanceCount_ == 0?

"[Reanimated] More than one instance of " + className +
" present. This may indicate a memory leak due to a retain cycle.");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,12 @@ protected HybridData getHybridData() {
return mHybridData;
}

private native void invalidateCpp();

public void invalidate() {
invalidateCpp();
}

public static NativeMethodsHolder createNativeMethodsHolder(
LayoutAnimations ignoredLayoutAnimations) {
return new NativeMethodsHolder() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,8 @@ void NativeProxy::registerNatives() {
makeNativeMethod(
"isAnyHandlerWaitingForEvent",
NativeProxy::isAnyHandlerWaitingForEvent),
makeNativeMethod("performOperations", NativeProxy::performOperations)});
makeNativeMethod("performOperations", NativeProxy::performOperations),
makeNativeMethod("invalidateCpp", NativeProxy::invalidateCpp)});
}

void NativeProxy::requestRender(
Expand Down Expand Up @@ -619,4 +620,10 @@ void NativeProxy::setupLayoutAnimations() {
});
}

void NativeProxy::invalidateCpp() {
workletsModuleProxy_.reset();
reanimatedModuleProxy_->invalidate();
reanimatedModuleProxy_.reset();
}

} // namespace reanimated
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,8 @@ class NativeProxy : public jni::HybridClass<NativeProxy> {
void commonInit(jni::alias_ref<facebook::react::JFabricUIManager::javaobject>
&fabricUIManager);
#endif // RCT_NEW_ARCH_ENABLED

void invalidateCpp();
};

} // namespace reanimated
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,13 @@ using namespace facebook;
using namespace react;

WorkletsModule::WorkletsModule(
jni::alias_ref<WorkletsModule::javaobject> jThis,
jsi::Runtime *rnRuntime,
const std::string &valueUnpackerCode,
jni::alias_ref<JavaMessageQueueThread::javaobject> messageQueueThread,
const std::shared_ptr<facebook::react::CallInvoker> &jsCallInvoker,
const std::shared_ptr<worklets::JSScheduler> &jsScheduler,
const std::shared_ptr<UIScheduler> &uiScheduler)
: javaPart_(jni::make_global(jThis)),
rnRuntime_(rnRuntime),
: rnRuntime_(rnRuntime),
workletsModuleProxy_(std::make_shared<WorkletsModuleProxy>(
*rnRuntime,
valueUnpackerCode,
Expand All @@ -38,7 +36,7 @@ WorkletsModule::WorkletsModule(
}

jni::local_ref<WorkletsModule::jhybriddata> WorkletsModule::initHybrid(
jni::alias_ref<jhybridobject> jThis,
jni::alias_ref<jhybridobject> /*jThis*/,
jlong jsContext,
const std::string &valueUnpackerCode,
jni::alias_ref<JavaMessageQueueThread::javaobject> messageQueueThread,
Expand All @@ -52,7 +50,6 @@ jni::local_ref<WorkletsModule::jhybriddata> WorkletsModule::initHybrid(
std::make_shared<worklets::JSScheduler>(*rnRuntime, jsCallInvoker);
auto uiScheduler = androidUIScheduler->cthis()->getUIScheduler();
return makeCxxInstance(
jThis,
rnRuntime,
Comment on lines 52 to 53
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks weird, do we really need WorkletsModule to be a Hybrid class if we don't use jThis here in makeCxxInstance?

valueUnpackerCode,
messageQueueThread,
Expand All @@ -61,8 +58,14 @@ jni::local_ref<WorkletsModule::jhybriddata> WorkletsModule::initHybrid(
uiScheduler);
}

void WorkletsModule::invalidateCpp() {
workletsModuleProxy_.reset();
}

void WorkletsModule::registerNatives() {
registerHybrid({makeNativeMethod("initHybrid", WorkletsModule::initHybrid)});
registerHybrid(
{makeNativeMethod("initHybrid", WorkletsModule::initHybrid),
makeNativeMethod("invalidateCpp", WorkletsModule::invalidateCpp)});
}

} // namespace worklets
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class WorkletsModule : public jni::HybridClass<WorkletsModule> {
"Lcom/swmansion/worklets/WorkletsModule;";

static jni::local_ref<jhybriddata> initHybrid(
jni::alias_ref<jhybridobject> jThis,
jni::alias_ref<jhybridobject> /*jThis*/,
jlong jsContext,
const std::string &valueUnpackerCode,
jni::alias_ref<JavaMessageQueueThread::javaobject> messageQueueThread,
Expand All @@ -46,19 +46,19 @@ class WorkletsModule : public jni::HybridClass<WorkletsModule> {
}

private:
friend HybridBase;
jni::global_ref<WorkletsModule::javaobject> javaPart_;
jsi::Runtime *rnRuntime_;
std::shared_ptr<WorkletsModuleProxy> workletsModuleProxy_;

explicit WorkletsModule(
jni::alias_ref<WorkletsModule::jhybridobject> jThis,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out we don't need to keep the reference to Java part of WorkletsModule in cpp.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it still have to be a HybridClass or can we convert it back to JavaClass or something?

jsi::Runtime *rnRuntime,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, let's pass the runtime as a reference instead of a pointer, i.e. jsi::Runtime &.

const std::string &valueUnpackerCode,
jni::alias_ref<JavaMessageQueueThread::javaobject> messageQueueThread,
const std::shared_ptr<facebook::react::CallInvoker> &jsCallInvoker,
const std::shared_ptr<worklets::JSScheduler> &jsScheduler,
const std::shared_ptr<UIScheduler> &uiScheduler);

void invalidateCpp();

friend HybridBase;
jsi::Runtime *rnRuntime_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, let's store runtime as a reference.

Suggested change
jsi::Runtime *rnRuntime_;
jsi::Runtime &rnRuntime_;

std::shared_ptr<WorkletsModuleProxy> workletsModuleProxy_;
};

} // namespace worklets
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ public void invalidate() {
}

if (mNativeProxy != null) {
mNativeProxy.invalidate();
mNativeProxy = null;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ public boolean installTurboModule(String valueUnpackerCode) {
}

public void invalidate() {
// We have to destroy extra runtimes when invalidate is called. If we clean
// it up later instead there's a chance the runtime will retain references
// to invalidated memory and will crash on its destruction.
invalidateCpp();
mAndroidUIScheduler.deactivate();
Comment on lines +72 to 76
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a newline here as well

Suggested change
// We have to destroy extra runtimes when invalidate is called. If we clean
// it up later instead there's a chance the runtime will retain references
// to invalidated memory and will crash on its destruction.
invalidateCpp();
mAndroidUIScheduler.deactivate();
// We have to destroy extra runtimes when invalidate is called. If we clean
// it up later instead there's a chance the runtime will retain references
// to invalidated memory and will crash on its destruction.
invalidateCpp();
mAndroidUIScheduler.deactivate();

}

private native void invalidateCpp();
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ protected HybridData getHybridData() {
return mHybridData;
}

private native void invalidateCpp();

public void invalidate() {
invalidateCpp();
}

public static NativeMethodsHolder createNativeMethodsHolder(LayoutAnimations layoutAnimations) {
WeakReference<LayoutAnimations> weakLayoutAnimations = new WeakReference<>(layoutAnimations);
return new NativeMethodsHolder() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,13 @@ @implementation WorkletsModule {
return @YES;
}

- (void)invalidate
{
// We have to destroy extra runtimes when invalidate is called. If we clean
// it up later instead there's a chance the runtime will retain references
// to invalidated memory and will crash on destruction.
workletsModuleProxy_.reset();
[super invalidate];
Comment on lines +57 to +61
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a newline here since [super invalidate]; is unrelated to the comment above.

Suggested change
// We have to destroy extra runtimes when invalidate is called. If we clean
// it up later instead there's a chance the runtime will retain references
// to invalidated memory and will crash on destruction.
workletsModuleProxy_.reset();
[super invalidate];
// We have to destroy extra runtimes when invalidate is called. If we clean
// it up later instead there's a chance the runtime will retain references
// to invalidated memory and will crash on destruction.
workletsModuleProxy_.reset();
[super invalidate];

}

@end
Loading