[core] Convert GeometryTileWorker to "one-phase" loading #11575
Conversation
|
/cc @lilykaiser |
ChrisLoer
force-pushed
the
one-phase-tiles
branch
from
fefef01
to
e4dda35
Compare
There was a problem hiding this comment.
I think we should go this route for release-boba. I think the very fact that we're unable to have confidence in changes in this area means we need to prioritize simplification, even if the likelihood of bugs in the short term is higher than with #11570.
src/mbgl/tile/geometry_tile.cpp
Outdated
| loaded = true; | ||
| renderable = true; | ||
| if (resultCorrelationID == correlationID) { | ||
| pending = false; | ||
| } | ||
|
|
||
| nonSymbolBuckets = std::move(result.nonSymbolBuckets); |
There was a problem hiding this comment.
We can also merge nonSymbolBuckets and buckets everywhere.
src/mbgl/tile/geometry_tile.cpp
Outdated
| pendingFeatureIndex->first = true; | ||
| } | ||
|
|
||
| dataPendingCommit = {{ std::move(result.tileData), std::move(result.featureIndex) }}; |
There was a problem hiding this comment.
Since they must be synchronized, can tileData be owned by the FeatureIndex?
There was a problem hiding this comment.
👍 Making this change actually exposed a bug I had previously introduced with this PR. 😬
Because the data.clone() call had moved to symbol layout time, the feature index could be desynchronized from the data if a setData arrived while the worker was waiting for symbols (the new data is supposed to be queued up for a re-layout after the current one finishes). Moving ownership of the GeometryTileData into FeatureIndex solves the problem by forcing the clone to happen at the time the index is created.
Conversion to one-phase tile loading removed any need to track them separately.
Prevents querying a FeatureIndex built against a separate set of data, which can lead to invalid index exceptions. The GeometryTileWorker 'data' member can still change independently of the data in the feature index, at the time 'setData' is called. The GeometryTileWorker maintains ownership of its local data (which may be used to re-parse) and clones the data for use by the FeatureIndex in the foreground.
This is an alternative to PR #11570 as a fix for issue #11538.
It's a more complicated change than #11570, but it addresses some of the root cause by removing unnecessary intermediate state from
GeometryTile(we wanted to make these changes already: tracked as issue #10457). I used @friedbunny's repro branch to verify that it fixes #11538.While
GeometryTilegets simpler because it no longer maintains a half-loaded (everything but symbol buckets) state, some of the complexity moves toGeometryTileWorkerbecause it becomes responsible for holding on to thefeatureIndexandnonSymbolBucketsin the period betweenparse()andperformSymbolLayout().This PR brings us a little closer to the JS behavior, but native is still significantly more complicated. The primary reason is that native supports incremental updates of glyphs/images, while JS requires each tile load to get a completely new set of glyphs/images from the foreground. I don't know if there are many situations where this is actually an important performance optimization for native -- although for frequently updated tiles w/ symbols, it potentially allows the reloading to be done in a single step on the background, instead of always requiring the symbol dependency back-and-forth between background and foreground.
If we did decide to go all the way to the JS behavior, we'd also have to implement some equivalent of the callback registration/management handled by
actor.js.I have mixed feelings about using this as the fix for
release-boba. I think it's a move in the right direction, but I'm also worried about introducing instability late in the game. RefactoringGeometryTileWorkeris really tricky, and I know I almost introduced some bugs this afternoon./cc @jfirebaugh @ansis @friedbunny @tobrun @julianrex