From 2a09100cb78c8728a6288a6de1368e5263cbf819 Mon Sep 17 00:00:00 2001 From: Tim Sylvester Date: Thu, 30 May 2024 10:08:53 -0700 Subject: [PATCH] RasterTile / mailbox fix (#2443) --- src/mbgl/actor/mailbox.cpp | 22 ++++++++++------ src/mbgl/tile/geometry_tile.hpp | 6 ++--- src/mbgl/tile/raster_dem_tile.cpp | 42 +++++++++++++++++++++++------- src/mbgl/tile/raster_dem_tile.hpp | 7 +++++ src/mbgl/tile/raster_tile.cpp | 42 +++++++++++++++++++++++------- src/mbgl/tile/raster_tile.hpp | 7 +++++ src/mbgl/tile/tile.cpp | 2 -- src/mbgl/tile/tile.hpp | 2 +- src/mbgl/tile/tile_loader.hpp | 4 +-- src/mbgl/tile/tile_loader_impl.hpp | 17 +++++++++--- src/mbgl/tile/vector_tile.cpp | 10 +++++++ src/mbgl/tile/vector_tile.hpp | 1 + test/style/source.test.cpp | 2 ++ 13 files changed, 124 insertions(+), 40 deletions(-) diff --git a/src/mbgl/actor/mailbox.cpp b/src/mbgl/actor/mailbox.cpp index ad63f4bc5..55ba8899e 100644 --- a/src/mbgl/actor/mailbox.cpp +++ b/src/mbgl/actor/mailbox.cpp @@ -20,15 +20,17 @@ void Mailbox::open(Scheduler& scheduler_) { std::lock_guard receivingLock(receivingMutex); std::lock_guard pushingLock(pushingMutex); - weakScheduler = scheduler_.makeWeakPtr(); - if (closed) { return; } + weakScheduler = scheduler_.makeWeakPtr(); + if (!queue.empty()) { auto guard = weakScheduler.lock(); - if (weakScheduler) weakScheduler->schedule(makeClosure(shared_from_this())); + if (weakScheduler) { + weakScheduler->schedule(makeClosure(shared_from_this())); + } } } @@ -45,6 +47,8 @@ void Mailbox::close() { std::lock_guard pushingLock(pushingMutex); closed = true; + + weakScheduler = {}; } void Mailbox::abandon() { @@ -57,7 +61,7 @@ void Mailbox::abandon() { } bool Mailbox::isOpen() const { - return bool(weakScheduler); + return bool(weakScheduler) && !closed; } void Mailbox::push(std::unique_ptr message) { @@ -105,9 +109,6 @@ void Mailbox::receive() { }}; std::lock_guard receivingLock(receivingMutex); - auto guard = weakScheduler.lock(); - assert(weakScheduler); - if (closed) { state = State::Abandoned; return; @@ -126,8 +127,13 @@ void Mailbox::receive() { (*message)(); + // If there are more messages in the queue and the scheduler + // is still active, create a new task to handle the next one if (!wasEmpty) { - weakScheduler->schedule(makeClosure(shared_from_this())); + auto guard = weakScheduler.lock(); + if (weakScheduler) { + weakScheduler->schedule(makeClosure(shared_from_this())); + } } } diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp index 31aa70b66..1948983d6 100644 --- a/src/mbgl/tile/geometry_tile.hpp +++ b/src/mbgl/tile/geometry_tile.hpp @@ -100,12 +100,12 @@ protected: const GeometryTileData* getData() const; LayerRenderData* getLayerRenderData(const style::Layer::Impl&); -private: - void markObsolete(); - // Used to signal the worker that it should abandon parsing this tile as soon as possible. std::atomic obsolete{false}; +private: + void markObsolete(); + const std::shared_ptr threadPool; const std::shared_ptr mailbox; diff --git a/src/mbgl/tile/raster_dem_tile.cpp b/src/mbgl/tile/raster_dem_tile.cpp index bbdaaeab8..5628c8aea 100644 --- a/src/mbgl/tile/raster_dem_tile.cpp +++ b/src/mbgl/tile/raster_dem_tile.cpp @@ -17,6 +17,7 @@ namespace mbgl { RasterDEMTile::RasterDEMTile(const OverscaledTileID& id_, const TileParameters& parameters, const Tileset& tileset) : Tile(Kind::RasterDEM, id_), loader(*this, id_, parameters, tileset), + threadPool(Scheduler::GetBackground()), mailbox(std::make_shared(*Scheduler::GetCurrent())), worker(Scheduler::GetBackground(), ActorRef(*this, mailbox)) { encoding = tileset.encoding; @@ -31,7 +32,14 @@ RasterDEMTile::RasterDEMTile(const OverscaledTileID& id_, const TileParameters& } } -RasterDEMTile::~RasterDEMTile() = default; +RasterDEMTile::~RasterDEMTile() { + markObsolete(); + + // The bucket has resources that need to be released on the render thread. + if (bucket) { + threadPool->runOnRenderThread([bucket_{std::move(bucket)}]() {}); + } +} std::unique_ptr RasterDEMTile::createRenderData() { return std::make_unique>(bucket); @@ -48,19 +56,23 @@ void RasterDEMTile::setMetadata(std::optional modified_, std::optiona } void RasterDEMTile::setData(const std::shared_ptr& data) { - pending = true; - ++correlationID; - worker.self().invoke(&RasterDEMTileWorker::parse, data, correlationID, encoding); + if (!obsolete) { + pending = true; + ++correlationID; + worker.self().invoke(&RasterDEMTileWorker::parse, data, correlationID, encoding); + } } void RasterDEMTile::onParsed(std::unique_ptr result, const uint64_t resultCorrelationID) { - bucket = std::move(result); - loaded = true; - if (resultCorrelationID == correlationID) { - pending = false; + if (!obsolete) { + bucket = std::move(result); + loaded = true; + if (resultCorrelationID == correlationID) { + pending = false; + } + renderable = static_cast(bucket); + observer->onTileChanged(*this); } - renderable = static_cast(bucket); - observer->onTileChanged(*this); } void RasterDEMTile::onError(std::exception_ptr err, const uint64_t resultCorrelationID) { @@ -125,4 +137,14 @@ void RasterDEMTile::setUpdateParameters(const TileUpdateParameters& params) { loader.setUpdateParameters(params); } +void RasterDEMTile::cancel() { + markObsolete(); +} + +void RasterDEMTile::markObsolete() { + obsolete = true; + pending = false; + mailbox->abandon(); +} + } // namespace mbgl diff --git a/src/mbgl/tile/raster_dem_tile.hpp b/src/mbgl/tile/raster_dem_tile.hpp index 337dd3701..644360d11 100644 --- a/src/mbgl/tile/raster_dem_tile.hpp +++ b/src/mbgl/tile/raster_dem_tile.hpp @@ -86,9 +86,14 @@ public: void onParsed(std::unique_ptr result, uint64_t correlationID); void onError(std::exception_ptr, uint64_t correlationID); + void cancel() override; + private: + void markObsolete(); + TileLoader loader; + const std::shared_ptr threadPool; std::shared_ptr mailbox; Actor worker; @@ -98,6 +103,8 @@ private: // Contains the Bucket object for the tile. Buckets are render // objects and they get added by tile parsing operations. std::shared_ptr bucket; + + bool obsolete = false; }; } // namespace mbgl diff --git a/src/mbgl/tile/raster_tile.cpp b/src/mbgl/tile/raster_tile.cpp index 58192e5e0..702a06d59 100644 --- a/src/mbgl/tile/raster_tile.cpp +++ b/src/mbgl/tile/raster_tile.cpp @@ -17,10 +17,18 @@ namespace mbgl { RasterTile::RasterTile(const OverscaledTileID& id_, const TileParameters& parameters, const Tileset& tileset) : Tile(Kind::Raster, id_), loader(*this, id_, parameters, tileset), + threadPool(Scheduler::GetBackground()), mailbox(std::make_shared(*Scheduler::GetCurrent())), worker(Scheduler::GetBackground(), ActorRef(*this, mailbox)) {} -RasterTile::~RasterTile() = default; +RasterTile::~RasterTile() { + markObsolete(); + + // The bucket has resources that need to be released on the render thread. + if (bucket) { + threadPool->runOnRenderThread([bucket_{std::move(bucket)}]() {}); + } +} std::unique_ptr RasterTile::createRenderData() { return std::make_unique>(bucket); @@ -37,19 +45,23 @@ void RasterTile::setMetadata(std::optional modified_, std::optional& data) { - pending = true; - ++correlationID; - worker.self().invoke(&RasterTileWorker::parse, data, correlationID); + if (!obsolete) { + pending = true; + ++correlationID; + worker.self().invoke(&RasterTileWorker::parse, data, correlationID); + } } void RasterTile::onParsed(std::unique_ptr result, const uint64_t resultCorrelationID) { - bucket = std::move(result); - loaded = true; - if (resultCorrelationID == correlationID) { - pending = false; + if (!obsolete) { + bucket = std::move(result); + loaded = true; + if (resultCorrelationID == correlationID) { + pending = false; + } + renderable = static_cast(bucket); + observer->onTileChanged(*this); } - renderable = static_cast(bucket); - observer->onTileChanged(*this); } void RasterTile::onError(std::exception_ptr err, const uint64_t resultCorrelationID) { @@ -78,4 +90,14 @@ void RasterTile::setUpdateParameters(const TileUpdateParameters& params) { loader.setUpdateParameters(params); } +void RasterTile::cancel() { + markObsolete(); +} + +void RasterTile::markObsolete() { + obsolete = true; + pending = false; + mailbox->abandon(); +} + } // namespace mbgl diff --git a/src/mbgl/tile/raster_tile.hpp b/src/mbgl/tile/raster_tile.hpp index b0cc46e25..18c3dd878 100644 --- a/src/mbgl/tile/raster_tile.hpp +++ b/src/mbgl/tile/raster_tile.hpp @@ -35,9 +35,14 @@ public: void onParsed(std::unique_ptr result, uint64_t correlationID); void onError(std::exception_ptr, uint64_t correlationID); + void cancel() override; + private: + void markObsolete(); + TileLoader loader; + const std::shared_ptr threadPool; std::shared_ptr mailbox; Actor worker; @@ -46,6 +51,8 @@ private: // Contains the Bucket object for the tile. Buckets are render // objects and they get added by tile parsing operations. std::shared_ptr bucket; + + std::atomic obsolete{false}; }; } // namespace mbgl diff --git a/src/mbgl/tile/tile.cpp b/src/mbgl/tile/tile.cpp index 7b174d44b..306a86e1c 100644 --- a/src/mbgl/tile/tile.cpp +++ b/src/mbgl/tile/tile.cpp @@ -19,8 +19,6 @@ void Tile::setObserver(TileObserver* observer_) { observer = observer_; } -void Tile::cancel() {} - void Tile::setTriedCache() { triedOptional = true; observer->onTileChanged(*this); diff --git a/src/mbgl/tile/tile.hpp b/src/mbgl/tile/tile.hpp index c487155d5..73ab8140e 100644 --- a/src/mbgl/tile/tile.hpp +++ b/src/mbgl/tile/tile.hpp @@ -68,7 +68,7 @@ public: virtual void setUpdateParameters(const TileUpdateParameters&) {} // Mark this tile as no longer needed and cancel any pending work. - virtual void cancel(); + virtual void cancel() = 0; // Notifies this tile of the updated layer properties. // diff --git a/src/mbgl/tile/tile_loader.hpp b/src/mbgl/tile/tile_loader.hpp index 6c07af8fd..f728a169f 100644 --- a/src/mbgl/tile/tile_loader.hpp +++ b/src/mbgl/tile/tile_loader.hpp @@ -62,8 +62,8 @@ private: std::atomic_bool aborted{false}; }; - // Allocated as a share_ptr so either the loader or request can outlive the - // other and still see this. + // Allocated as a `shared_ptr` so either the loader or + // request can outlive the other and still see this. std::shared_ptr shared; }; diff --git a/src/mbgl/tile/tile_loader_impl.hpp b/src/mbgl/tile/tile_loader_impl.hpp index 68781d378..ff9315b10 100644 --- a/src/mbgl/tile/tile_loader_impl.hpp +++ b/src/mbgl/tile/tile_loader_impl.hpp @@ -59,11 +59,20 @@ TileLoader::TileLoader(T& tile_, template TileLoader::~TileLoader() { - std::unique_lock lock(shared->requestLock); - shared->aborted = true; - tile.cancel(); + // Tasks may outlive this object, make sure that they are not + // currently running and do not run if they are still pending. + { + std::unique_lock lock(shared->requestLock); + shared->aborted = true; + } + + // Don't touch `tile` here, we may be called from its destructor + // tile.cancel(); + request.reset(); -}; + fileSource.reset(); + shared.reset(); +} template void TileLoader::setNecessity(TileNecessity newNecessity) { diff --git a/src/mbgl/tile/vector_tile.cpp b/src/mbgl/tile/vector_tile.cpp index 736c6ff9e..989c86fb1 100644 --- a/src/mbgl/tile/vector_tile.cpp +++ b/src/mbgl/tile/vector_tile.cpp @@ -13,6 +13,12 @@ VectorTile::VectorTile(const OverscaledTileID& id_, : GeometryTile(id_, std::move(sourceID_), parameters), loader(*this, id_, parameters, tileset) {} +VectorTile::~VectorTile() { + // Don't rely on `~TileLoader` to close, it's not safe to call there. + // We're still calling a virtual method from a destructor, so any overrides will not be called. + GeometryTile::cancel(); +} + void VectorTile::setNecessity(TileNecessity necessity) { loader.setNecessity(necessity); } @@ -27,6 +33,10 @@ void VectorTile::setMetadata(std::optional modified_, std::optional& data_) { + if (obsolete) { + return; + } + GeometryTile::setData(data_ ? std::make_unique(data_) : nullptr); } diff --git a/src/mbgl/tile/vector_tile.hpp b/src/mbgl/tile/vector_tile.hpp index d703a3b70..788e61996 100644 --- a/src/mbgl/tile/vector_tile.hpp +++ b/src/mbgl/tile/vector_tile.hpp @@ -11,6 +11,7 @@ class TileParameters; class VectorTile : public GeometryTile { public: VectorTile(const OverscaledTileID&, std::string sourceID, const TileParameters&, const Tileset&); + ~VectorTile() override; void setNecessity(TileNecessity) final; void setUpdateParameters(const TileUpdateParameters&) final; diff --git a/test/style/source.test.cpp b/test/style/source.test.cpp index 55655d0fd..8be5ed674 100644 --- a/test/style/source.test.cpp +++ b/test/style/source.test.cpp @@ -772,6 +772,8 @@ public: std::unique_ptr createRenderData() override { return nullptr; } + void cancel() override {} + private: FakeTileSource& source; };