[not ready] fixes #1267: properly break reference cycle with tile data & style

[incanus]

No description provided.

[jfirebaugh]

I'm skeptical that this actually fixes the issue. The destructor will already do the equivalent of style.reset() automatically -- isn't the issue that the destructor never gets called, because of the reference loop?

[incanus]

Yep, still looking at this with the stuff from caching #1255 piled atop to make sure. I think you're right.

[incanus]

TileData fall out of scope properly, the problem is that LiveTileData didn't have a destructor at all. With no annotations on the map at startup every time, LiveTileData was getting created by StyleParser's creation of Source and Source::addTile(), but when AnnotationManager::getTile() would subsequently fail to return a pointer due to no annotations, the reset/parse block was never attempted.

You are right that we don't need a style.reset() in the destructor; I wasn't clear on that. I was determining that style.use_count() was non-zero in the destructor, but a reset() there is redundant.

[incanus]

isn't the issue that the destructor never gets called, because of the reference loop?

The destructor does get called, but it's too late to let go of the style.

We did have a ~LiveTileData(); the actual problem was a bad copy job of VectorTileData's try...catch around tile parsing. LiveTileData's has an additional condition around whether the annotation manager returns a tile or not, and if it didn't (the startup, annotation-less case and also quite frequently otherwise) the style wasn't reset when the tile data was marked obsolete. This needs to happen before marking obsolete so that the Source, when it sees them as obsolete, can throw out unneeded tiles but not have the reference to Style (which indirectly references itself, Source) left hanging around.

[incanus]

I still don't quite understand why this is the case, even though I've outlined it above. Are objects unable to release references to shared pointers when the released objects follow a shared pointer reference chain back to themselves?

[incanus]

Got it, finally. You are right @jfirebaugh that the problem is the TileData destructor never getting called. I was confused by seeing it called for viewport changes but missed that it wasn't being called for style changes. I was also confused because the Style destructor seemed like it always being called, but in fact it was only the initial dummy style created in Map::reloadStyle() before async JSON loading happens each style load and the dummy is replaced and zeros out. So the over-retained objects include the entire Style > StyleLayer > StyleBucket > Source > TileData chain and my initial hunch in #1267 of sources and the style being kept around was right.

When LiveTileData isn't in the picture (commenting out the annotations functionality in StyleParser), VectorTileData objects are destructed properly when the style changes. Style releases StyleLayer releases StyleBucket releases Source releases VectorTileData, which doesn't have a hold on Style. Though Source holds TileData weakly (in tile_data), it holds Tile strongly (in tiles), and Tile holds TileData strongly (in data), so this is a true cyclical reference and needs this style release.

Here you can see the dummy, then the real, style being created, then the dummy destroyed (style going away), then upon application of the new style, the first real style destroyed (style asset://styles/ going away), and then the whole graph after it (including all layers, then sources, then their live and vector tiles — total cleanup).

When annotation support was introduced with the bug of not releasing Style from LiveTileData when no annotations existed (startup condition), we'd get the reference cycle which would result in neither LiveTileData nor VectorTileData being released since the old style was over-retained (only on the "LiveTileData side", which of course was enough).

Here you see the dummy style pre-async load, and the real post-parse real style, created each time, but no real styles nor layers/buckets/sources/tiles ever destroyed.

This is good to go and I'm satisfied that I know why. Going to merge and get back to #1255.

[mb12]

@incanus and @jfirebaugh, can this cyclic reference explain why one might get "App terminated due to memory pressure" in xcode. I see this couple of times everyday. I do not have any live tiles/annotations explicitly added to the map. Its just plain zoom in/zoom out/pan operations on the map.

[jfirebaugh]

@mb12 My guess is that's an unrelated issue.

[incanus]

It could have been related — annotation tiles (LiveTileData, the stuff to render) are searched for always and from startup, even when no annotations exist, because annotation tiles (LiveTile, the coverage structures) exist, and those were over-retaining the style and leading to this cycle.

@mb12 Are you up to date with this PR and still seeing it?

[mb12]

My repo does not have this PR. I do not have any explicit annotation/livetiledata. Would this code still kick in?
I had tried running leaks a few times but it only showed minor leaks in mbgl::AssetRequest. Leaks would not show objects if there is a valid reference to them.

[incanus]

Yes, annotations are always used in a sense.

• Tile structures like LiveTile are always created to cover the current viewport. Answers the question "what tiles do I need right now?"
• TileData structures like LiveTileData are then loaded/pulled from cache/pulled from annotation store asynchronously in response. Answers the question "what data is in a tile and is it ready?"

So even without any annotations, the annotation source is always present and LiveTile structures are always generated to cover the viewport. Then, since annotations are always ready, LiveTileData structures are always created, query the annotation store for their data, and go away if not relevant right now. The going away, however, was not properly releasing the style in the case of annotation tiles.

[mb12]

@incanus Thanks a lot for the detailed clarification.

[jfirebaugh]

@incanus Could that leak occur even if you only loaded a style once? My impression was you had to be doing repeated style loads for it to happen.

[incanus]

Ah, good point @jfirebaugh. If the first style never goes away, over-retaining does not matter.