Hang on each tile load — VectorTileData, TileWorker::parseLayer

include/mbgl/util/run_loop.hpp

@@ -1,8 +1,11 @@
 #ifndef MBGL_UTIL_RUN_LOOP
 #define MBGL_UTIL_RUN_LOOP
 
+#include <mbgl/platform/log.hpp>
+#include <mbgl/util/chrono.hpp>
 #include <mbgl/util/noncopyable.hpp>
 #include <mbgl/util/util.hpp>
+#include <mbgl/util/thread_context.hpp>
 #include <mbgl/util/work_task.hpp>
 #include <mbgl/util/work_request.hpp>
 
@@ -139,8 +142,33 @@ class RunLoop : private util::noncopyable {
         // If the task has completed and the after callback has executed, this will
         // do nothing.
         void cancel() override {
+#if defined(DEBUG)
+            auto now = Clock::now();
+
+            if (!tryCancel()) {
+                std::lock_guard<std::recursive_mutex> lock(mutex);
+                *canceled = true;
+
+                using std::chrono::duration_cast;
+                long elapsed = duration_cast<Milliseconds>(Clock::now() - now).count();
+
+                if (ThreadContext::currentlyOn(ThreadType::Map)) {
+                    Log::Warning(mbgl::Event::General, "Map thread blocked for %ld ms.", elapsed);
+                }
+            };
+#else
             std::lock_guard<std::recursive_mutex> lock(mutex);
             *canceled = true;
+#endif
+        }
+
+        bool tryCancel() override {
+            if (mutex.try_lock()) {
+                *canceled = true;
+                mutex.unlock();
+                return true;
+            }
+            return false;
         }
 
     private:

include/mbgl/util/work_request.hpp

@@ -15,6 +15,8 @@ class WorkRequest : public util::noncopyable {
     WorkRequest(Task);
     ~WorkRequest();
 
+    bool tryCancel();
+
 private:
     std::shared_ptr<WorkTask> task;
 };

include/mbgl/util/work_task.hpp

@@ -14,6 +14,7 @@ class WorkTask : private util::noncopyable {
 
     virtual void operator()() = 0;
     virtual void cancel() = 0;
+    virtual bool tryCancel() = 0;
 };
 
 } // namespace mbgl

src/mbgl/map/raster_tile_data.cpp

@@ -17,7 +17,8 @@ RasterTileData::RasterTileData(const TileID& id_,
 }
 
 RasterTileData::~RasterTileData() {
-    cancel();
+    assert(tryCancel());
+    workRequest.reset();
 }
 
 void RasterTileData::request(const std::string& url,
@@ -56,7 +57,9 @@ void RasterTileData::request(const std::string& url,
 
         workRequest.reset();
         workRequest = worker.parseRasterTile(std::make_unique<RasterBucket>(texturePool), res.data, [this, callback] (RasterTileParseResult result) {
+            WorkCompletedNotifier notifier(this);
             workRequest.reset();
+
             if (state != State::loaded) {
                 return;
             }
@@ -80,10 +83,20 @@ Bucket* RasterTileData::getBucket(StyleLayer const&) {
     return bucket.get();
 }
 
-void RasterTileData::cancel() {
+bool RasterTileData::tryCancel(bool force) {
     if (state != State::obsolete) {
         state = State::obsolete;
     }
+
     req = nullptr;
-    workRequest.reset();
+
+    if (force) {
+        workRequest.reset();
+    }
+
+    if (workRequest) {
+        return workRequest->tryCancel();
+    }
+
+    return true;
 }

src/mbgl/map/raster_tile_data.hpp

@@ -21,7 +21,7 @@ class RasterTileData : public TileData {
     void request(const std::string& url,
                  const Callback& callback);
 
-    void cancel() override;
+    bool tryCancel(bool force = false) override;
 
     Bucket* getBucket(StyleLayer const &layer_desc) override;
 

src/mbgl/map/source.cpp

@@ -267,9 +267,11 @@ TileData::State Source::addTile(const TileID& tileID, const StyleUpdateParameter
 
         // If we don't find working tile data, we're just going to load it.
         if (type == SourceType::Raster) {
-            auto tileData = std::make_shared<RasterTileData>(normalizedID,
-                                                             parameters.texturePool,
-                                                             parameters.worker);
+            auto tileData = std::shared_ptr<RasterTileData>(new RasterTileData{
+                normalizedID,
+                parameters.texturePool,
+                parameters.worker
+            }, [this](TileData* data) { tileDataDeleter.add(data); });
             tileData->request(util::templateTileURL(info->tiles.at(0), normalizedID, parameters.pixelRatio), callback);
             newTile->data = tileData;
         } else {
@@ -286,12 +288,14 @@ TileData::State Source::addTile(const TileID& tileID, const StyleUpdateParameter
                 return TileData::State::invalid;
             }
 
-            newTile->data = std::make_shared<VectorTileData>(normalizedID,
-                                                             std::move(monitor),
-                                                             id,
-                                                             parameters.style,
-                                                             parameters.mode,
-                                                             callback);
+            newTile->data = std::shared_ptr<VectorTileData>(new VectorTileData{
+                normalizedID,
+                std::move(monitor),
+                id,
+                parameters.style,
+                parameters.mode,
+                callback
+            }, [this](TileData* data) { tileDataDeleter.add(data); });
         }
 
         tileDataMap.emplace(newTile->data->id, newTile->data);
@@ -485,7 +489,7 @@ bool Source::update(const StyleUpdateParameters& parameters) {
         bool obsolete = retain_data.find(tile->id) == retain_data.end();
         if (obsolete) {
             if (!tileCache.has(tile->id.normalized().to_uint64())) {
-                tile->cancel();
+                tile->tryCancel();
             }
             return true;
         } else {

src/mbgl/map/source.hpp

@@ -2,6 +2,7 @@
 #define MBGL_MAP_SOURCE
 
 #include <mbgl/map/tile_cache.hpp>
+#include <mbgl/map/tile_data_deleter.hpp>
 #include <mbgl/map/source_info.hpp>
 
 #include <mbgl/util/mat4.hpp>
@@ -111,6 +112,8 @@ class Source : private util::noncopyable {
 
     Observer nullObserver;
     Observer* observer = &nullObserver;
+
+    TileDataDeleter tileDataDeleter;
 };
 
 } // namespace mbgl

src/mbgl/map/tile_data.hpp

@@ -21,6 +21,13 @@ class DebugBucket;
 
 class TileData : private util::noncopyable {
 public:
+    class Observer {
+    public:
+        virtual ~Observer() = default;
+
+        virtual void onTileDataWorkCompleted(TileData*) {};
+    };
+
     // initial:
     //   Initial state, only used when the TileData object is created.
     //
@@ -73,8 +80,11 @@ class TileData : private util::noncopyable {
     TileData(const TileID&);
     virtual ~TileData();
 
-    // Mark this tile as no longer needed and cancel any pending work.
-    virtual void cancel() = 0;
+    // Mark this tile as no longer needed and cancel any pending work. Returns
+    // true if the work was canceled, false otherwise. Canceling is not always
+    // possible because in some circumstances it might block. Force will force
+    // the cancellation and always return true, but it can potentially block.
+    virtual bool tryCancel(bool force = false) = 0;
 
     virtual Bucket* getBucket(const StyleLayer&) = 0;
 
@@ -92,6 +102,10 @@ class TileData : private util::noncopyable {
 
     void dumpDebugLogs() const;
 
+    void setObserver(Observer* observer_) {
+        observer = observer_;
+    }
+
     const TileID id;
     optional<SystemTimePoint> modified;
     optional<SystemTimePoint> expires;
@@ -100,7 +114,21 @@ class TileData : private util::noncopyable {
     std::unique_ptr<DebugBucket> debugBucket;
 
 protected:
+    class WorkCompletedNotifier {
+    public:
+        WorkCompletedNotifier(TileData* data_) : data(data_) {}
+        ~WorkCompletedNotifier() {
+            data->observer->onTileDataWorkCompleted(data);
+        }
+
+    private:
+        TileData* data;
+    };
+
     std::atomic<State> state;
+
+    Observer nullObserver;
+    Observer* observer = &nullObserver;
 };
 
 } // namespace mbgl

src/mbgl/map/tile_data_deleter.cpp

@@ -0,0 +1,34 @@
+#include <mbgl/map/tile_data_deleter.hpp>
+
+#include <mbgl/map/tile_data.hpp>
+
+namespace mbgl {
+
+TileDataDeleter::~TileDataDeleter() {
+    // Force the canceling of all pending TileData
+    // before destroying. We have assert()s on Debug
+    // mode on TileData to make sure it doesn't block
+    // at the destructor unless explicitly canceled.
+    for (auto& entry : tileDataMap) {
+        entry.first->tryCancel(true);
+    }
+}
+
+void TileDataDeleter::add(TileData* data) {
+    std::unique_ptr<TileData> tileData(data);
+
+    // This custom deleter will try to cancel the task if it is
+    // non-blocking, otherwise we keep it and subscribe to the
+    // event when the work is completed. When the event triggers
+    // we delete the TileData because this time it won't block.
+    if (!tileData->tryCancel()) {
+        data->setObserver(this);
+        tileDataMap.emplace(data, std::move(tileData));
+    }
+}
+
+void TileDataDeleter::onTileDataWorkCompleted(TileData* data) {
+    tileDataMap.erase(data);
+}
+
+} // namespace mbgl

src/mbgl/map/tile_data_deleter.hpp

@@ -0,0 +1,29 @@
+#ifndef MBGL_MAP_TILE_DATA_DELETER
+#define MBGL_MAP_TILE_DATA_DELETER
+
+#include <mbgl/map/tile_data.hpp>
+#include <mbgl/util/noncopyable.hpp>
+
+#include <memory>
+#include <unordered_map>
+
+namespace mbgl {
+
+class TileData;
+
+class TileDataDeleter : public TileData::Observer, private util::noncopyable {
+public:
+    virtual ~TileDataDeleter();
+
+    void add(TileData*);
+
+    // TileData::Observer implementation.
+    void onTileDataWorkCompleted(TileData*) override;
+
+private:
+    std::unordered_map<TileData*, std::unique_ptr<TileData>> tileDataMap;
+};
+
+} // namespace mbgl
+
+#endif

src/mbgl/map/vector_tile_data.cpp

@@ -24,7 +24,8 @@ VectorTileData::VectorTileData(const TileID& id_,
                  *style_.glyphStore,
                  state,
                  mode_),
-      monitor(std::move(monitor_))
+      monitor(std::move(monitor_)),
+      delayedRedoPlacement([this]{ delayedRedoPlacementFunction(); })
 {
     state = State::loading;
     tileRequest = monitor->monitorTile([callback, this](std::exception_ptr err,
@@ -59,6 +60,7 @@ VectorTileData::VectorTileData(const TileID& id_,
         // request in case there is one.
         workRequest.reset();
         workRequest = worker.parseGeometryTile(tileWorker, style.getLayers(), std::move(tile), targetConfig, [callback, this, config = targetConfig] (TileParseResult result) {
+            WorkCompletedNotifier notifier(this);
             workRequest.reset();
             if (state == State::obsolete) {
                 return;
@@ -88,7 +90,8 @@ VectorTileData::VectorTileData(const TileID& id_,
 }
 
 VectorTileData::~VectorTileData() {
-    cancel();
+    assert(tryCancel());
+    workRequest.reset();
 }
 
 bool VectorTileData::parsePending(std::function<void(std::exception_ptr)> callback) {
@@ -97,8 +100,8 @@ bool VectorTileData::parsePending(std::function<void(std::exception_ptr)> callba
         return false;
     }
 
-    workRequest.reset();
     workRequest = worker.parsePendingGeometryTileLayers(tileWorker, targetConfig, [this, callback, config = targetConfig] (TileParseResult result) {
+        WorkCompletedNotifier notifier(this);
         workRequest.reset();
         if (state == State::obsolete) {
             return;
@@ -153,8 +156,8 @@ void VectorTileData::redoPlacement(const PlacementConfig newConfig, const std::f
 }
 
 void VectorTileData::redoPlacement(const std::function<void()>& callback) {
-    workRequest.reset();
-    workRequest = worker.redoPlacement(tileWorker, buckets, targetConfig, [this, callback, config = targetConfig] {
+    auto redoPlacementCallback = [this, callback, config = targetConfig] {
+        WorkCompletedNotifier notifier(this);
         workRequest.reset();
 
         // Persist the configuration we just placed so that we can later check whether we need to
@@ -172,13 +175,36 @@ void VectorTileData::redoPlacement(const std::function<void()>& callback) {
         } else {
             callback();
         }
-    });
+    };
+
+    // We overwrite any existing function as
+    // we only care about the last placement.
+    delayedRedoPlacementFunction = [this, redoPlacementCallback] {
+        if (workRequest) {
+            delayedRedoPlacement.send();
+        } else {
+            workRequest = worker.redoPlacement(
+                tileWorker, buckets, targetConfig, redoPlacementCallback);
+            delayedRedoPlacementFunction = nullptr;
+        }
+    };
+
+    delayedRedoPlacementFunction();
 }
 
-void VectorTileData::cancel() {
+bool VectorTileData::tryCancel(bool force) {
     state = State::obsolete;
     tileRequest.reset();
-    workRequest.reset();
+
+    if (force) {
+        workRequest.reset();
+    }
+
+    if (workRequest) {
+        return workRequest->tryCancel();
+    }
+
+    return true;
 }
 
 } // namespace mbgl