-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
performance-boost and fix of memory-issues: Get rid of Object.finalize()
#1363
Comments
A huge +1 for this. Our app uses Fresco extensively, and I've been seeing a lot of |
While I see your point that having finalizers adds a marginal perf penalty I don't think that it's the major cause for the memory issues here. We're going to take another look at the perf penalty and the reason why you see so many FinalizerReferences - but I really don't think that's the cause for high memory usage. Both Facebook and Twitter are using Fresco - so if there would be any regression we'd see that in our graphs very-very early and it would effect us in a major way. |
@balazsbalazs Except in the case of animated images, right? |
@balazsbalazs And finalizers add more than a "marginal perf penalty". It's orders of magnitude. Just instantiation takes 3 times as long (test here: facebook/react-native#8780 (comment) ), if you add the multiple GC cycles finalizable objects need, you get much larger slowdowns ( 2400ns instead of 6ns here: http://www.informit.com/articles/article.aspx?p=1216151&seqNum=7 ). |
It's simple: Fresco is wasting precious resources by using an obsolete technique that is discouraged since years for not one, but about 20 different reasons. What you are doing is muddling your memory-management with that of every other library running in the same JVM. You are throwing away years of GC optimization as well. |
@fab1an: As with every engineering decision there's a tradeoff here:
The question is how do you evaluate this tradeoff: what is exactly the perf penalty and how much you can loose if you leak a bitmap. Example: On the other hand if you forget to close a CloseableReference - which can happen very easily, since deterministic memory management is very alien for Java - you can leak a a few MB of Android Shared memory easily. On low end phones memory is much more valuable than CPU cycles. Now here the real cost is the GC for the finalizers - and since Android is not Java we need to measure the penalty on both Dalvik and ART GC to understand what does that really mean to make that tradeoff. Getting rid of the finalizers introduces the risk of leaking huge amount of memory - and since this is an open source library and we can't control the callers + deterministic memory management is an unusual thing in Java this is a real risk. |
@fab1an : you've already done some great analysis on this, I'd be happy if you'd help us analyze the cost of the GC penalty for Finalizers on Dalvik and ART. (A bit deeper than there's a second pass GC and object references on the heap). |
Oh, and yes, you're right: we need to fix the animated images |
@balazsbalazs I know why you are using finalizers, I have read the code, that's why I already posted in facebook/react-native#8711: "Finalization might seem to be a good mechanism for making sure that resources do not go undisposed, [..] an alternate fallback mechanism, a "second chance" safeguard which will automagically save the day by disposing of the resources that they forgot. This is dead wrong."
As a last comment, I'm quoting Wikipedia (https://en.wikipedia.org/wiki/Finalizer#Problems) "Finalizers can cause a significant number of problems, and are thus strongly discouraged by a number of authorities.
I won't spend more time on profiling what essentially are vendor-dependent race-conditions. I consider it a waste of development time to reproduce mistakes that thousands of other have done and written about before. I'll probably just switch to Picasso from square for now, since I mostly have static images anyway. |
@balazsbalazs And if you look into why the queue stops being processed, there seem to be more people having that problem: https://stackoverflow.com/questions/37001181/android-finalizerdaemon-hanging-up I reported a bug with google, but got no answer so far: https://code.google.com/p/android/issues/detail?id=215906 |
This is also an issue on react-native-animatable with animations that loop: |
Any news on this? |
This seems to be a huge issue with Fresco. We just switched to Fresco from Picasso and now our memory heap looks like this: We weren't seeing anything like this before in terms of the number of finalizers. Our analytics are showing a lot of OOMs. I won't pretend to understand the issue anywhere near as well as fab1an, but I can verify that the issue does seem to be having a very negative impact. |
@brianhama Can you post a screenshot of the memory heap using picasso before the switch? That would be very informative and help this issue along. |
@brianhama that is a red herring: the vm will create a Relating to the finalizers issue: we've run an experiment over a couple of months where we provide a separate implementation of
In conclusion, we will be sticking with finalizers for now. |
@foghina Great that you got around to analyse this. Could you publish the data, as that's really interesting evidence about the JVM and ART! |
There's not much more data than "none of our metrics changed" :), and if there were, I doubt I could post it. Just to clarify, this is not to say that finalizers are good, but that alternative options such as PhantomReferences are not necessarily better. And we're not in a place where we can live without a solution for this. |
Here (facebook/react-native#8711) you can find a lengthy explanation why
Object.finalize()
is bad, but the two main reasons, why Fresco should stop using it are:The
FinalizerQueue
can fill-up and stop being processed, this can happen for many reasons, and it's possible thatObject.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-colllected. So even finalizers that log only can lead to memory issues.Finalizers slow everything down by a giant amount. Quote from effective java:
I know it's tedious to release everything manually, but doing it will give Fresco and depending projects an extreme performance-boost and lower memory-footprint. This will help everyone.
Finalizers were just a bad idea which should have never happened.
The text was updated successfully, but these errors were encountered: