[core] Replace three maps/mutexes in GlyphAtlas with a single map and mutex

4 files changed, 40 insertions, 56 deletions

diff --git a/src/mbgl/layout/symbol_layout.cpp b/src/mbgl/layout/symbol_layout.cpp
index 867b82e285..409128a6fd 100644
--- a/src/mbgl/layout/symbol_layout.cpp
+++ b/src/mbgl/layout/symbol_layout.cpp
@@ -256,7 +256,7 @@ void SymbolLayout::prepare(uintptr_t tileUID,
 
                 // Add the glyphs we need for this label to the glyph atlas.
                 if (result) {
-                    glyphAtlas.addGlyphs(tileUID, text, layout.get<TextFont>(), **glyphSet, face);
+                    glyphAtlas.addGlyphs(tileUID, text, layout.get<TextFont>(), glyphSet, face);
                 }
 
                 return result;
diff --git a/src/mbgl/text/glyph_atlas.cpp b/src/mbgl/text/glyph_atlas.cpp
index 88b319366d..1b3f7518b5 100644
--- a/src/mbgl/text/glyph_atlas.cpp
+++ b/src/mbgl/text/glyph_atlas.cpp
@@ -23,16 +23,17 @@ GlyphAtlas::GlyphAtlas(const Size size, FileSource& fileSource_)
 GlyphAtlas::~GlyphAtlas() = default;
 
 void GlyphAtlas::requestGlyphRange(const FontStack& fontStack, const GlyphRange& range) {
-    std::lock_guard<std::mutex> lock(rangesMutex);
-    auto& rangeSets = ranges[fontStack];
+    std::lock_guard<std::mutex> lock(mutex);
+    auto& rangeSets = entries[fontStack].ranges;
 
     const auto& rangeSetsIt = rangeSets.find(range);
     if (rangeSetsIt != rangeSets.end()) {
         return;
     }
 
-    rangeSets.emplace(range,
-        std::make_unique<GlyphPBF>(this, fontStack, range, observer, fileSource));
+    rangeSets.emplace(std::piecewise_construct,
+                      std::forward_as_tuple(range),
+                      std::forward_as_tuple(this, fontStack, range, observer, fileSource));
 }
 
 bool GlyphAtlas::hasGlyphRanges(const FontStack& fontStack, const GlyphRangeSet& glyphRanges) {
@@ -40,8 +41,8 @@ bool GlyphAtlas::hasGlyphRanges(const FontStack& fontStack, const GlyphRangeSet&
         return true;
     }
 
-    std::lock_guard<std::mutex> lock(rangesMutex);
-    const auto& rangeSets = ranges[fontStack];
+    std::lock_guard<std::mutex> lock(mutex);
+    const auto& rangeSets = entries[fontStack].ranges;
 
     bool hasRanges = true;
     for (const auto& range : glyphRanges) {
@@ -55,7 +56,7 @@ bool GlyphAtlas::hasGlyphRanges(const FontStack& fontStack, const GlyphRangeSet&
             continue;
         }
 
-        if (!rangeSetsIt->second->isParsed()) {
+        if (!rangeSetsIt->second.isParsed()) {
             hasRanges = false;
         }
     }
@@ -64,16 +65,8 @@ bool GlyphAtlas::hasGlyphRanges(const FontStack& fontStack, const GlyphRangeSet&
 }
 
 util::exclusive<GlyphSet> GlyphAtlas::getGlyphSet(const FontStack& fontStack) {
-    auto lock = std::make_unique<std::lock_guard<std::mutex>>(glyphSetsMutex);
-
-    auto it = glyphSets.find(fontStack);
-    if (it == glyphSets.end()) {
-        it = glyphSets.emplace(fontStack, std::make_unique<GlyphSet>()).first;
-    }
-
-    // FIXME: We lock all GlyphSets, but what we should
-    // really do is lock only the one we are returning.
-    return { it->second.get(), std::move(lock) };
+    auto lock = std::make_unique<std::lock_guard<std::mutex>>(mutex);
+    return { &entries[fontStack].glyphSet, std::move(lock) };
 }
 
 void GlyphAtlas::setObserver(GlyphAtlasObserver* observer_) {
@@ -83,12 +76,10 @@ void GlyphAtlas::setObserver(GlyphAtlasObserver* observer_) {
 void GlyphAtlas::addGlyphs(uintptr_t tileUID,
                            const std::u16string& text,
                            const FontStack& fontStack,
-                           const GlyphSet& glyphSet,
+                           const util::exclusive<GlyphSet>& glyphSet,
                            GlyphPositions& face)
 {
-    std::lock_guard<std::mutex> lock(mtx);
-
-    const std::map<uint32_t, SDFGlyph>& sdfs = glyphSet.getSDFs();
+    const std::map<uint32_t, SDFGlyph>& sdfs = glyphSet->getSDFs();
 
     for (char16_t chr : text)
     {
@@ -107,7 +98,7 @@ Rect<uint16_t> GlyphAtlas::addGlyph(uintptr_t tileUID,
                                     const FontStack& fontStack,
                                     const SDFGlyph& glyph)
 {
-    std::map<uint32_t, GlyphValue>& face = index[fontStack];
+    std::map<uint32_t, GlyphValue>& face = entries[fontStack].glyphValues;
     auto it = face.find(glyph.id);
 
     // The glyph is already in this texture.
@@ -151,10 +142,10 @@ Rect<uint16_t> GlyphAtlas::addGlyph(uintptr_t tileUID,
 }
 
 void GlyphAtlas::removeGlyphs(uintptr_t tileUID) {
-    std::lock_guard<std::mutex> lock(mtx);
+    std::lock_guard<std::mutex> lock(mutex);
 
-    for (auto& faces : index) {
-        std::map<uint32_t, GlyphValue>& face = faces.second;
+    for (auto& entry : entries) {
+        std::map<uint32_t, GlyphValue>& face = entry.second.glyphValues;
         for (auto it = face.begin(); it != face.end(); /* we advance in the body */) {
             GlyphValue& value = it->second;
             value.ids.erase(tileUID);
@@ -190,8 +181,6 @@ Size GlyphAtlas::getSize() const {
 }
 
 void GlyphAtlas::upload(gl::Context& context, gl::TextureUnit unit) {
-    std::lock_guard<std::mutex> lock(mtx);
-
     if (!texture) {
         texture = context.createTexture(image, unit);
     } else if (dirty) {
diff --git a/src/mbgl/text/glyph_atlas.hpp b/src/mbgl/text/glyph_atlas.hpp
index 84f29aeb73..8267630096 100644
--- a/src/mbgl/text/glyph_atlas.hpp
+++ b/src/mbgl/text/glyph_atlas.hpp
@@ -17,8 +17,6 @@
 #include <unordered_set>
 #include <unordered_map>
 #include <mutex>
-#include <exception>
-#include <vector>
 
 namespace mbgl {
 
@@ -57,7 +55,7 @@ public:
     void addGlyphs(uintptr_t tileUID,
                    const std::u16string& text,
                    const FontStack&,
-                   const GlyphSet&,
+                   const util::exclusive<GlyphSet>&,
                    GlyphPositions&);
     void removeGlyphs(uintptr_t tileUID);
 
@@ -77,20 +75,9 @@ private:
                             const FontStack&,
                             const SDFGlyph&);
 
-
     FileSource& fileSource;
     std::string glyphURL;
 
-    std::unordered_map<FontStack, std::map<GlyphRange, std::unique_ptr<GlyphPBF>>, FontStackHash> ranges;
-    std::mutex rangesMutex;
-
-    std::unordered_map<FontStack, std::unique_ptr<GlyphSet>, FontStackHash> glyphSets;
-    std::mutex glyphSetsMutex;
-
-    util::WorkQueue workQueue;
-
-    GlyphAtlasObserver* observer = nullptr;
-
     struct GlyphValue {
         GlyphValue(Rect<uint16_t> rect_, uintptr_t id)
             : rect(std::move(rect_)), ids({ id }) {}
@@ -98,9 +85,19 @@ private:
         std::unordered_set<uintptr_t> ids;
     };
 
-    std::mutex mtx;
+    struct Entry {
+        std::map<GlyphRange, GlyphPBF> ranges;
+        GlyphSet glyphSet;
+        std::map<uint32_t, GlyphValue> glyphValues;
+    };
+
+    std::unordered_map<FontStack, Entry, FontStackHash> entries;
+    std::mutex mutex;
+
+    util::WorkQueue workQueue;
+    GlyphAtlasObserver* observer = nullptr;
+
     BinPack<uint16_t> bin;
-    std::unordered_map<FontStack, std::map<uint32_t, GlyphValue>, FontStackHash> index;
     AlphaImage image;
     std::atomic<bool> dirty;
     mbgl::optional<gl::Texture> texture;
diff --git a/test/text/glyph_atlas.test.cpp b/test/text/glyph_atlas.test.cpp
index 1ad2af9d82..3679cabc1b 100644
--- a/test/text/glyph_atlas.test.cpp
+++ b/test/text/glyph_atlas.test.cpp
@@ -83,8 +83,7 @@ TEST(GlyphAtlas, LoadingFail) {
         EXPECT_TRUE(error != nullptr);
         EXPECT_EQ(util::toString(error), "Failed by the test case");
 
-        auto glyphSet = test.glyphAtlas.getGlyphSet({{"Test Stack"}});
-        ASSERT_TRUE(glyphSet->getSDFs().empty());
+        ASSERT_TRUE(test.glyphAtlas.getGlyphSet({{"Test Stack"}})->getSDFs().empty());
         ASSERT_FALSE(test.glyphAtlas.hasGlyphRanges({{"Test Stack"}}, {{0, 255}}));
 
         test.end();
@@ -112,8 +111,7 @@ TEST(GlyphAtlas, LoadingCorrupted) {
         EXPECT_TRUE(error != nullptr);
         EXPECT_EQ(util::toString(error), "unknown pbf field type exception");
 
-        auto glyphSet = test.glyphAtlas.getGlyphSet({{"Test Stack"}});
-        ASSERT_TRUE(glyphSet->getSDFs().empty());
+        ASSERT_TRUE(test.glyphAtlas.getGlyphSet({{"Test Stack"}})->getSDFs().empty());
         ASSERT_FALSE(test.glyphAtlas.hasGlyphRanges({{"Test Stack"}}, {{0, 255}}));
 
         test.end();
@@ -144,21 +142,21 @@ TEST(GlyphAtlas, LoadingCancel) {
 }
 
 TEST(GlyphAtlas, InvalidSDFGlyph) {
-    GlyphSet glyphSet;
-    glyphSet.insert(66, SDFGlyph{ 66 /* ASCII 'B' */,
+    const FontStack fontStack{ "Mock Font" };
+
+    GlyphAtlasTest test;
+    GlyphPositions positions;
+
+    auto glyphSet = test.glyphAtlas.getGlyphSet(fontStack);
+    glyphSet->insert(66, SDFGlyph{ 66 /* ASCII 'B' */,
                                   AlphaImage({7, 7}), /* correct */
                                   { 1 /* width */, 1 /* height */, 0 /* left */, 0 /* top */,
                                     0 /* advance */ } });
-    glyphSet.insert(67, SDFGlyph{ 67 /* ASCII 'C' */,
+    glyphSet->insert(67, SDFGlyph{ 67 /* ASCII 'C' */,
                                   AlphaImage({518, 8}), /* correct */
                                   { 512 /* width */, 2 /* height */, 0 /* left */, 0 /* top */,
                                     0 /* advance */ } });
 
-
-    const FontStack fontStack{ "Mock Font" };
-
-    GlyphAtlasTest test;
-    GlyphPositions positions;
     test.glyphAtlas.addGlyphs(1, std::u16string{u"ABC"}, fontStack, glyphSet, positions);
 
     ASSERT_EQ(2u, positions.size());