[core] Convert GeometryTileWorker to "one-phase" loading

Modest simplification refactoring (issue #10457).
Also, fixes issue #11538, which was caused in part by a hole in the vestigial two-phase loading.

5 files changed, 141 insertions, 162 deletions

diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp
index 0c8a92d8b7..f95d7220bd 100644
--- a/src/mbgl/tile/geometry_tile.cpp
+++ b/src/mbgl/tile/geometry_tile.cpp
@@ -125,36 +125,17 @@ void GeometryTile::setShowCollisionBoxes(const bool showCollisionBoxes_) {
 }
 
 void GeometryTile::onLayout(LayoutResult result, const uint64_t resultCorrelationID) {
-    // Don't mark ourselves loaded or renderable until the first successful placement
-    // TODO: Ideally we'd render this tile without symbols as long as this tile wasn't
-    //  replacing a tile at a different zoom that _did_ have symbols.
-    (void)resultCorrelationID;
-    nonSymbolBuckets = std::move(result.nonSymbolBuckets);
-    // It is possible for multiple onLayouts to be called before an onPlacement is called, in which
-    // case we will discard previous pending data
-    pendingFeatureIndex = {{ false, std::move(result.featureIndex) }};
-    pendingData = {{ false, std::move(result.tileData) }};
-    observer->onTileChanged(*this);
-}
-
-void GeometryTile::onPlacement(PlacementResult result, const uint64_t resultCorrelationID) {
     loaded = true;
     renderable = true;
     if (resultCorrelationID == correlationID) {
         pending = false;
     }
+    
+    nonSymbolBuckets = std::move(result.nonSymbolBuckets);
     symbolBuckets = std::move(result.symbolBuckets);
-    // When symbolBuckets arrive, mark pendingData/FeatureIndex as "ready for commit" at the
-    // time of the next global placement. We are counting on these symbolBuckets being
-    // in sync with the data/index in the last `onLayout` call, even though the correlation IDs
-    // may not be the same (e.g. "setShowCollisionBoxes" could bump the correlation ID while
-    // waiting for glyph dependencies)
-    if (pendingData) {
-        pendingData->first = true;
-    }
-    if (pendingFeatureIndex) {
-        pendingFeatureIndex->first = true;
-    }
+    
+    dataPendingCommit = {{ std::move(result.tileData), std::move(result.featureIndex) }};
+
     if (result.glyphAtlasImage) {
         glyphAtlasImage = std::move(*result.glyphAtlasImage);
     }
@@ -227,17 +208,12 @@ Bucket* GeometryTile::getBucket(const Layer::Impl& layer) const {
 }
 
 void GeometryTile::commitFeatureIndex() {
-    // We commit our pending FeatureIndex and GeometryTileData when:
-    // 1) An `onPlacement` result has delivered us updated symbolBuckets since we received the pending data
-    // 2) A global placement has run, synchronizing the global CollisionIndex with the latest
-    //    symbolBuckets (and thus with the latest FeatureIndex/GeometryTileData)
-    if (pendingFeatureIndex && pendingFeatureIndex->first) {
-        featureIndex = std::move(pendingFeatureIndex->second);
-        pendingFeatureIndex = nullopt;
-    }
-    if (pendingData && pendingData->first) {
-        data = std::move(pendingData->second);
-        pendingData = nullopt;
+    // We commit our pending FeatureIndex and GeometryTileData when a global placement has run,
+    // synchronizing the global CollisionIndex with the latest symbolBuckets/FeatureIndex/GeometryTileData
+    if (dataPendingCommit) {
+        data = std::move(dataPendingCommit->first);
+        featureIndex = std::move(dataPendingCommit->second);
+        dataPendingCommit = nullopt;
     }
 }
 
diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp
index 8843d6e579..98b5f0c490 100644
--- a/src/mbgl/tile/geometry_tile.hpp
+++ b/src/mbgl/tile/geometry_tile.hpp
@@ -68,30 +68,24 @@ public:
         std::unordered_map<std::string, std::shared_ptr<Bucket>> nonSymbolBuckets;
         std::unique_ptr<FeatureIndex> featureIndex;
         std::unique_ptr<GeometryTileData> tileData;
+        std::unordered_map<std::string, std::shared_ptr<Bucket>> symbolBuckets;
+        optional<AlphaImage> glyphAtlasImage;
+        optional<PremultipliedImage> iconAtlasImage;
 
         LayoutResult(std::unordered_map<std::string, std::shared_ptr<Bucket>> nonSymbolBuckets_,
                      std::unique_ptr<FeatureIndex> featureIndex_,
-                     std::unique_ptr<GeometryTileData> tileData_)
+                     std::unique_ptr<GeometryTileData> tileData_,
+                     std::unordered_map<std::string, std::shared_ptr<Bucket>> symbolBuckets_,
+                     optional<AlphaImage> glyphAtlasImage_,
+                     optional<PremultipliedImage> iconAtlasImage_)
             : nonSymbolBuckets(std::move(nonSymbolBuckets_)),
               featureIndex(std::move(featureIndex_)),
-              tileData(std::move(tileData_)) {}
-    };
-    void onLayout(LayoutResult, uint64_t correlationID);
-
-    class PlacementResult {
-    public:
-        std::unordered_map<std::string, std::shared_ptr<Bucket>> symbolBuckets;
-        optional<AlphaImage> glyphAtlasImage;
-        optional<PremultipliedImage> iconAtlasImage;
-
-        PlacementResult(std::unordered_map<std::string, std::shared_ptr<Bucket>> symbolBuckets_,
-                        optional<AlphaImage> glyphAtlasImage_,
-                        optional<PremultipliedImage> iconAtlasImage_)
-            : symbolBuckets(std::move(symbolBuckets_)),
+              tileData(std::move(tileData_)),
+              symbolBuckets(std::move(symbolBuckets_)),
               glyphAtlasImage(std::move(glyphAtlasImage_)),
               iconAtlasImage(std::move(iconAtlasImage_)) {}
     };
-    void onPlacement(PlacementResult, uint64_t correlationID);
+    void onLayout(LayoutResult, uint64_t correlationID);
 
     void onError(std::exception_ptr, uint64_t correlationID);
     
@@ -124,16 +118,15 @@ private:
     uint64_t correlationID = 0;
 
     std::unordered_map<std::string, std::shared_ptr<Bucket>> nonSymbolBuckets;
+    std::unordered_map<std::string, std::shared_ptr<Bucket>> symbolBuckets;
+    
+    optional<std::pair<std::unique_ptr<const GeometryTileData>, std::unique_ptr<FeatureIndex>>> dataPendingCommit;
     std::unique_ptr<FeatureIndex> featureIndex;
-    optional<std::pair<bool,std::unique_ptr<FeatureIndex>>> pendingFeatureIndex;
     std::unique_ptr<const GeometryTileData> data;
-    optional<std::pair<bool, std::unique_ptr<const GeometryTileData>>> pendingData;
 
     optional<AlphaImage> glyphAtlasImage;
     optional<PremultipliedImage> iconAtlasImage;
 
-    std::unordered_map<std::string, std::shared_ptr<Bucket>> symbolBuckets;
-
     const MapMode mode;
     
     bool showCollisionBoxes;
diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp
index 8b6f1f63bf..482cbe0053 100644
--- a/src/mbgl/tile/geometry_tile_worker.cpp
+++ b/src/mbgl/tile/geometry_tile_worker.cpp
@@ -41,48 +41,73 @@ GeometryTileWorker::GeometryTileWorker(ActorRef<GeometryTileWorker> self_,
 GeometryTileWorker::~GeometryTileWorker() = default;
 
 /*
-   NOTE: The comments below are technically correct, but currently
-   conceptually misleading. The change to foreground label placement
-   means that:
-   (1) "placement" here is a misnomer: the remaining role of
-    "attemptPlacement" is symbol buffer generation
-   (2) Once a tile has completed layout, we will only run
-    "attemptPlacement" once
-   (3) Tiles won't be rendered until "attemptPlacement" has run once
- 
-   TODO: Simplify GeometryTileWorker to fit its new role
-    https://github.com/mapbox/mapbox-gl-native/issues/10457
-
    GeometryTileWorker is a state machine. This is its transition diagram.
    States are indicated by [state], lines are transitions triggered by
    messages, (parentheses) are actions taken on transition.
 
-                              [idle] <----------------------------.
-                                 |                                |
-      set{Data,Layers,Placement}, symbolDependenciesChanged       |
-                                 |                                |
-           (do layout/placement; self-send "coalesced")           |
-                                 v                                |
-                           [coalescing] --- coalesced ------------.
-                               |   |
-             .-----------------.   .---------------.
+                         [Idle] <-------------------------.
+                            |                             |
+        set{Data,Layers}, symbolDependenciesChanged,      |
+                    setShowCollisionBoxes                 |
+                            |                             |
+   (do parse and/or symbol layout; self-send "coalesced") |
+                            v                             |
+                      [Coalescing] --- coalesced ---------.
+                         |   |
+             .-----------.   .---------------------.
              |                                     |
-   .--- set{Data,Layers}                      setPlacement -----.
-   |         |                                     |            |
-   |         v                                     v            |
-   .-- [need layout] <-- set{Data,Layers} -- [need placement] --.
+   .--- set{Data,Layers}                setShowCollisionBoxes,
+   |         |                         symbolDependenciesChanged --.
+   |         |                                     |               |
+   |         v                                     v               |
+   .-- [NeedsParse] <-- set{Data,Layers} -- [NeedsSymbolLayout] ---.
              |                                     |
          coalesced                             coalesced
              |                                     |
              v                                     v
-    (do layout or placement; self-send "coalesced"; goto [coalescing])
-
-   The idea is that in the [idle] state, layout or placement happens immediately
-   in response to a "set" message. During this processing, multiple "set" messages
-   might get queued in the mailbox. At the end of processing, we self-send "coalesced",
-   read all the queued messages until we get to "coalesced", and then redo either
-   layout or placement if there were one or more "set"s (with layout taking priority,
-   since it will trigger placement when complete), or return to the [idle] state if not.
+   (do parse or symbol layout; self-send "coalesced"; goto [coalescing])
+
+   The idea is that in the [idle] state, parsing happens immediately in response to
+   a "set" message, and symbol layout happens once all symbol dependencies are met.
+   During this processing, multiple "set" messages might get queued in the mailbox.
+   At the end of processing, we self-send "coalesced", read all the queued messages
+   until we get to "coalesced", and then re-parse if there were one or more "set"s or
+   return to the [idle] state if not.
+ 
+   One important goal of the design is to prevent starvation. Under heavy load new
+   requests for tiles should not prevent in progress request from completing.
+   It is nevertheless possible to restart an in-progress request:
+ 
+    - [Idle] setData -> parse()
+        sends getGlyphs, hasPendingSymbolDependencies() is true
+        enters [Coalescing], sends coalesced
+    - [Coalescing] coalesced -> [Idle]
+    - [Idle] setData -> new parse(), interrupts old parse()
+        sends getGlyphs, hasPendingSymbolDependencies() is true
+        enters [Coalescing], sends coalesced
+    - [Coalescing] onGlyphsAvailable -> [NeedsSymbolLayout]
+           hasPendingSymbolDependencies() may or may not be true
+    - [NeedsSymbolLayout] coalesced -> performSymbolLayout()
+           Generates result depending on whether dependencies are met
+           -> [Idle]
+ 
+   In this situation, we are counting on the idea that even with rapid changes to
+   the tile's data, the set of glyphs/images it requires will not keep growing without
+   limit.
+ 
+   Although parsing (which populates all non-symbol buckets and requests dependencies
+   for symbol buckets) is internally separate from symbol layout, we only return
+   results to the foreground when we have completed both steps. Because we _move_
+   the result buckets to the foreground, it is necessary to re-generate all buckets from
+   scratch for `setShowCollisionBoxes`, even though it only affects symbol layers.
+ 
+   The GL JS equivalent (in worker_tile.js and vector_tile_worker_source.js)
+   is somewhat simpler because it relies on getGlyphs/getImages calls that transfer
+   an entire set of glyphs/images on every tile load, while the native logic
+   maintains a local state that can be incrementally updated. Because each tile load
+   call becomes self-contained, the equivalent of the coalescing logic is handled by
+   'reloadTile' queueing a single extra 'reloadTile' callback to run after the next
+   completed parse.
 */
 
 void GeometryTileWorker::setData(std::unique_ptr<const GeometryTileData> data_, uint64_t correlationID_) {
@@ -92,14 +117,14 @@ void GeometryTileWorker::setData(std::unique_ptr<const GeometryTileData> data_,
 
         switch (state) {
         case Idle:
-            redoLayout();
+            parse();
             coalesce();
             break;
 
         case Coalescing:
-        case NeedLayout:
-        case NeedPlacement:
-            state = NeedLayout;
+        case NeedsParse:
+        case NeedsSymbolLayout:
+            state = NeedsParse;
             break;
         }
     } catch (...) {
@@ -114,16 +139,16 @@ void GeometryTileWorker::setLayers(std::vector<Immutable<Layer::Impl>> layers_,
 
         switch (state) {
         case Idle:
-            redoLayout();
+            parse();
             coalesce();
             break;
 
         case Coalescing:
-        case NeedPlacement:
-            state = NeedLayout;
+        case NeedsSymbolLayout:
+            state = NeedsParse;
             break;
 
-        case NeedLayout:
+        case NeedsParse:
             break;
         }
     } catch (...) {
@@ -138,16 +163,20 @@ void GeometryTileWorker::setShowCollisionBoxes(bool showCollisionBoxes_, uint64_
 
         switch (state) {
         case Idle:
-            attemptPlacement();
-            coalesce();
+            if (!hasPendingParseResult()) {
+                // Trigger parse if nothing is in flight, otherwise symbol layout will automatically
+                // pick up the change
+                parse();
+                coalesce();
+            }
             break;
 
         case Coalescing:
-            state = NeedPlacement;
+            state = NeedsSymbolLayout;
             break;
 
-        case NeedPlacement:
-        case NeedLayout:
+        case NeedsSymbolLayout:
+        case NeedsParse:
             break;
         }
     } catch (...) {
@@ -160,19 +189,23 @@ void GeometryTileWorker::symbolDependenciesChanged() {
         switch (state) {
         case Idle:
             if (symbolLayoutsNeedPreparation) {
-                attemptPlacement();
+                // symbolLayoutsNeedPreparation can only be set true by parsing
+                // and the parse result can only be cleared by performSymbolLayout
+                // which also clears symbolLayoutsNeedPreparation
+                assert(hasPendingParseResult());
+                performSymbolLayout();
                 coalesce();
             }
             break;
 
         case Coalescing:
             if (symbolLayoutsNeedPreparation) {
-                state = NeedPlacement;
+                state = NeedsSymbolLayout;
             }
             break;
 
-        case NeedPlacement:
-        case NeedLayout:
+        case NeedsSymbolLayout:
+        case NeedsParse:
             break;
         }
     } catch (...) {
@@ -191,13 +224,16 @@ void GeometryTileWorker::coalesced() {
             state = Idle;
             break;
 
-        case NeedLayout:
-            redoLayout();
+        case NeedsParse:
+            parse();
             coalesce();
             break;
 
-        case NeedPlacement:
-            attemptPlacement();
+        case NeedsSymbolLayout:
+            // We may have entered NeedsSymbolLayout while coalescing
+            // after a performSymbolLayout. In that case, we need to
+            // start over with parsing in order to do another layout.
+            hasPendingParseResult() ? performSymbolLayout() : parse();
             coalesce();
             break;
         }
@@ -279,7 +315,7 @@ static std::vector<std::unique_ptr<RenderLayer>> toRenderLayers(const std::vecto
     return renderLayers;
 }
 
-void GeometryTileWorker::redoLayout() {
+void GeometryTileWorker::parse() {
     if (!data || !layers) {
         return;
     }
@@ -292,8 +328,8 @@ void GeometryTileWorker::redoLayout() {
     }
 
     std::unordered_map<std::string, std::unique_ptr<SymbolLayout>> symbolLayoutMap;
-    std::unordered_map<std::string, std::shared_ptr<Bucket>> buckets;
-    auto featureIndex = std::make_unique<FeatureIndex>();
+    nonSymbolBuckets.clear();
+    featureIndex = std::make_unique<FeatureIndex>();
     BucketParameters parameters { id, mode, pixelRatio };
 
     GlyphDependencies glyphDependencies;
@@ -352,7 +388,7 @@ void GeometryTileWorker::redoLayout() {
             }
 
             for (const auto& layer : group) {
-                buckets.emplace(layer->getID(), bucket);
+                nonSymbolBuckets.emplace(layer->getID(), bucket);
             }
         }
     }
@@ -368,13 +404,7 @@ void GeometryTileWorker::redoLayout() {
     requestNewGlyphs(glyphDependencies);
     requestNewImages(imageDependencies);
 
-    parent.invoke(&GeometryTile::onLayout, GeometryTile::LayoutResult {
-        std::move(buckets),
-        std::move(featureIndex),
-        *data ? (*data)->clone() : nullptr,
-    }, correlationID);
-
-    attemptPlacement();
+    performSymbolLayout();
 }
 
 bool GeometryTileWorker::hasPendingSymbolDependencies() const {
@@ -385,9 +415,13 @@ bool GeometryTileWorker::hasPendingSymbolDependencies() const {
     }
     return !pendingImageDependencies.empty();
 }
+    
+bool GeometryTileWorker::hasPendingParseResult() const {
+    return bool(featureIndex);
+}
 
-void GeometryTileWorker::attemptPlacement() {
-    if (!data || !layers || hasPendingSymbolDependencies()) {
+void GeometryTileWorker::performSymbolLayout() {
+    if (!data || !layers || !hasPendingParseResult() || hasPendingSymbolDependencies()) {
         return;
     }
     
@@ -435,11 +469,14 @@ void GeometryTileWorker::attemptPlacement() {
     }
 
     firstLoad = false;
-
-    parent.invoke(&GeometryTile::onPlacement, GeometryTile::PlacementResult {
+    
+    parent.invoke(&GeometryTile::onLayout, GeometryTile::LayoutResult {
+        std::move(nonSymbolBuckets),
+        std::move(featureIndex),
+        *data ? (*data)->clone() : nullptr,
         std::move(buckets),
         std::move(glyphAtlasImage),
-        std::move(iconAtlasImage),
+        std::move(iconAtlasImage)
     }, correlationID);
 }
 
diff --git a/src/mbgl/tile/geometry_tile_worker.hpp b/src/mbgl/tile/geometry_tile_worker.hpp
index 0276392679..458ec76988 100644
--- a/src/mbgl/tile/geometry_tile_worker.hpp
+++ b/src/mbgl/tile/geometry_tile_worker.hpp
@@ -8,6 +8,8 @@
 #include <mbgl/util/optional.hpp>
 #include <mbgl/util/immutable.hpp>
 #include <mbgl/style/layer_impl.hpp>
+#include <mbgl/geometry/feature_index.hpp>
+#include <mbgl/renderer/bucket.hpp>
 
 #include <atomic>
 #include <memory>
@@ -43,8 +45,8 @@ public:
 
 private:
     void coalesced();
-    void redoLayout();
-    void attemptPlacement();
+    void parse();
+    void performSymbolLayout();
     
     void coalesce();
 
@@ -53,6 +55,7 @@ private:
    
     void symbolDependenciesChanged();
     bool hasPendingSymbolDependencies() const;
+    bool hasPendingParseResult() const;
 
     ActorRef<GeometryTileWorker> self;
     ActorRef<GeometryTile> parent;
@@ -62,12 +65,15 @@ private:
     const std::atomic<bool>& obsolete;
     const MapMode mode;
     const float pixelRatio;
+    
+    std::unique_ptr<FeatureIndex> featureIndex;
+    std::unordered_map<std::string, std::shared_ptr<Bucket>> nonSymbolBuckets;
 
     enum State {
         Idle,
         Coalescing,
-        NeedLayout,
-        NeedPlacement
+        NeedsParse,
+        NeedsSymbolLayout
     };
 
     State state = Idle;
diff --git a/test/tile/vector_tile.test.cpp b/test/tile/vector_tile.test.cpp
index 9d42f7ae74..cd53c7c4a4 100644
--- a/test/tile/vector_tile.test.cpp
+++ b/test/tile/vector_tile.test.cpp
@@ -65,39 +65,6 @@ TEST(VectorTile, onError) {
     EXPECT_TRUE(tile.isComplete());
 }
 
-TEST(VectorTile, Issue7615) {
-    VectorTileTest test;
-    VectorTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, test.tileset);
-
-    style::SymbolLayer symbolLayer("symbol", "source");
-    std::vector<SymbolInstance> symbolInstances;
-    auto symbolBucket = std::make_shared<SymbolBucket>(
-        style::SymbolLayoutProperties::PossiblyEvaluated(),
-        std::map<
-            std::string,
-            std::pair<style::IconPaintProperties::PossiblyEvaluated, style::TextPaintProperties::PossiblyEvaluated>>(),
-        16.0f, 1.0f, 0.0f, false, false, false, std::move(symbolInstances));
-    
-    // Simulate placement of a symbol layer.
-    tile.onPlacement(GeometryTile::PlacementResult {
-        {{
-            symbolLayer.getID(),
-            symbolBucket
-        }},
-        {},
-        {},
-    }, 0);
-
-    // Subsequent onLayout should not cause the existing symbol bucket to be discarded.
-    tile.onLayout(GeometryTile::LayoutResult {
-        std::unordered_map<std::string, std::shared_ptr<Bucket>>(),
-        nullptr,
-        nullptr,
-    }, 0);
-
-    EXPECT_EQ(symbolBucket.get(), tile.getBucket(*symbolLayer.baseImpl));
-}
-
 TEST(VectorTile, Issue8542) {
     VectorTileTest test;
     VectorTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, test.tileset);