Synchronize attribute key cache refresh#898
Open
Symmetricity wants to merge 1 commit into
Open
Conversation
AttributeKeyStore publishes new keys to worker-local caches while PBF reading runs in parallel. The previous code advanced keys2indexSize before appending the matching key and refreshed the thread-local cache without holding keys2indexMutex, so another worker could read the deque while it was being mutated. Take the existing mutex before refreshing the thread-local cache and publish the new key only after it is appended to the shared store. Add a threaded AttributeKeyStore test for the concurrent lookup/insert path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is AI generated.
Summary
This synchronizes
AttributeKeyStore's thread-local key cache refresh with theshared key store mutation path.
AttributeKeyStore::key2index()is called while PBF blocks are processed inparallel. The previous code allowed worker threads to refresh their
thread-local
keys2indexcache from the sharedkeysdeque without holdingkeys2indexMutex. It also advancedkeys2indexSizebefore appending thecorresponding key to
keys.That left a window where another worker could observe a newly published key
count before the matching
keys[]entry was safely available, or while thedeque/map was being mutated by another thread.
Evidence
While investigating an intermittent Windows
--storecrash, a full ProcDumpcapture showed an access violation in the attribute key cache path:
The failing instruction was inside an MSVC
std::stringcomparison used by thestd::map<const std::string*, ...>comparator inAttributeKeyStore. One sideof the comparison was a null/invalid string pointer, consistent with a worker
thread caching an invalid
&keys[index]pointer.Reproduction signal from the Windows VM:
--storerepro produced a ProcDump crash dumpon run 15 with
0xC0000005without producing an access-violation dump
This is also a plausible source of non-crashing downstream issues: if the stale
or invalid cache path does not crash immediately, it can return the wrong
attribute key index or corrupt the key lookup path used later when serializing
feature attributes.
Implementation
The patch keeps the existing data structures and synchronization model, but
closes the unsafe window:
keys2indexMutexbefore refreshing the thread-local cache fromkeyskeys2indexmap before publishingthe new
keys2indexSizeThis means worker-local caches only observe key indexes that have already been
fully installed in the shared store.
Testing
The branch adds
test_attribute_key_store_threaded, which exercises concurrentlookup and insertion into a single
AttributeKeyStorefrom multiple threads.The test checks that:
0Recommended local checks:
git diff --check origin/master..fix/attribute-key-cache-race cmake -S . -B local-builds/attribute-key-cache-race -DCMAKE_BUILD_TYPE=RelWithDebInfo cmake --build local-builds/attribute-key-cache-race --target attribute_store.test -j2 ./local-builds/attribute-key-cache-race/tests/attribute_store.testThe original crash is scheduling-sensitive, so the unit test is a focused
concurrency regression rather than a guaranteed pre-fix crash reproducer.
Performance
I also compared the submitted PR stack with and without this patch on a warmed
Austria Geofabrik extract using the OpenMapTiles profile, PMTiles output, and
--threads 4. The comparison used three alternating runs for each build.All six runs exited successfully. The timings are close enough that I would not
claim a speedup from this change, but the test did not show a meaningful memory
or runtime regression from synchronizing the cache refresh path.
Possible Regressions
This adds a mutex hold around the thread-local cache refresh path after a cache
miss. The hot path remains unchanged when the key is already present in the
thread-local cache.
Expected behavior changes:
corruption or inconsistent key lookup
newly added keys