From 044454417b61bdd102a376c1125ad6ee3a5eacd4 Mon Sep 17 00:00:00 2001 From: "Thiago Marcos P. Santos" Date: Wed, 17 Jun 2015 16:41:36 +0300 Subject: [PATCH] Do not hold a reference to the Style at the [Live|Vector]TileData Layers are added and removed dynamically on the Map thread when we use shape annotations and we are iterating style.layers on the Worker thread without any lock. Now when we create a [Live|Vector]TileData at the Map thread we give it a layers list (instead of the Style) and [Live|Vector]TileData will ultimately make a copy of this list (on the Map thread, so no concurrency with adding shape annotations). The copy should be relatively cheap because we are using a shared pointer for storing the layers on the list. --- src/mbgl/map/live_tile_data.cpp | 7 ++++--- src/mbgl/map/live_tile_data.hpp | 3 ++- src/mbgl/map/source.cpp | 4 ++-- src/mbgl/map/tile_parser.cpp | 7 +++---- src/mbgl/map/tile_parser.hpp | 7 ++++--- src/mbgl/map/vector_tile_data.cpp | 16 ++++++++-------- src/mbgl/map/vector_tile_data.hpp | 7 ++++--- 7 files changed, 27 insertions(+), 24 deletions(-) diff --git a/src/mbgl/map/live_tile_data.cpp b/src/mbgl/map/live_tile_data.cpp index 4935c013451..ccae542ee2c 100644 --- a/src/mbgl/map/live_tile_data.cpp +++ b/src/mbgl/map/live_tile_data.cpp @@ -10,7 +10,8 @@ using namespace mbgl; LiveTileData::LiveTileData(const TileID& id_, AnnotationManager& annotationManager_, - Style& style_, + const std::vector>& layers_, + Worker& workers_, GlyphAtlas& glyphAtlas_, GlyphStore& glyphStore_, SpriteAtlas& spriteAtlas_, @@ -18,7 +19,7 @@ LiveTileData::LiveTileData(const TileID& id_, const SourceInfo& source_, float angle_, bool collisionDebug_) - : VectorTileData::VectorTileData(id_, style_, glyphAtlas_, glyphStore_, + : VectorTileData::VectorTileData(id_, layers_, workers_, glyphAtlas_, glyphStore_, spriteAtlas_, sprite_, source_, angle_, collisionDebug_), annotationManager(annotationManager_) { // live features are always ready @@ -43,7 +44,7 @@ void LiveTileData::parse() { // Parsing creates state that is encapsulated in TileParser. While parsing, // the TileParser object writes results into this objects. All other state // is going to be discarded afterwards. - TileParser parser(*tile, *this, style, glyphAtlas, glyphStore, spriteAtlas, sprite); + TileParser parser(*tile, *this, layers, glyphAtlas, glyphStore, spriteAtlas, sprite); parser.parse(); } catch (const std::exception& ex) { Log::Error(Event::ParseTile, "Live-parsing [%d/%d/%d] failed: %s", id.z, id.x, id.y, ex.what()); diff --git a/src/mbgl/map/live_tile_data.hpp b/src/mbgl/map/live_tile_data.hpp index 16206712301..57d2e8c0d3f 100644 --- a/src/mbgl/map/live_tile_data.hpp +++ b/src/mbgl/map/live_tile_data.hpp @@ -11,7 +11,8 @@ class LiveTileData : public VectorTileData { public: LiveTileData(const TileID&, AnnotationManager&, - Style&, + const std::vector>&, + Worker&, GlyphAtlas&, GlyphStore&, SpriteAtlas&, diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp index 7fb015c1da5..663f1d38167 100644 --- a/src/mbgl/map/source.cpp +++ b/src/mbgl/map/source.cpp @@ -294,7 +294,7 @@ TileData::State Source::addTile(MapData& data, // If we don't find working tile data, we're just going to load it. if (info.type == SourceType::Vector) { new_tile.data = - std::make_shared(normalized_id, style, glyphAtlas, + std::make_shared(normalized_id, style.layers, style.workers, glyphAtlas, glyphStore, spriteAtlas, sprite, info, transformState.getAngle(), data.getCollisionDebug()); new_tile.data->request(style.workers, transformState.getPixelRatio(), callback); @@ -304,7 +304,7 @@ TileData::State Source::addTile(MapData& data, style.workers, transformState.getPixelRatio(), callback); } else if (info.type == SourceType::Annotations) { new_tile.data = std::make_shared(normalized_id, data.annotationManager, - style, glyphAtlas, + style.layers, style.workers, glyphAtlas, glyphStore, spriteAtlas, sprite, info, transformState.getAngle(), data.getCollisionDebug()); new_tile.data->reparse(style.workers, callback); diff --git a/src/mbgl/map/tile_parser.cpp b/src/mbgl/map/tile_parser.cpp index 61af227034b..1ddf07ae9e9 100644 --- a/src/mbgl/map/tile_parser.cpp +++ b/src/mbgl/map/tile_parser.cpp @@ -8,7 +8,6 @@ #include #include #include -#include #include @@ -21,14 +20,14 @@ TileParser::~TileParser() = default; TileParser::TileParser(const GeometryTile& geometryTile_, VectorTileData& tile_, - const Style& style_, + const std::vector>& layers_, GlyphAtlas& glyphAtlas_, GlyphStore& glyphStore_, SpriteAtlas& spriteAtlas_, const util::ptr& sprite_) : geometryTile(geometryTile_), tile(tile_), - style(style_), + layers(layers_), glyphAtlas(glyphAtlas_), glyphStore(glyphStore_), spriteAtlas(spriteAtlas_), @@ -42,7 +41,7 @@ bool TileParser::obsolete() const { } void TileParser::parse() { - for (const auto& layer_desc : style.layers) { + for (const auto& layer_desc : layers) { // Cancel early when parsing. if (obsolete()) { return; diff --git a/src/mbgl/map/tile_parser.hpp b/src/mbgl/map/tile_parser.hpp index 88e56878045..7eadc45d463 100644 --- a/src/mbgl/map/tile_parser.hpp +++ b/src/mbgl/map/tile_parser.hpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -22,7 +23,6 @@ class GlyphAtlas; class GlyphStore; class SpriteAtlas; class Sprite; -class Style; class StyleBucket; class StyleLayoutFill; class StyleLayoutRaster; @@ -34,7 +34,7 @@ class TileParser : private util::noncopyable { public: TileParser(const GeometryTile& geometryTile, VectorTileData& tile, - const Style& style, + const std::vector>& layers, GlyphAtlas& glyphAtlas, GlyphStore& glyphStore, SpriteAtlas& spriteAtlas, @@ -61,8 +61,9 @@ class TileParser : private util::noncopyable { const GeometryTile& geometryTile; VectorTileData& tile; + std::vector> layers; + // Cross-thread shared data. - const Style& style; GlyphAtlas& glyphAtlas; GlyphStore& glyphStore; SpriteAtlas& spriteAtlas; diff --git a/src/mbgl/map/vector_tile_data.cpp b/src/mbgl/map/vector_tile_data.cpp index 6dc6e8e9b35..017d3ecb7c5 100644 --- a/src/mbgl/map/vector_tile_data.cpp +++ b/src/mbgl/map/vector_tile_data.cpp @@ -9,12 +9,12 @@ #include #include #include -#include using namespace mbgl; VectorTileData::VectorTileData(const TileID& id_, - Style& style_, + const std::vector>& layers_, + Worker& workers_, GlyphAtlas& glyphAtlas_, GlyphStore& glyphStore_, SpriteAtlas& spriteAtlas_, @@ -23,11 +23,12 @@ VectorTileData::VectorTileData(const TileID& id_, float angle, bool collisionDebug) : TileData(id_, source_), + layers(layers_), + workers(workers_), glyphAtlas(glyphAtlas_), glyphStore(glyphStore_), spriteAtlas(spriteAtlas_), sprite(sprite_), - style(style_), collision(std::make_unique(id_.z, 4096, source_.tile_size * id.overscaling, angle, collisionDebug)), lastAngle(angle), currentAngle(angle) { @@ -51,7 +52,7 @@ void VectorTileData::parse() { // is going to be discarded afterwards. VectorTile vectorTile(pbf((const uint8_t *)data.data(), data.size())); const VectorTile* vt = &vectorTile; - TileParser parser(*vt, *this, style, glyphAtlas, glyphStore, spriteAtlas, sprite); + TileParser parser(*vt, *this, layers, glyphAtlas, glyphStore, spriteAtlas, sprite); parser.parse(); if (getState() == State::obsolete) { @@ -128,15 +129,14 @@ void VectorTileData::redoPlacement(float angle, bool collisionDebug) { currentCollisionDebug = collisionDebug; auto callback = std::bind(&VectorTileData::endRedoPlacement, this); - workRequest = style.workers.send([this, angle, collisionDebug] { workerRedoPlacement(angle, collisionDebug); }, callback); - + workRequest = workers.send([this, angle, collisionDebug] { workerRedoPlacement(angle, collisionDebug); }, callback); } } void VectorTileData::workerRedoPlacement(float angle, bool collisionDebug) { collision->reset(angle, 0); collision->setDebug(collisionDebug); - for (const auto& layer_desc : style.layers) { + for (const auto& layer_desc : layers) { auto bucket = getBucket(*layer_desc); if (bucket) { bucket->placeFeatures(); @@ -145,7 +145,7 @@ void VectorTileData::workerRedoPlacement(float angle, bool collisionDebug) { } void VectorTileData::endRedoPlacement() { - for (const auto& layer_desc : style.layers) { + for (const auto& layer_desc : layers) { auto bucket = getBucket(*layer_desc); if (bucket) { bucket->swapRenderData(); diff --git a/src/mbgl/map/vector_tile_data.hpp b/src/mbgl/map/vector_tile_data.hpp index 5a6f448c69c..51a627d7b73 100644 --- a/src/mbgl/map/vector_tile_data.hpp +++ b/src/mbgl/map/vector_tile_data.hpp @@ -26,14 +26,14 @@ class GlyphAtlas; class GlyphStore; class SpriteAtlas; class Sprite; -class Style; class VectorTileData : public TileData { friend class TileParser; public: VectorTileData(const TileID&, - Style&, + const std::vector>&, + Worker&, GlyphAtlas&, GlyphStore&, SpriteAtlas&, @@ -66,11 +66,12 @@ class VectorTileData : public TileData { TriangleElementsBuffer triangleElementsBuffer; LineElementsBuffer lineElementsBuffer; + std::vector> layers; + Worker& workers; GlyphAtlas& glyphAtlas; GlyphStore& glyphStore; SpriteAtlas& spriteAtlas; util::ptr sprite; - Style& style; private: // Contains all the Bucket objects for the tile. Buckets are render