mapbox / mapbox-sdk-cs

C# libraries for Mapbox API layer
https://mapbox.github.io/mapbox-sdk-cs/
Other
20 stars 11 forks source link

Optimize http requests - huge Texture2D memory leak #31

Open wilhelmberg opened 7 years ago

wilhelmberg commented 7 years ago

After introducing #23 Unity.exe uses a few GB of RAM after only a few pans.

Textures are destroyed like this: https://github.com/mapbox/mapbox-sdk-unity/blob/master/sdkproject/Assets/Mapbox/Examples/Slippy/Scripts/SlippyTile.cs#L114-L130

This used to work with the previous single threaded implementation and memory usage was constant during panning.

@david-rhodes @brnkhy Any ideas why textures are not referenced anymore but stay memory?

image

david-rhodes commented 7 years ago

If this used to work before threading, I would put the blame on WWW if that's what you have as your implementation right now. So, compare memory to UnityWebRequest for starters.

I'm pretty sure this is it because I've noticed the current MapboxGO and Lyft Demos are both giving me low memory warning or crashing since changing to WWW.

If that does not work, see below.

More information here: http://answers.unity3d.com/questions/1158877/memory-leak-with-www-on-ios.html

Resources.UnloadUnusedAssets will likely help, but it's also not good to call frequently (frame rate will take a dive during GC). If you have a hook to the specific texture, you should be able to call Resources.UnloadAsset directly on it.

We may also need to dispose WWW

I'm going to test Unity 5.5 to see if the UnityWebRequest has been improved at all. I'm not comfortable having different IFileSources with different problems. It's good to have them both, but we should intend to recommend and support one over the other.

wilhelmberg commented 7 years ago

Already tried http://answers.unity3d.com/questions/1158877/memory-leak-with-www-on-ios.html and disposing of WWW.

I'm already on Unity 5.5

Going to try with UnityWebRequest again.

Another common source of memory leaks is not disconnecting event handlers. Going to check that too - although I don't think that's the case here.

wilhelmberg commented 7 years ago

Nope, still the same with UnityWebRequest:

image


Current hunch:

How the tile data (byte[]) is passed through all the different abstraction layers up to creating the texture: TileFetchingThread -> TileMemoryCache -> TileFetcher -> Map -> Slippy -> SlippyTile -> Texture2D.

Maybe some lower levels still holding a reference that keeps Texture2D from properly disposing. Most likely TileMemoryCache. Going to test cloning the byte array before applying to texture.

david-rhodes commented 7 years ago

@BergWerkGIS You can add your own custom profiling data. Perhaps this will help identify the leak.

wilhelmberg commented 7 years ago

You can add your own custom profiling data. Perhaps this will help identify the leak.

@david-rhodes thanks, that looks good, haven't tried it yet.


Finally - a small victory!

Calling

Resources.UnloadUnusedAssets();

after Destroy()ing and nulling the SlippyTile objects frees the memory and removes the unreferenced Texture2Ds:

image


TODOs

UnloadUnusedAssets()

Have to read up about Resources.UnloadUnusedAssets() and its performance penalties. Calling such methods manually always has some impacts 😏

other memory leaks

Although the huge memory leak is gone now, memory usage still increases gradually. So there must be something else somewhere.

wilhelmberg commented 7 years ago

Before and after screenshots of extended panning.

Memory usage of Unity.exe in the Task Manager had risen from ~550MB to 750MB.

The ~200MB memory increase correlates to Other. The other groups stayed the same.

image

image

david-rhodes commented 7 years ago

Revisiting this with a reminder: I was working on an optimization for a developer and noticed that you simply need to destroy the texture to free up that memory (assuming nthing has a reference to it).

Destroy(texture);

// And for good measure
texture = null;