mapbox / mapbox-maps-flutter

Interactive, thoroughly customizable maps for Flutter powered by Mapbox Maps SDK
https://www.mapbox.com/mobile-maps-sdk
Other
281 stars 112 forks source link

Memory leak on iOS and Android #101

Closed Lah123 closed 1 year ago

Lah123 commented 1 year ago

Not all memory is being disposed. The memory slowly builds up when you move around the map and does not get released when the widget is disposed.

Steps to reproduce: Start the example app (I'm using a iOS device with version 12) Open a random map, scroll around a bit Notice that the memory keeps getting larger (Trying to set a budget using setMemoryBudget does not help) Close the map and try another one, scroll around a bit Keep on doing this untill the app crashes.

On my device (an old one, but still) it crashes when the memory takes up around 630MB. Newer devices have more RAM so it takes a bit longer.

In my own app I tried to set the memoryBudget, activated reduceMemoryUse and tried to dispose the map to solve the problem. Unfortunately, it still persists. This is a blocking issue for me (and probably for others as well). Can someone help me out? Am I missing something or is this a real bug?

Edit: See comments, it also crashes on android devices. I updated the title accordingly.

PhilippMatthes commented 1 year ago

Is this related to our problem @adeveloper-wq ?

adeveloper-wq commented 1 year ago

Yes, I think that we also experience this problem. The following are our observations and findings with this:

Lah123 commented 1 year ago

Thank you for endorsing this. Although it is not the outcome I hoped for, it is nice to see that I'm not crazy.

I tried my app on around 15 different physical devices (andriod and iOS), but only iOS crashed. It's good to know that also Android is suffering from this issue. I hope the developers can pick this up soon...

adeveloper-wq commented 1 year ago

Just tested it again on the Motorola Moto X Style and iPhone XR, just to be sure. It crashed on both phones in the example app.

Lah123 commented 1 year ago

Just tested it again on the Motorola Moto X Style and iPhone XR, just to be sure. It crashed on both phones in the example app.

Thank you for this extra test. I updated the title and description accordingly.

yunikkk commented 1 year ago

Hey all, looking into this one, thanks for reports.

ivan-ljubicic commented 1 year ago

Hi everyone. I did some quick debugging and wanted to share my findings here.

I did the debugging only on android and observed that only native memory is being increased when dynamically pushing and popping a route with MapWidget. Also, example app has some issues regarding the memory leaks (in dart) because of the caching of the ExamplePage widgets. If we dynamically return ExamplePage when pushing route without it being cached in the list there are no memory leaks. Actually there are, but they are related to other issues, for example #106, which is FullMapPage example.

I think this might be some bug in flutters Platform Views or race condition in disposal. I did the profiling of the android memory and found out that MapView and its related classes don't get dealocated, even with manually forcing GC, but the maps state is marked as destroyed so I supose that Platform Views are the ones causing the memory leak. Because I couldn't find out which is blocking which in being dealocated, I tried to run the same profiling with Google Maps, and found out that there is the same problem with that plugin, which led me to conclusion that there are some bugs in flutter.

This conclusion could use more debugging to be sure that this is in fact the case, which I intent to do when I have more spare time.

PhilippMatthes commented 1 year ago

If this is really an internal Flutter problem we would still appreciate a workaround.

PhilippMatthes commented 1 year ago

Any progress on this?

Lah123 commented 1 year ago

Thanks @LjubiTech-Maxko and @yunikkk for your debugging and attention. The conclusion freigthens me a bit, that would mean that your package will never be able to go into production (unless the bug in flutter is fixed). How does Google maps prevent a app crash? Do you have any thoughts on how to fix or work around this issue? I would love to hear from you.

yunikkk commented 1 year ago

For anyone instrested - I was able to look more into this memory leak yesterday and it seems the problem is clear. I'll share branch with fixes to test soon.

PhilippMatthes commented 1 year ago

Nice!

yunikkk commented 1 year ago

Hey @LjubiTech-Maxko @Lah123 @adeveloper-wq @PhilippMatthes, I've pushed bunch of memory leaks fixes, testing it now, so far it seems map instance and handlers are not leaking anymore. Anyone willing to test please use my branch :

  mapbox_maps_flutter:
    git:
      url: git@github.com:mapbox/mapbox-maps-flutter.git
      ref: yds-fix-memory-leaks
PhilippMatthes commented 1 year ago

@adeveloper-wq we should test this tomorrow 🚀

PhilippMatthes commented 1 year ago

@yunikkk thanks for the fix, our app indeed does not crash anymore. Seems like the memory leaks which caused our app to crash is fixed ✅

yunikkk commented 1 year ago

@PhilippMatthes good to hear! I'll conduct more tests today and plan to publish the release with fixes on Monday

yunikkk commented 1 year ago

Hey all, heads up that during testing I've found one more problem on Android, seems to be this SurfaceView bugreport. Reproduces if textureView flag is not set.

ivan-ljubicic commented 1 year ago

Hi everyone,

@yunikkk I did some profiling on android but it seems like the memory leak is not solved. Here are some screenshots from the memory allocations and before/after pushing and poping a screen with map widget 5 times.

We can se most of the classed never beeing dealocated. mbx_leak_allocations

Memory before pushing the screen with MapWidget mbx_leak_before_map

Memory after pushing and poping the screen with MapWidget 5 times mbx_leak_after_map

ivan-ljubicic commented 1 year ago

Hey all, heads up that during testing I've found one more problem on Android, seems to be this SurfaceView bugreport. Reproduces if textureView flag is not set.

I have seen that bug report when I comented earlier about the possibility of bugs in flutters PlatformViews and tried then and again now with the flag set, but the outcome is the same.

yunikkk commented 1 year ago

I have seen that bug report when I comented earlier about the possibility of bugs in flutters PlatformViews and tried then and again now with the flag set, but the outcome is the same.

I will recheck java heap, thanks

yunikkk commented 1 year ago

@LjubiTech-Maxko I did more testing and in my tests Java heap is not leaking also. Have you triggered full GC using force garbage collection button in Studio? On my Pixel I'm seeing app keeping 10-20 or more MapView instances until I force GC and after that no maps are left in heap.

UPD. Though it's not leaking only with one of the latest fixes indeed, I've pushed the latest version.

ivan-ljubicic commented 1 year ago

@LjubiTech-Maxko I did more testing and in my tests Java heap is not leaking also.

Have you triggered full GC using force garbage collection button in Studio? On my Pixel I'm seeing app keeping 10-20 or more MapView instances until I force GC and after that no maps are left in heap.

Yes I did. You can see in the first screenshot garbage icons when I did the manual GC. After every pop of the map page and in the end multiple times just to make sure. I will do some more tests tomorrow with different device and post my findings here.

yunikkk commented 1 year ago

Yes I did. You can see in the first screenshot garbage icons when I did the manual GC.

Right, I was testing the more recent version of the branch indeed. With the latest one all Dart / EGL / Java seem to retain the memory correctly.

ivan-ljubicic commented 1 year ago

Hi @yunikkk

The first test with above screenshots was done on Xperia XZ Premium which has Android 9. I suspected that the Android 9 could be the culprit so I tested it again with Samsung S22 with Android 13 and Samsung A6 with Android 10 but the result is the same. I tested both with textureView enabled/disabled and both debug/profile. I could also paste the code used for tests but it is a simple button which when pressed pushes the route with MapWidget with only token and textureView set.

These are screenshots in profile mode with textureView enabled. I also took the heap snapshot because of the differences in the allocations, for example LogoViewImpl is allocated 3, 3, 5 times but it was 5 in the previous screenshot as it should be.

  1. Samsung A6 Android 10 - 81.7/160.7 mb
  2. Samsung S22 Android 13 - 161.1/327.1 mb
  3. Xperia XZ Premium Android 9 - 126.1/154.9 mb

Based on theese tests it turns out that the Android 9 has the smallest leak.

Allocations mbx_leak_allocations

Heap mbx_leak_heap

Before pushing and poping map mbx_leak_before_map

After pushing and poping map mbx_leak_after_map

EDIT: I misunderstood the last comment and haven't seen the updated comment before that. Theese tests were before Your latest fixes. Will test again later.

ivan-ljubicic commented 1 year ago

Hi @yunikkk

I have redone the tests using the latest fixes with Android 9 and Android 13 devices and can confirm that the memory leak is not present anymore. All MapViews and MapControllers are dealocated and not found when running heap dump after tracking allocations.

I can still observe increase in Native and Graphics memory only on Android 13 device but I suppose that might be cache or something similar because it never goes above a certain threshold, no mater how many maps I create and dispose of.

yunikkk commented 1 year ago

@LjubiTech-Maxko thanks for tests and confirming! Native / graphics memory can grow of course, internal c++ engine caches various resources, though they should not leak and be released when map is destroyed.

I'll land the implemented fixes and publish the patch tomorrow.

yunikkk commented 1 year ago

Patch 0.4.3 with the memory leak fixes has been released to pub.dev! Closing this one.