performance-boost and fix memory-issues: Get rid of Object.finalize()

[fab1an]

I opened this bug to separate it from the Fresco image-loading bug.

Here (#8711) you can find a lengthy explanation why Object.finalize() is bad, but the two main reasons, why RN should stop using it are:

1. The FinalizerQueue can fill-up and stop being processed, this can happen for many reasons, and it's possible that Object.finalize() is never called until the JVM exits. This also means that, every object that's referenced from an object whose finalizer isn't called, isn't garbage-collected. Like everything referenced from CatalystInstanceImpl because of this log-statement.

2. Finalizers slow everything down by a giant amount. Quote from Effective Java:

   Oh, and one more thing: there is a severe performance penalty for using finalizers. On my machine, the time to create and destroy a simple object is about 5.6 ns. Adding a finalizer increases the time to 2,400 ns. In other words, it is about 430 times slower to create and destroy objects with finalizers.

Usage of finalize() in RN.

I would write the PR myself, but I'm not sure how RN manages HybridData. I know it's tedious to release everything manually, but doing it will give RN an extreme performance-boost and lower memory-footprint. This will help everyone.

Finalizers were just a bad idea which should have never happened.

[ide]

As with all performance-related diffs we should profile the results. There's some chance that finalizers are cheap enough or behave differently on ART.

[fab1an]

Ok, but how would you profile this?

[ide]

Well you claimed there would be an extreme performance boost and reduced memory consumption - so show a before-and-after comparison of speed and memory use.

[fab1an]

• GC of objects having a finalizer takes at least twice as long as normal, because the GC needs to run at least two times to reclaim the object.
• GC of all (even non-finalizable) objects in the dependent tree are sharing these costs, so that's a 100% slowdown in the theoretical best-case.
• Since GC runs when space is needed, but finalizable objects are only reclaimed after running GC twice, in the real case GC probably has to run more than twice until the space is finally cleared.
• Generational GC can't do any optimizations since the finalizer can contain arbitrary code.
• Then you have to add locking overhead from the FinalizerQueue.
• Also any other thread in any other library or activity could do something in it's finalizer which blocks for, say, 100ms. This time is added as well.
• For correct behaviour you would need to synchronize access to the finalizers since they could be run concurrently and out of order. Again, more locking costs.
• When you add native code you get into worse territory. Your finalizers might not be executed at all, so your App crashes because it's out of native memory. This seems to happen with Fresco.

I actually don't know how to craft a correct reproducible GC profiling test-case, since this behaviour is not defined and relies on the manufacturers of the phones and their memory and heapsize settings.

I hope someone else can step in here.

The thing I know for sure is that in the last 10 years, every java expert anywhere advised against finalizers for many reasons.

But everybody has the right to discard well meant advice.

[fab1an]

@ide Here is a test-case: FinalizerTest.java

It profiles creation-time of 1000 simple objects (1 int field) with

1. no finalizer
2. a trivial finalizer that just calls super.finalize(), and
3. more complex finalizer that includes an if

Results:

Worst:
. no finalizer:    1742 µs
. trivial:         4560 µs
. complex:         4591 µs

Best:
. no finalizer:    1392 µs
. trivial:         3680 µs
. complex:         3797 µs

Keep in mind that this is just instantiation of objects, not the actual finalisation and GC time and complexity that's imposed later.

[st0ffern]

@fab1an sorry for my bad Java knowledge, but what would you change a finalize() function with?
Can you write a example solution for this

cc @andreicoman11 @mkonicek

[fab1an]

@Stoffern Good question.

1. Remove it from a production-build or better:

2. Remove it all together.

   Since there's no guarantee that finalizers are ever processed (in fact on my phone and that of others they aren't), you might not see this statement even though your app is leaking memory.

   How useful is a warning that's not guaranteed to be displayed?

   It must be possible to this warning elsewhere, for example in an ActivityLifecycleCallback, but I don't know enough of RN's structure yet.

[st0ffern]

@fab1an can you run a app in simulator:

- run preformance monitoring
- add all react native components into a view, and refresh it a couple of times.
- then do the same with react-native without finaliser() in the sourcecode?
- and report difference?

I know my app is leaking memory after 30-40 refreshs when loading 10 different components.

[fab1an]

@Stoffern That's not much use. The native resources that are disposed in the finalizers in HybridData.java and Countable.java need to be disposed elsewhere first. I don't know enough about RN's internals to do this.

[fab1an]

@Stoffern I've setup and compiled ReactNative, but unfortunately I can't afford putting more time into this right now.

I've already spent about 10 hours researching, reading the code, providing benchmarks, detailed bug-reports and citing sources for this. I'm not getting paid for doing this work ;).

[st0ffern]

@fab1an i dont get payed either 😄 And this issue is a core issue both in RN and Fresco lib, it will be quite a job for both of us to figure out how to fix this.
Altho i think if they can fix the problems with Fresco lib it will be quite good for RN allready here.

@andreicoman11 @mkonicek you guys know more of the core to how it works, would this be possible to fix?

[fab1an]

@ide I figured a way of profiling this, or at least the beginning of a way: Since ReadableMap for arguments are using finalizers, just make some module that passes an object into Javascript a lot of times.
Then instead serialize the object as JSON, pass it as a string and rebuild it on the JS-side without using Readable(Native)Map. That might give some insights.

[ide]

@fab1an That seems reasonable to me so we get a basic idea of whether there is a performance improvement (or unexpected regression) and roughly how big the improvement is.

[lacker]

I think the discussion here has converged on saying, if there is indeed a performance improvement, it would be valuable to have a pull request for it, we just need to profile to be sure. I'm going to close this issue since it no longer seems useful but if someone does indeed create a pull request improving performance that would be pretty nice here.

[fab1an]

@lacker There are two benchmarks in this bug-report. One here: #8780 (comment)

There other one linked from here: #10504

But it seems that just nobody wants to change this.

[lacker]

Is there a pull request somewhere that is improving performance? Maybe I missed it. If there is a pull request to improve performance then it is easier for me to drive that to a conclusion, vs having an issue where it's hard to see what should be concluded per se.

[kesha-antonov]

Hello guys!

What you decided?

I'm trying to find memory leaks in my app and #2 in heap dump is FinalizerReference

I've created 2 issues on this

#16600
#16590

[lhfiii]

This leak is killing me. Will this be fixed?

[Gark]

Hello, gentlemen.
Is there any progress with that?

I still have the same issue. During my application usage,FinalizerReference retained size grows all time.

How to deal with this?