-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
packages/react-native-reanimated/android/src/main/cpp/worklets/android/WorkletsModule.h
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/worklets/Tools/SingleInstanceChecker.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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:
- Why do we need a separate
invalidate
method? Why can't we just clear the pointers in C++ destructors like we used to do? - Once
invalidate
method is called, shall we add some assertions to prevent us from calling any methods of an invalidated object? - Why can't we just use weak pointers to store
WorkletsModule
inReanimatedModule
? This way there's no strong link between those two.
packages/react-native-reanimated/Common/cpp/worklets/Tools/SingleInstanceChecker.h
Outdated
Show resolved
Hide resolved
packages/react-native-reanimated/android/src/main/cpp/worklets/android/WorkletsModule.cpp
Show resolved
Hide resolved
packages/react-native-reanimated/apple/worklets/apple/WorkletsModule.mm
Outdated
Show resolved
Hide resolved
...ges/react-native-reanimated/android/src/main/java/com/swmansion/worklets/WorkletsModule.java
Outdated
Show resolved
Hide resolved
packages/react-native-reanimated/android/src/main/cpp/worklets/android/WorkletsModule.h
Show resolved
Hide resolved
packages/react-native-reanimated/android/src/main/cpp/worklets/android/WorkletsModule.h
Show resolved
Hide resolved
packages/react-native-reanimated/android/src/main/cpp/worklets/android/WorkletsModule.h
Show resolved
Hide resolved
There was a problem hiding this 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.
@piaskowyk Native Modules are invalidated and destroyed in stages. This is more or less how React Native gets cleaned up now (e.g. on reload):
Our Worklet Runtime can hold instances to various Host Objects, like Shadow Nodes. Therefore, if we destroy our runtime on step 3. it might crash on garbage collection because the C++ side of these host objects is already destroyed. This probably could be mitigated with a different kind of bindings, but it's definitely outside of the scope of this PR. |
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:
Test plan