feat: Trigger Java GC on reload

[mrousavy]

Summary:

While we (Margelo) were developing a new C++ 3D library for react-native, we noticed that Java often keeps a lot of dead instances in memory, making it hard to debug memory allocations (or actually de-allocations), especially since we use jsi::HostObject and jni::HybridClass in conjunction. Having two garbage-collected languages retain an object is a bit tricky, and making sure that we aren't doing anything wrong with our allocations and references was not easy - but manually calling System.gc() on app reloads helped us see that much better.

Before this, we needed to wait multiple minutes until some Java objects are actually freed from the GC. Our use-case was a facebook::jni::HybridClass, which was held strong in a facebook::jsi::HostObject (so again, two GC'd languages).

There should be no change in behaviour with this PR, just two things to note:

1. Memory might be free'd more eagerly in full reloads (dev builds) - makes sense for library developers, especially when working with C++ modules.
2. System.gc() only suggests garbage collection, it does not force it. But when it runs, it might impact performance, although we haven't noticed any impact of that at all. The garbage collector runs anyways - better during a reload than later when exceuting the app normally.

Changelog:

[ANDROID] [ADDED] - Trigger Java GC on app reload

Test Plan:

Open an app, create a Java module that holds a few objects, add finalize() methods to those objects and log their deletion.

Reload the app to see the logs, compare before vs after.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	18,910,437	-4,132
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	22,279,102	-4,132
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: ce811a0
Branch: main

[javache]

Seems reasonable, and should only affect development. I'm not 100% confident that the existing runtime will have been torn down by the time this method is called. Perhaps onJSBundleLoadedFromServer?

Can you expand the comment in-code with some of the information from this PR?

[mrousavy]

@javache what do you think of putting it inside ReactInstanceManager::tearDownReactContext?

[mrousavy]

Or I guess at the end of ReactInstanceManager::recreateReactContextInBackground - after that ran, this.mCurrentReactContext is also null, and it definitely looks like this is a better place for a "GC when reloading the app", as opposed to "run GC when tearing down the context", which sounds a bit more hacky. 😅

[mrousavy]

Okay I moved it to recreateReactContextInBackground and added a comment 👍

let me know if that makes sense

EDIT: hmm actually I don't think this makes sense - for bridge mode this doesn't get called afaik. I'll try to find a better place

[javache]

Yes, this will only get called in bridge mode, you'll need an equivalent point in ReactHostImpl for this.

[mrousavy]

I just moved it into DevSupportManagerBase.java::onReactInstanceDestroyed(...), what do you think about that?

For me this always hits after refreshing in debug. In release, this isn't used afaik

[mrousavy]

sorry for the bump here @javache - have you had a chance to look at this?

[react-native-bot]

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[react-native-bot]

This PR was closed because it has been stalled for 7 days with no activity.

[mrousavy]

not stale

[javache]

The semantics of this in the new architecture are still not great - we call this when the ReactContext is destroyed, not the ReactInstance - that's outside the scope of this diff though.

[facebook-github-bot]

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[javache]

Keep in mind that in production we have ReleaseDevSupportManager which overrides this method to be a no-op.

[mrousavy]

Keep in mind that in production we have ReleaseDevSupportManager which overrides this method to be a no-op.

That's good tho, no? We only want this in DEV

[mrousavy]

The semantics of this in the new architecture are still not great - we call this when the ReactContext is destroyed, not the ReactInstance - that's outside the scope of this diff though.

Not sure if I understand - is there anything I can change here?

[facebook-github-bot]

@javache merged this pull request in de3c1ee.

[react-native-bot]

This pull request was successfully merged by @mrousavy in de3c1ee

^{When will my fix make it into a release? | How to file a pick request?}

[javache]

The semantics of this in the new architecture are still not great - we call this when the ReactContext is destroyed, not the ReactInstance - that's outside the scope of this diff though.

Not sure if I understand - is there anything I can change here?

No, outside the scope of this PR. Just means that the GC here may be less effective in the new architecture as the full instance hasn't been destroyed yet.