Tile textures are retained indefinitely when raster sources are removed

[Scarysize]

mapbox-gl-js version: v0.31.0 (also found the issue with 0.26.0)

Steps to Trigger Behavior

jsbin: https://jsbin.com/kojobocuka/edit?js,output

1. Open jsbin. Take a heap snapshot with DevTools. Check allocated WebGLTexture object count (should be around 8, if the map wasn't moved/zoomed)
2. Hit the "GO!" button, this will add and remove raster sources & layers with random ids and varying tile urls every 500ms a 180 times.
3. Take another heap snapshopt. Check the texture object count again, it's now at about 3000.
4. (The GO! button hit again to re-run the test and further increase the object count)

Expected Behavior

Textures aren't retained indefinitely for removed sources.

Actual Behavior

Textures are retained for removed sources; as far as I can see they won't be deleted. For long running applications with multiple raster sources & layers being added/removed over time this will lead to increasing memory consumption.

[mourner]

Thanks for the report! This issue actually looks like a good task for a contribution. Want to work on a PR fixing this? I'll be happy to review and help you land this.

[Scarysize]

Yes, I could work on this.

What would be the behaviour? Delete all Textures as soon as a source is removed?

What is the intention of keeping those textures around anyway? Is it to re-use them, when tiles are re-entering the viewport?

[mourner]

See the original issue: #591. The reason is to avoid expensive gl.createTexture/deleteTexture calls. Let's find out why the pool grows out of control and fix it so that it's stable.

[Scarysize]

Okay, I think I'm understanding it now. WebGLTexture instances with certain sizes are cached, we are not interested in the textures contents.

So textures are saved in an array indexed by their size:

//...
this.reusableTextures[texture.size] = [texture];

If a texture of this size is already available, the texture is just pushed into that array:

textures.push(texture);

The retained textures are accessed again through painter.getTileTexture(width, height); this is called e.g. by a RasterTileSource before creating a new texture from a loaded tile image:

tile.texture = this.map.painter.getTileTexture(img.width);

Now getTileTexture tries to look for a reusable texture, but does the indexing wrong (?):

const widthTextures = this.reusableTextures[width];
if (widthTextures) {
    const textures = widthTextures[height || width];
    return textures && textures.length > 0 ? textures.pop() : null;
}

height is undefined for that called, so it basically checks if this.reusableTextures[width][width] is available. This won't be the case, because the textures were just pushed into the second array, not correctly indexed. textures.pop() will never be called, unless we saved width amount of textures.

[lbud]

Oh rats, this is my fault. Previously we only used square textures and so saved them only by one dimension. fill-extrusion layers now also utilize textures the size of the full screen, which, being almost always rectangular, necessitated indexing saved textures both by width and height. I think I got this indexing garbled in a mid-PR refactor (I had originally been attempting to save all textures to this same textures object, before switching to storing them in separate tile vs viewport objects).

(intro'ed in c8cbb9d#diff-d385e592acb94e49c99d265414e2f099R318)

Thanks for digging in here @Scarysize — looks like it should be a pretty straightforward PR if you're still up for fixing it!

[Scarysize]

@lbud Thanks for chiming in. I will work on a PR today :)

[Scarysize]

See #3974

[lucaswoj]

Fixed by #3974 😄