Big memory issues because of RN's and Fresco's usage of Object.finalize()

[fab1an]

I did some memory profiling in Android Studio and noticed that there are huge amounts of FinalizerReference queueing up in the heap, which aren't garbage collected.

Using Object.finalize() is a bad idea. Because of the way GC works, the queue can quickly fill up, which seems to be happen often in Android. Here is a discussion about this: http://stackoverflow.com/a/8355147/342947

• Finalizers are bad for GC and memory.
• Finalizers are also bad coding, because you should never rely on GC functionality in your code. It could change anytime on any phone or runtime and give you headaches.

RN uses Object.finalize() in a couple of places, 2 of which seem to be fine because they log only. The usage in Countable.java and HybridData.java are problems, and I really really hope this get's fixed before a release, because it will mess with memory in unpredictable ways.

I think this is also the reasons for the image-loading bugs mentioned here #8677 and ultimately in facebook/fresco#621.

Fresco uses finalizers even more than RN: search.

Here is a screenshot of my profile. This is reproducible and even after minutes of runtime and manually invoking GC this queue isn't cleared. As you can see, there's references to CloseableStaticBitmap from Fresco.

I'm 98% sure this is also the reason for the image memory issue mentioned in the FAQ

[fab1an]

More explanation on why finalizers really shouldn't be used: https://www.securecoding.cert.org/confluence/display/java/MET12-J.+Do+not+use+finalizers

Quotes, that I think apply here:

• Coding errors that result in memory leaks can cause objects to incorrectly remain reachable; consequently, their finalizers are never invoked.

= tiny memory leaks are quickly growing into larger ones.

• Garbage collection usually depends on memory availability and usage rather than on the scarcity of some other particular resource. Consequently, when memory is readily available, a scarce resource may be exhausted in spite of the presence of a finalizer that could release the scarce resource if it were executed.

= as long as enough heap space is available, native resources won't be cleared.

• It is a common myth that finalizers aid garbage collection. On the contrary, they increase garbage-collection time and introduce space overheads. Finalizers interfere with the operation of modern generational garbage collectors by extending the lifetimes of many objects. Incorrectly programmed finalizers could also attempt to finalize reachable objects, which is always counterproductive and can violate program invariants.
• [...] neither the invocation order nor the specific executing thread or threads for finalizers can be guaranteed or controlled.

[narychen]

I opened another issue on Fresco facebook/fresco#1351
Since we couldn't change the code of Fresco in ReactAndroid, should someone pay attention to it in Fresco team.

[fab1an]

Agreed but there also usages in RN that should be fixed.

Another relevant discussion: https://stackoverflow.com/questions/18042176/what-will-the-finalizer-thread-do-if-there-is-a-infinite-loop-or-deadlock-in-the

That is, finalization may occur in the garbage collector thread, in a separate thead, or even a separate thread pool.

A JVM is not permitted to simply abort executing a finalizer, and can only use a finite number of threads (threads are operating system resources, and operating systems don't support arbitrarily many threads). Non-terminating finalizers will therefore of necessity starve that thread pool, thereby inhibit collection of any finalizable objects, and cause a memory leak.

[charpeni]

@facebook-github-bot label Android

[balazsbalazs]

Fresco is not dependent on finalizers. Finalizers are used in Fresco as a safety measure - if Fresco is not abused finalizers will do nothing.
For the memory issue I'm actually not sure why the FinalizerReferences won't get collected by the manually triggered GC. My hypothesis is that there is enough free memory in which case the GC is not bothered by collecting those references. I'll double check if that's the case.

[balazsbalazs]

One additional question: are you using animated gifs or webps anywhere?

[fab1an]

There are 11 usages, that do things, like the native release code here GifFrame.

The immediate problems I see:

• Most of the code isn't wrapped in try {...} finally {super.finalize()}. Even the isClosed here could throw an exception.

• In a couple of files like AnimatedDrawableCachingBackendImpl.java and CloseableReference.java synchronized is used.

  Use of locks or other synchronization-based mechanisms within a finalizer can cause deadlock or starvation. This possibility arises because neither the invocation order nor the specific executing thread or threads for finalizers can be guaranteed or controlled.

But generally: Execution of an object's finalizer may be delayed for an arbitrarily long time after the object becomes unreachable.

Again, thank you for your work on Fresco and everything, but in my opinion (and that of any Java-expert you will ask) Fresco and RN need to get completely rid of the finalizers and explicitly release everything, if correct code is what's wanted.

About using finalizers as a "safety-measure": another quote from: http://programmers.stackexchange.com/a/288724/48128

There is one and only one reason for overriding Object.finalize() [...] To place error logging code in finalize() which notifies you if you ever forget to invoke close().

Finalization might seem to be a good mechanism for making sure that resources do not go undisposed, but most people see it in a completely wrong way: they think of it as 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.

Also getting rid of the methods seem will give performance benefits:

Objects with finalizers (those that have a non-trivial finalize() method) have significant overhead compared to objects without finalizers, and should be used sparingly. Finalizeable objects are both slower to allocate and slower to collect. [...] Combine that with the fact that finalizers are not guaranteed to run in any predictable timeframe, or even at all, and you can see that there are relatively few situations for which finalization is the right tool to use.

Quote from Effective Java:

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.

Conclusion: If you insist on keeping them, the only code that should be left in the methods is code that logs. This is ok:

  protected void finalize() throws Throwable {
     try {
        if (!isClosed()) {
           L.error("not closed")
        } 
     } finally {
        super.finalize();
     }
  }

Any calls to close() or nativeClose() are not ok.

You cannot reliably fix these bugs and keep using finalizers, because on the next phone with different RAM or heap-size it will malfunction again. Heap-size is up to the phone-manufacturer by the way.

[fab1an]

@balazsbalazs Regarding why the manual GC doesn't clear: I think it's the memory problem. Also, i'm not sure if I followed the code correctly, but I think this call to .close() is not properly synchronised.

Couldn't you get rid of a lot of synchronisation along with the finalizers?

Also: It could be that the thread, which is invoking the finalizer, is from the application that uses Fresco and actually holding some other lock on this, deadlocking itself.

[narychen]

I confirm that the there must be memory problem in Fresco.
See #8677 (comment)

[narychen]

I got the logcat before the app crashed. The error is sharedmem_gpumem_alloc: mmap failed errno 12 Out of memory

07-13 17:24:15.370 23299-30736/com.eggf D/myfresco: -31-51001840
07-13 17:24:15.370 23299-23299/com.eggf V/unknown:AbstractDraweeController: controller 43faa1c8 1075: onTouchEvent MotionEvent { action=ACTION_DOWN, id[0]=0, x[0]=155.2655, y[0]=290.88635, toolType[0]=TOOL_TYPE_FINGER, buttonState=0, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=73184687, downTime=73184687, deviceId=5, source=0x1002 }
07-13 17:24:15.380 23299-30605/com.eggf V/unknown:NativeMemoryChunkPool: release (free) (object, size) = (447eab60, 131072)
07-13 17:24:15.380 23299-30605/com.eggf V/unknown:NativeMemoryChunkPool: Used = (25, 6129664); Free = (17, 1399808)
07-13 17:24:15.380 23299-30605/com.eggf V/unknown:NativeMemoryChunkPool: release (free) (object, size) = (43f934e0, 262144)
07-13 17:24:15.380 23299-30605/com.eggf V/unknown:NativeMemoryChunkPool: Used = (24, 5867520); Free = (17, 1399808)
07-13 17:24:15.380 23299-30605/com.eggf V/unknown:NativeMemoryChunkPool: release (free) (object, size) = (45011930, 524288)
07-13 17:24:15.380 23299-30605/com.eggf V/unknown:NativeMemoryChunkPool: Used = (23, 5343232); Free = (17, 1399808)
07-13 17:24:15.380 23299-30666/com.eggf V/unknown:SoftRefByteArrayPool: Used = (1, 524288); Free = (4, 1441792)
07-13 17:24:15.380 23299-30666/com.eggf V/unknown:SoftRefByteArrayPool: get (reuse) (object, size) = (44a75f48, 524288)
07-13 17:24:15.390 23299-23299/com.eggf V/unknown:AbstractDraweeController: controller 43faa1c8 1075: set_final_result @ onNewResult: image: CloseableReference 44694778
07-13 17:24:15.390 23299-30666/com.eggf D/skia: ---------- mmap failed for imageref_ashmem size=8294400
07-13 17:24:15.390 23299-30666/com.eggf V/unknown:SoftRefByteArrayPool: release (reuse) (object, size) = (44a75f48, 524288)
07-13 17:24:15.390 23299-30666/com.eggf V/unknown:SoftRefByteArrayPool: Used = (0, 0); Free = (5, 1966080)
07-13 17:24:15.390 23299-23372/com.eggf D/skia: --- SkImageDecoder::Factory returned null
07-13 17:24:15.390 23299-23372/com.eggf D/skia: --- SkImageDecoder::Factory returned null
07-13 17:24:15.400 23299-23299/com.eggf V/unknown:AbstractDraweeController: controller 432c43b8 1074: final_failed @ onFailure: failure: java.lang.RuntimeException: Failed to pin Bitmap
07-13 17:24:15.400 23299-23372/com.eggf D/skia: --- SkImageDecoder::Factory returned null
07-13 17:24:15.400 23299-23372/com.eggf D/skia: --- SkImageDecoder::Factory returned null
07-13 17:24:15.420 23299-23299/com.eggf W/Adreno-GSL: <sharedmem_gpumem_alloc_id:1427>: sharedmem_gpumem_alloc: mmap failed errno 12 Out of memory
07-13 17:24:15.420 23299-23299/com.eggf E/Adreno-GSL: <ioctl_kgsl_sharedmem_alloc:1528>: ioctl_kgsl_sharedmem_alloc: FATAL ERROR : (null)

[fab1an]

Okay, the spec says it's guaranteed that the thread that invokes finalize will not be holding any user-visible synchronization locks when finalize is invoked. So, assuming the finalizer itself doesn't deadlock and there's no deadlock with native code, it's not a deadlock.

Another theory: In the screenshot you can see there are 2527 finalizers waiting to be run.

• It's possible that the deadlock is in a finalizer from another library.
• That an extra slow finalizer from somewhere else comes before Fresco in the queue (the execution order isn't guaranteed after all, though I guess it's more or less FIFO).
• That the queue just fills up so quickly (I believe every JSmethod call of RN is finalized) that the fresco finalizers are just waiting for ages.
• It's theoretically possible that the thread doing finalization is running at a slower priority than the threads disposing finalizable objects.

[fab1an]

@narychen Do you use animated gifs or webps?

[narychen]

@fab1an Yes I do use gifs. But the problem could happen with only jpgs

[fab1an]

@narychen Can you do a ThreadDump using Android Studio and post it here?

[fab1an]

This might be related: https://stackoverflow.com/questions/37001181/android-finalizerdaemon-hanging-up

[fab1an]

@narychen Why did you delete the ThreadDump. I've linked to it in the google bugtracker.

Can you also do a heapdump and check how many FinalizerReference are in your queue?

[fab1an]

Ok here's a Threaddump from me: threads_report.txt

[narychen]

@fab1an Because I think it's too long for people to read.

[narychen]

And I post another issue on Fresco about the new clue I've found.
You may be interested to have a look facebook/fresco#1360

[fab1an]

I'm closing this issue in favor of #8780 and facebook/fresco#1363

[balazsbalazs]

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.
Fresco uses deterministic memory management so if used correctly all the memory is freed up before you get to the finalizer.
If it not used correctly (which being an open source library we can't guarantee) we're using the finalizers to clean up the memory. The reason we're doing this is because we're using the ashmem, which is a shared memory - so if you hold on to memory you might degrade not just your apps but the phones experience.

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.

[fab1an]

@balazsbalazs It might be that the issues come up here and not at FB/Twitter, because the data-structures for the bridge, WritableMap and so on are all in native memory and use Finalizers. But I don't know whether Facebook uses react-native of course.

If the problem really is that RN is interfering with fresco's memory in a bad way, it could be tested by temporarily replacing RN's WritableMap/WritableArray structures with serialised alternatives, like I wrote here: #8780 (comment)

[narychen]

@fab1an I agree with you. The problem hasn't been solved and it often crashes down the whole app especially with GIF pictures. And my same app doesn't have the issue even on my old iphone 5, so the problem is android only.

[kesha-antonov]

+1
App on Android uses a lot of memory

[ghost]

+1
when I pop a page, the memory used by image cannot recycle.