Potential memory leak with arrays in shared values #2824
Labels
bug-bash-jan22
Issues visited during Bug Bash Jan 2022
Needs review
Issue is ready to be reviewed by a maintainer
Platform: iOS
This issue is specific to iOS
Repro provided
A reproduction with a snippet of code, snack or repo is provided
Comments
github-actions
bot
added
Platform: iOS
|
I'm also seeing this in iOS from several users:
|
3 tasks
|
Seeing this quite frequently from multiple users:
|
|
Seeing this issue on iOS. Using Reanimated with arrays is a pretty standard use case and makes it unusable for me. What is the status on this? |
|
Can confirm we're seeing important memory leaks on arrays in shared values as well |
4 tasks
3 tasks
|
Any updates here? The library is pretty unusable in this state. |
|
I've concluded it's happening with all versions. I'm having to resort to state which is just about okay, but really needs looking into. |
|
Temporary fix: #3304 (comment) |
|
We started investigating this issue and after some testing it seems like there is something wacky happening on the JS VM garbage collector side. Manually triggering garbage collector on both UI and RN JS VMs results in all the objects being cleared up. We'd normally expect the GC to kick in at some point and dealloc all those hanging objects, however it seems like that's not the case and in our testing we cal also see that even after significant time we still don't get these objects cleaned up property. We will continue this investigation and share more updates as we discover more details about this issue. |
|
@kmagiera Do you have any extra info for how you're triggering the garbage collecting so we can work with that in the meantime? |
|
I'm not sure if there exists a way with JSC to trigger garbage collector programmatically. We use a connected debugger that has a button for starting a GC. If you use hermes you can do |
|
Here is the patch that makes it possible to call hermes' GC from the UI thread: diff --git a/Common/cpp/Tools/RuntimeDecorator.cpp b/Common/cpp/Tools/RuntimeDecorator.cpp
index fbc35f754..9fd7c88d4 100644
--- a/Common/cpp/Tools/RuntimeDecorator.cpp
+++ b/Common/cpp/Tools/RuntimeDecorator.cpp
@@ -31,6 +31,7 @@ void RuntimeDecorator::decorateRuntime(
rt, "_LABEL", jsi::String::createFromAscii(rt, label));
jsi::Object dummyGlobal(rt);
+ dummyGlobal.setProperty(rt, "gc", rt.global().getProperty(rt, "gc"));
rt.global().setProperty(rt, "global", dummyGlobal);
rt.global().setProperty(rt, "jsThis", jsi::Value::undefined());
|
|
Then you need to trigger gc at some moment both on the main JS and UI threads, for example with this method: import { runOnUI } from 'react-native-reanimated';
function triggerGC() {
global.gc();
runOnUI(() => {
'worklet';
global.gc();
})();
} |
|
@kmagiera glad to hear that some progress is being made. From my end, I tried to grab the nightly release and trigger this garbage collection function but unfortunately, I'm not seeing any memory get cleared up after doing so: Simulator.Screen.Recording.-.iPhone.13.-.2022-09-20.at.11.20.30.mp4I'm guessing that there may still be some pointer references at the time of triggering this function that is preventing the old values from being cleared up. |
|
hey @VinceAbruzzese – the patch I posted hasn't been merged to the library so pulling latest version won't make the gc function available on the UI thread. |
|
also it seem that tapping GC button more than once (specifically three times seem to give the best outcome) gives the best results. |
|
@kmagiera Is there a branch I can follow, please? |
piaskowyk
pushed a commit
that referenced
this issue
Nov 29, 2022
## Description This PR contains a new implementation of the "shareable value" concept – one of the lowest level key concepts behind shared values/worklets etc. There were many reasons for attempting this rewrite, but the main objective was to address memory-related issues rooted in that core part and difficult to fix due to an inherent complexity of that bit of the codebase. Below, I list a couple of reasons providing some more details description: 1. The aforementioned memory issues can be best noted on #2824 – the root cause of that and similar issue lies in the way we reference objects between the two runtimes. Due to the fact we use JSI's host objects and direct references it turns out that a lot of objects can outlast garbage collection because, for example they are referenced on the secondary runtime where garbage collection didn't run yet. Moreover, JSI objects have their own, simplified version of GC that has to run as well. As a result, for some objects to be cleaned up properly, we needed to run GC on the main JS runtime, then on the UI runtime and once again on the main JS runtime. 2. Secondary objective for the rewrite was to address the maintainability of the codebase part being rewritten. After investigating the issue mentioned above we concluded that it is too difficult and risky to try to fix it given the architectural choices we made in the past. 3. As a side-effect of the things being rewritten we expect the library to perform better. When tracking performance issues in the past, we detected that a lot of issues are due to an excessive communication over JSI. JSI is a lot quicker compared to calling things over "the bridge", however it still incurs some performance penalty that is significant enough on lower level devices even at a level of couple hundred calls. This issue has became apparent to us at some point, but the existing architecture wouldn't allow us to address it in a easy way. As an example we can bring up the issue with worklet functions that, before this rewrite, were represented as JSI's HostFunctions. As a result, calling worklet from on the UI thread would require a JSI roundtrip, and that has became problematic as we expanded the codebase and had one more logic extracted out into smaller worklets. This rewrite has been architected to optimize the number of cross-JSI calls. 4. The new core makes it possible to implement some new and often requested functionalities. Specifically, the ability to make complex shareable objects, that is, to be able to append to a shared array or add/remove entries from a shared map as opposed to always having to copy the whole new object with the modifications applied. Adding this also helps to improve some bits internally, specifically the case of shared styles, where we have a single animated style object assigned to multiple views. Before, we'd use an immutable array that'd represent the list of so-called view descriptors. With this rewrite we can add and remove entries without a need to copy the whole descriptors array as we mount new views. The description of this PR is also a work in progress and will update sections of this description as the work progresses. ## Changes This PR changes a lot of things under the hood. The main change lies in a way we create and reference shareable values. As part of this change the whole "notion" of shareable values is removed from the core and implemented on top of a different abstraction. We now allow for "shareable" data to provide an initializer function which is called on the UI runtime in order to instantiate data that then will be accessed by worklets. In addition, we avoid keeping JSI object cache on c++ side, this prevents issues with them being referenced for longer than necessary and hence was a root cause of memory related issues (leaks -- but these weren't actual leaks as triggering GC several times would result in a complete cleanup).
Trying this on iOS and it seems to help but doesn't clean up fully. But on Android I get instant crash: |
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this issue
Jun 5, 2023
## Description This PR contains a new implementation of the "shareable value" concept – one of the lowest level key concepts behind shared values/worklets etc. There were many reasons for attempting this rewrite, but the main objective was to address memory-related issues rooted in that core part and difficult to fix due to an inherent complexity of that bit of the codebase. Below, I list a couple of reasons providing some more details description: 1. The aforementioned memory issues can be best noted on software-mansion#2824 – the root cause of that and similar issue lies in the way we reference objects between the two runtimes. Due to the fact we use JSI's host objects and direct references it turns out that a lot of objects can outlast garbage collection because, for example they are referenced on the secondary runtime where garbage collection didn't run yet. Moreover, JSI objects have their own, simplified version of GC that has to run as well. As a result, for some objects to be cleaned up properly, we needed to run GC on the main JS runtime, then on the UI runtime and once again on the main JS runtime. 2. Secondary objective for the rewrite was to address the maintainability of the codebase part being rewritten. After investigating the issue mentioned above we concluded that it is too difficult and risky to try to fix it given the architectural choices we made in the past. 3. As a side-effect of the things being rewritten we expect the library to perform better. When tracking performance issues in the past, we detected that a lot of issues are due to an excessive communication over JSI. JSI is a lot quicker compared to calling things over "the bridge", however it still incurs some performance penalty that is significant enough on lower level devices even at a level of couple hundred calls. This issue has became apparent to us at some point, but the existing architecture wouldn't allow us to address it in a easy way. As an example we can bring up the issue with worklet functions that, before this rewrite, were represented as JSI's HostFunctions. As a result, calling worklet from on the UI thread would require a JSI roundtrip, and that has became problematic as we expanded the codebase and had one more logic extracted out into smaller worklets. This rewrite has been architected to optimize the number of cross-JSI calls. 4. The new core makes it possible to implement some new and often requested functionalities. Specifically, the ability to make complex shareable objects, that is, to be able to append to a shared array or add/remove entries from a shared map as opposed to always having to copy the whole new object with the modifications applied. Adding this also helps to improve some bits internally, specifically the case of shared styles, where we have a single animated style object assigned to multiple views. Before, we'd use an immutable array that'd represent the list of so-called view descriptors. With this rewrite we can add and remove entries without a need to copy the whole descriptors array as we mount new views. The description of this PR is also a work in progress and will update sections of this description as the work progresses. ## Changes This PR changes a lot of things under the hood. The main change lies in a way we create and reference shareable values. As part of this change the whole "notion" of shareable values is removed from the core and implemented on top of a different abstraction. We now allow for "shareable" data to provide an initializer function which is called on the UI runtime in order to instantiate data that then will be accessed by worklets. In addition, we avoid keeping JSI object cache on c++ side, this prevents issues with them being referenced for longer than necessary and hence was a root cause of memory related issues (leaks -- but these weren't actual leaks as triggering GC several times would result in a complete cleanup).
|
Fixed with #3722 - The fix available in Reanimated 3 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
I'm noticing a spike in memory usage over time when updating a shared value in
react-native-reanimated-2. I suspect that it may be an issue in C++ when deleting array pointer references.Expected behavior
Memory usage should stay the same over time. I left this code running for 10 minutes and checked the memory usage through xcode and it was much higher.
Shareable value pointers should be cleared up when re-assigning the value but instead we are seeing 1131065 shared value pointer references:
Actual behavior & steps to reproduce
Create a react-native-reanimated-2 playground that reassigns a shared value to an object with an array and set an interval to continuously update it
Snack or minimal code example
Package versions
Affected platforms
The text was updated successfully, but these errors were encountered: