Gamua / Starling-Framework

The Cross Platform Game Engine
http://www.starling-framework.org
Other
2.84k stars 819 forks source link

iOS specific memory leak in BatchPool #950

Closed neuronix closed 7 years ago

neuronix commented 7 years ago

Hi Daniel,

On ios our app had 45% of cpu time used up by BatchPool.get & memory cleaning due to a very high number of String allocations/deallocations.

The code is very simple, no object creation at first sight. However my intuition was that it was related to the dictionary Class -> Vector.\<MeshBatch> and a bad implementation in the iOS IOT. Indeed, after testing on Android/Windows, no string allocation.

I stumbled upon this topic that has the exact same issue, string allocation with a dictionary of vectors on ios: https://forums.adobe.com/thread/2088454

In BatchPool, I did a dump of the _batchLists content and noticed that there were only 2 keys: MeshStyle & DistanceFieldStyle, so a simple fix was to add 2 new vectors and some if/else tests to directly use these vectors instead of looking them up in the dictionary.

My fix:

class BatchPool
{
    private var _batchLists:Dictionary;
    private var _batchMeshStyle:Vector.<MeshBatch>;
    private var _batchDistanceFieldStyle:Vector.<MeshBatch>;

    public function BatchPool()
    {
        _batchLists = new Dictionary();
        _batchMeshStyle = new <MeshBatch>[];
        _batchDistanceFieldStyle = new <MeshBatch>[];
    }

    public function purge():void
    {
        for each (var batchList:Vector.<MeshBatch> in _batchLists)
        {
            purgeList(batchList);
        }
        purgeList(_batchMeshStyle);
        purgeList(_batchDistanceFieldStyle);
    }

    private function purgeList(batchList:Vector.<MeshBatch>) : void {
        for (var i:int = 0; i < batchList.length; ++i)
        {
            batchList[i].dispose();
        }
        batchList.length = 0;
    }

    public function get(styleType:Class):MeshBatch
    {
        var batchList:Vector.<MeshBatch>;

        if (styleType == MeshStyle)
        {
            batchList = _batchMeshStyle;
        } 
        else if (styleType == DistanceFieldStyle)
        {
            batchList = _batchDistanceFieldStyle;
        } 
        else
        {
            batchList = _batchLists[styleType];
            if (batchList == null)
            {
                batchList = new <MeshBatch>[];
                _batchLists[styleType] = batchList;
            }
        }

        if (batchList.length > 0) return batchList.pop();
        else return new MeshBatch();
    }

    public function put(meshBatch:MeshBatch):void
    {
        var styleType:Class = meshBatch.style.type;

        var batchList:Vector.<MeshBatch>;

        if (styleType == MeshStyle)
        {
            batchList = _batchMeshStyle;
        } 
        else if (styleType == DistanceFieldStyle)
        {
            batchList = _batchDistanceFieldStyle;
        } 
        else
        {
            batchList = _batchLists[styleType];
            if (batchList == null)
            {
                batchList = new <MeshBatch>[];
                _batchLists[styleType] = batchList;
            }
        }

        meshBatch.clear();
        batchList[batchList.length] = meshBatch;
    }
}

The effect is quite obvious as you can see on these 2 scout recordings:

Before: batchpool

After: batchpool_fixed

  1. I think it is a mandatory optimization to skip the dictionary call at least for MeshStyle since it is the only style in 99% of cases.
  2. I would suggest implementing the same fix for all possible classes in Starling and relying on a dictionary for custom styles.

Thanks for your work Daniel!

Regards,

Cameron

PrimaryFeather commented 7 years ago

Thanks a lot for the heads-up, and for posting a possible workaround!

I contacted Adobe again about this issue (there's also a report on Adobe Tracker, and they said they are working on it. But we probably can't wait for the results ... who knows when the fix is going to be released.

I'm curious — did you also try to replace the Vector with an Array? I've run into performance problems with the Vector in AOT mode several times in the past, and often the standard Array worked better.

neuronix commented 7 years ago

I didn't think of trying an Array, I don't have much time for optimization right now, I just wanted to fix this as it was a big issue.

I'll investigate that later, our app is probably a good benchmark for testing since we use DistanceFieldStyle extensively so BatchPool is having to work pretty hard! I'll let you know what I find :)

PrimaryFeather commented 7 years ago

Don't worry, do it only when time allows it. I'll probably give it a try myself, too. Thanks again for sharing your solution, in any case, and all the best for your upcoming project!

PrimaryFeather commented 7 years ago

Semi-good news: while this still isn't fixed on Adobe's side, at least there is an easier workaround: using an Array instead of a Vector. That's not the first issue caused by Vectors in AOT compilation ... seems to be tricky with this mode.

neuronix commented 7 years ago

Okay, thanks for the info. However I still think that you should make and exception for MeshStyle & DistanceFieldStyle because of how many times per frame they will be used. I know my suggestion isn't aesthetically appealing but it does avoid a "useless" Dictionary call :)

PrimaryFeather commented 7 years ago

Well, it's only one Dictionary access per draw call, so it's probably not super critical — but I'll think about it!