systemed / tilemaker

Make OpenStreetMap vector tiles without the stack
https://tilemaker.org/
Other
1.5k stars 232 forks source link

thread-safety fixes: PooledString, layer metadata #761

Closed cldellow closed 2 months ago

cldellow commented 2 months ago

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 PooledStrings 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 #746.

systemed commented 2 months ago

Good sleuthing! And I can't see any performance hit from this. Thank you.