Skip to content

Commit

Permalink
thread-safety fixes: PooledString, layer metadata
Browse files Browse the repository at this point in the history
bug 1: PooledString resizes `vector` without locks

`tables` is a shared pool of `char*` pointers, where each pointer
points to a 64KB memory chunk.

Some `PooledString`s identify their content by an index into this pool.

However, `tables` can grow. We correctly guard against concurrent
mutation (for example, here:
https://github.com/systemed/tilemaker/blob/7f0343045687ab2125910c81eed598c58fc2ff2d/src/pooled_string.cpp#L33-L39)

But readers expect to be able to read it without a lock, for example
here, where the result of a read will be used to do a write: https://github.com/systemed/tilemaker/blob/7f0343045687ab2125910c81eed598c58fc2ff2d/src/pooled_string.cpp#L54

This pattern isn't safe with `vector`, since when the `vector` grows,
it invalidates all existing pointers. It is safe with `deque`, so the fix
is to switch to a `deque`.

bug 2: vector layer metadata `map` isn't guarded

`layers` is a shared object common to all OsmLuaProcessing threads.

`layers.layers` is a `vector` that gets initialized and populated fully on
the main thread before the Lua threads start, so accessing it without
locks is fine.

`layers.layers[layer].attributeMap` is just a vanilla `map`, though,
so mutating it from multiple threads without locks is dangerous.

I just added a coarse lock for now. On my 16-core machine, it didn't
seem to introduce contention, so I didn't bother to do anything fancier
to minimize locking overhead.

I will optimistically say that this fixes systemed#746.
  • Loading branch information
cldellow committed Sep 21, 2024
1 parent 7f03430 commit 928a886
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 1 deletion.
3 changes: 3 additions & 0 deletions src/osm_lua_processing.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ const std::string EMPTY_STRING = "";
thread_local kaguya::State *g_luaState = nullptr;
thread_local OsmLuaProcessing* osmLuaProcessing = nullptr;

std::mutex vectorLayerMetadataMutex;

void handleOsmLuaProcessingUserSignal(int signum) {
osmLuaProcessing->handleUserSignal(signum);
}
Expand Down Expand Up @@ -913,6 +915,7 @@ std::string OsmLuaProcessing::FindInRelation(const std::string &key) {

// Record attribute name/type for vector_layers table
void OsmLuaProcessing::setVectorLayerMetadata(const uint_least8_t layer, const string &key, const uint type) {
std::lock_guard<std::mutex> lock(vectorLayerMetadataMutex);
layers.layers[layer].attributeMap[key] = type;
}

Expand Down
3 changes: 2 additions & 1 deletion src/pooled_string.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
#include <stdexcept>
#include <mutex>
#include <cstring>
#include <deque>

namespace PooledStringNS {
std::vector<char*> tables;
std::deque<char*> tables;
std::mutex mutex;

const uint8_t ShortString = 0b00;
Expand Down

0 comments on commit 928a886

Please sign in to comment.