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

Conversation

tjzel
Copy link
Collaborator

@tjzel tjzel commented Dec 23, 2024

Summary

This pull requests refactors memory management of Worklets and Reanimated.

Basically, since Reanimated can obtain WorkletsModuleProxy and the Worklet Runtimes as shared pointers, it has to release them explicitly during the invalidation stage of Native Modules. Releasing them later on (e.g. via deconstructors) might lead into issues and crashes.

Ideally we'd instead use some different solution here than shared pointers, but it can wait as it's not mandatory at the moment and could be a significant refactor.

Fixes:

  • iOS crash on reload
  • Android crash on SingleInstanceChecker during third reload

Test plan

  • 0.76 iOS/Android Fabric works
  • 0.76 iOS/Android Paper works
  • 0.75 iOS/Android Fabric works
  • 0.75 iOS/Android Paper works
  • 0.74 iOS/Android Fabric works
  • 0.74 iOS/Android Paper works

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?

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?

@tjzel tjzel marked this pull request as ready for review December 24, 2024 13:24
@tjzel tjzel added the Check compatibility Trigger a time-consuming compatibility check action label Dec 24, 2024
Copy link
Member

@tomekzaw tomekzaw left a comment

Choose a reason for hiding this comment

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

The changes look good to me, I left some remarks in the comments. Two more general questions:

  1. Why do we need a separate invalidate method? Why can't we just clear the pointers in C++ destructors like we used to do?
  2. Once invalidate method is called, shall we add some assertions to prevent us from calling any methods of an invalidated object?
  3. Why can't we just use weak pointers to store WorkletsModule in ReanimatedModule? This way there's no strong link between those two.

assertWithMessage(
instanceCount_ <= 1,
instanceCount_ < 1,
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?

Comment on lines 52 to 53
return makeCxxInstance(
jThis,
rnRuntime,
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?

Comment on lines +57 to +61
// 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];
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];

Comment on lines +72 to 76
// 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();
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();

explicit WorkletsModule(
jni::alias_ref<WorkletsModule::jhybridobject> jThis,
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 &.

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_;

explicit WorkletsModule(
jni::alias_ref<WorkletsModule::jhybridobject> jThis,
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?

Copy link
Member

@piaskowyk piaskowyk left a comment

Choose a reason for hiding this comment

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

Could you elaborate more on why performing cleanup in the C++ destructor is considered too late? This isn't entirely clear to me, but it seems to be important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Check compatibility Trigger a time-consuming compatibility check action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants