react-native-worklets-core icon indicating copy to clipboard operation
react-native-worklets-core copied to clipboard

Published 20 hours ago • verified publisher icon margelo

Added disposing/release mechanism to the system

Open chrfalch opened this issue 1 year ago • 7 comments

To make sure we free any underlying values due to the dreaded hermes GC issue where internally allocated memory is not calcualated when considering garbage collection we need to implement this ourselves.

A shared value is now disposed and all resources allocated should be released. This is done through wrappers.

When deleting argument wrappers we also release any resources.

Fixes #129

chrfalch avatar Oct 28 '23 10:10 chrfalch

Wait, how does this work? Do we need to manually call close/dispose now when we no longer need a value? How do we know when we no longer need a value, is there now a ref counting mechanism here?

mrousavy avatar Oct 29 '23 17:10 mrousavy

Wait, how does this work? Do we need to manually call close/dispose now when we no longer need a value? How do we know when we no longer need a value, is there now a ref counting mechanism here?

No need for users to call dispose - but f.ex. in the useSharedValue hook we can do a dispose when we unmount - freeing up untracked (for Hermes) memory. Also after passing arguments through worklets we now dispose these JSI values that are no longer needed so that the memory pressure that Hermes uses for calculating gc is more correct.

chrfalch avatar Oct 30 '23 08:10 chrfalch

Also after passing arguments through worklets we now dispose these JSI values that are no longer needed so that the memory pressure that Hermes uses for calculating gc is more correct

How does that happen?

mrousavy avatar Oct 31 '23 12:10 mrousavy

Hey @chrfalch / @mrousavy any updates on this fix?

I believe I'm dealing with a similar memory leak issue as https://github.com/mrousavy/react-native-vision-camera/issues/1953 / https://github.com/margelo/react-native-worklets-core/issues/129.

Much appreciated, thanks!

mskalra avatar Dec 14 '23 08:12 mskalra

Hey @chrfalch -- quick FYI I monkey patched these changes into our build and it worked great. Memory leak is no longer an issue on iOS (have not tested Android).

We are officially unblocked with this patch, but would love to get this fix into main for others. Lmk how I can help, thanks!

mskalra avatar Dec 18 '23 15:12 mskalra

How is this PR related to https://github.com/facebook/hermes/commit/aae2c4260781178d7b2ca169811b3bfca9f924d2? Is this PR going to be close in favor of the usage of the new method introduced there?

bglgwyng avatar Dec 30 '23 10:12 bglgwyng

Yes it is definitely related. If it will replace it or be used in addition is not yet clear.

chrfalch avatar Dec 30 '23 15:12 chrfalch