tei/MapLibreParticleLab
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix string indexer reallocation bug (#1666)

Browse Source
This commit is contained in:
Stefan Karschti
2023-09-20 19:04:19 +03:00
committed by GitHub
parent 713edd2c8e
commit 5900d69e92
5 changed files with 68 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
include/mbgl/util/string_indexer.hpp
View File

@@ -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<std::string_view, StringIdentity>;
using VectorType = std::vector<std::string>;
using VectorType = std::vector<std::size_t>;
using BufferType = std::vector<char>;
static StringIndexer& instance() {
static StringIndexer inst;
@@ -41,6 +42,8 @@ protected:
MapType stringToIdentity;
VectorType identityToString;
BufferType buffer;
std::shared_mutex sharedMutex;
};

2
src/mbgl/gl/drawable_gl.cpp
View File

@@ -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;

3
src/mbgl/mtl/drawable.cpp
View File

@@ -287,9 +287,10 @@ void Drawable::setVertices(std::vector<uint8_t>&& 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);
}
}

32
src/mbgl/util/string_indexer.cpp
View File

@@ -1,17 +1,20 @@
#include <mbgl/util/string_indexer.hpp>
#include <cassert>
#include <memory>
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<std::shared_mutex> 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() {

37
test/util/string_indexer.test.cpp
View File

@@ -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());
}