From 5900d69e92e54b4d0cf5fca813c0274929aa14eb Mon Sep 17 00:00:00 2001 From: Stefan Karschti Date: Wed, 20 Sep 2023 19:04:19 +0300 Subject: [PATCH] Fix string indexer reallocation bug (#1666) --- include/mbgl/util/string_indexer.hpp | 7 ++++-- src/mbgl/gl/drawable_gl.cpp | 2 +- src/mbgl/mtl/drawable.cpp | 3 ++- src/mbgl/util/string_indexer.cpp | 32 ++++++++++++++++++------ test/util/string_indexer.test.cpp | 37 +++++++++++++++++++++++++++- 5 files changed, 68 insertions(+), 13 deletions(-) diff --git a/include/mbgl/util/string_indexer.hpp b/include/mbgl/util/string_indexer.hpp index 322030351..8cd5afbd3 100644 --- a/include/mbgl/util/string_indexer.hpp +++ b/include/mbgl/util/string_indexer.hpp @@ -21,7 +21,7 @@ public: static StringIdentity get(std::string_view); - static const std::string& get(const StringIdentity id); + static std::string get(const StringIdentity id); static void clear(); @@ -32,7 +32,8 @@ protected: ~StringIndexer() = default; using MapType = std::unordered_map; - using VectorType = std::vector; + using VectorType = std::vector; + using BufferType = std::vector; static StringIndexer& instance() { static StringIndexer inst; @@ -41,6 +42,8 @@ protected: MapType stringToIdentity; VectorType identityToString; + BufferType buffer; + std::shared_mutex sharedMutex; }; diff --git a/src/mbgl/gl/drawable_gl.cpp b/src/mbgl/gl/drawable_gl.cpp index 38acb733d..f9adc544e 100644 --- a/src/mbgl/gl/drawable_gl.cpp +++ b/src/mbgl/gl/drawable_gl.cpp @@ -123,7 +123,7 @@ void DrawableGL::bindUniformBuffers() const { if (!uniformBuffer) { using namespace std::string_literals; Log::Error(Event::General, - "DrawableGL::bindUniformBuffers: UBO "s + StringIndexer::get(element.first) + + "DrawableGL::bindUniformBuffers: UBO "s + std::string(StringIndexer::get(element.first)) + " not found. skipping."); assert(false); continue; diff --git a/src/mbgl/mtl/drawable.cpp b/src/mbgl/mtl/drawable.cpp index 99c06685e..19beb4305 100644 --- a/src/mbgl/mtl/drawable.cpp +++ b/src/mbgl/mtl/drawable.cpp @@ -287,9 +287,10 @@ void Drawable::setVertices(std::vector&& data, std::size_t count, gfx:: attrib->setRawData(std::move(data)); attrib->setStride(VertexAttribute::getStrideOf(type)); } else { + using namespace std::string_literals; Log::Warning( Event::General, - "Vertex attribute type mismatch: " + name + " / " + StringIndexer::get(impl->idVertexAttrName)); + "Vertex attribute type mismatch: "s + name + " / " + StringIndexer::get(impl->idVertexAttrName)); assert(false); } } diff --git a/src/mbgl/util/string_indexer.cpp b/src/mbgl/util/string_indexer.cpp index b59710d0a..d5e8ea112 100644 --- a/src/mbgl/util/string_indexer.cpp +++ b/src/mbgl/util/string_indexer.cpp @@ -1,17 +1,20 @@ #include #include +#include namespace mbgl { namespace { const std::string empty; constexpr std::size_t initialCapacity = 100; +constexpr std::size_t initialBufferCapacity = initialCapacity * 32; } // namespace StringIndexer::StringIndexer() { stringToIdentity.reserve(initialCapacity); identityToString.reserve(initialCapacity); + buffer.reserve(initialBufferCapacity); } StringIdentity StringIndexer::get(std::string_view string) { @@ -36,13 +39,25 @@ StringIdentity StringIndexer::get(std::string_view string) { if (const auto it = stringToIdentity.find(string); it == stringToIdentity.end()) { // this writer to insert + auto& buffer = instance().buffer; + const auto previousCapacity = buffer.capacity(); + const StringIdentity id = identityToString.size(); - identityToString.push_back(std::string(string)); - - [[maybe_unused]] auto result = stringToIdentity.insert( - {std::string_view(identityToString.back().data(), identityToString.back().length()), id}); - assert(result.second); - + identityToString.push_back(buffer.size()); + buffer.insert(buffer.end(), string.begin(), string.end()); + buffer.push_back(0); + if (buffer.capacity() != previousCapacity) { + // reallocation happened + stringToIdentity.clear(); + for (auto iIdentity = identityToString.begin(); iIdentity != identityToString.end(); ++iIdentity) { + stringToIdentity[std::string_view(buffer.data() + *iIdentity)] = iIdentity - + identityToString.begin(); + } + } else { + [[maybe_unused]] auto result = stringToIdentity.insert( + {std::string_view(buffer.data() + identityToString.back()), id}); + assert(result.second); + } return id; } else { // another writer inserted into the map @@ -51,13 +66,13 @@ StringIdentity StringIndexer::get(std::string_view string) { } } -const std::string& StringIndexer::get(const StringIdentity id) { +std::string StringIndexer::get(const StringIdentity id) { std::shared_lock readerLock(instance().sharedMutex); const auto& identityToString = instance().identityToString; assert(id < identityToString.size()); - return id < identityToString.size() ? identityToString[id] : empty; + return id < identityToString.size() ? (instance().buffer.data() + identityToString[id]) : empty; } void StringIndexer::clear() { @@ -65,6 +80,7 @@ void StringIndexer::clear() { instance().stringToIdentity.clear(); instance().identityToString.clear(); + instance().buffer.clear(); } size_t StringIndexer::size() { diff --git a/test/util/string_indexer.test.cpp b/test/util/string_indexer.test.cpp index cf4c6eb1d..f852992dd 100644 --- a/test/util/string_indexer.test.cpp +++ b/test/util/string_indexer.test.cpp @@ -30,7 +30,7 @@ TEST(StringIndexer, GetString) { EXPECT_EQ(id1, 0); EXPECT_EQ(StringIndexer::size(), 1); - const std::string str1 = StringIndexer::get(id1); + const auto str1 = StringIndexer::get(id1); EXPECT_EQ(str, str1); const auto str2 = str; @@ -38,13 +38,48 @@ TEST(StringIndexer, GetString) { EXPECT_EQ(id1, id2); } +TEST(StringIndexer, Reallocate) { + StringIndexer::clear(); + EXPECT_EQ(StringIndexer::size(), 0); + + constexpr auto str = "reallocate test string1"; + + const auto id1 = StringIndexer::get(str); + EXPECT_EQ(id1, 0); + EXPECT_EQ(StringIndexer::size(), 1); + + // force reallocation. assume capacity: 100 strings, 100 * 32 bytes buffer space + constexpr auto N = 1000; + auto string_for_i = [](int i) -> std::string { + using namespace std::string_literals; + return "1234567890"s + "1234567890"s + "1234567890"s + "---" + std::to_string(i); + }; + for (auto i = 1; i < N; ++i) { + StringIndexer::get(string_for_i(i)); + } + + EXPECT_EQ(StringIndexer::size(), N); + + const auto str1 = StringIndexer::get(id1); + EXPECT_EQ(str, str1); + + for (auto i = 1; i < N; ++i) { + const auto strN1 = StringIndexer::get(i); + EXPECT_EQ(strN1, string_for_i(i)); + } +} + TEST(StringIndexer, GetOOBIdentity) { StringIndexer::clear(); EXPECT_EQ(StringIndexer::size(), 0); std::string str; +#ifndef NDEBUG EXPECT_DEATH_IF_SUPPORTED(str = StringIndexer::get(42), "id < identityToString.size()"); +#endif EXPECT_TRUE(str.empty()); +#ifndef NDEBUG EXPECT_DEATH_IF_SUPPORTED(str = StringIndexer::get(-1), "id < identityToString.size()"); +#endif EXPECT_TRUE(str.empty()); }