tp: prove StringPool read path is concurrent-safe under locking - #7344
Open
LalitMaganti wants to merge 1 commit into
Open
tp: prove StringPool read path is concurrent-safe under locking#7344LalitMaganti wants to merge 1 commit into
LalitMaganti wants to merge 1 commit into
Conversation
LalitMaganti
force-pushed
the
dev/lalitm/string-pool-concurrent-reads
branch
from
September 4, 2026 17:06
76e729a to
850aa56
Compare
LalitMaganti
force-pushed
the
dev/lalitm/string-pool-concurrent-reads
branch
from
September 4, 2026 17:06
850aa56 to
59a29ab
Compare
LalitMaganti
marked this pull request as ready for review
September 4, 2026 17:08
🎨 Perfetto UI Builds
|
sashwinbalaji
approved these changes
Sep 4, 2026
Base automatically changed from
dev/lalitm/dataframe-concurrent-cursors
to
main
September 4, 2026 17:51
Adds two ThreadSanitizer stress tests proving the StringPool read path is safe for the multi-connection design, where string columns are read on every query thread: - ConcurrentReadsNoInterns: 8 threads concurrently Get()/GetId() a finalized pool (small, block-spanning and >1MB large strings). - ConcurrentReadsWhileLockedInterns: with locking enabled, reader threads do lock-free Get() of pre-existing ids while writer threads continuously intern ~2KB strings, forcing several real 4MB-block allocations during the read window (the new-block publication path), plus occasional large-string appends. The test asserts the bytes of every pre-existing id stay intact and self-validates that block growth actually occurred. No production fix was required: the lock-free Get() is safe against concurrent interns because |blocks_| is a fixed array that never reallocates (a new block writes a different slot), block bytes are written forward-only before the Id is published, and large strings are fully mutex-guarded. Documents this concurrency contract on set_locking() — including that concurrent interns from multiple threads are serialized by the mutex and supported (the prior draft contract wrongly stated otherwise).
LalitMaganti
force-pushed
the
dev/lalitm/string-pool-concurrent-reads
branch
from
September 4, 2026 17:51
59a29ab to
b2d4fa9
Compare
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.
Adds two ThreadSanitizer stress tests proving the StringPool read path
is safe for the multi-connection design, where string columns are read
on every query thread:
finalized pool (small, block-spanning and >1MB large strings).
threads do lock-free Get() of pre-existing ids while writer threads
continuously intern ~2KB strings, forcing several real 4MB-block
allocations during the read window (the new-block publication path),
plus occasional large-string appends. The test asserts the bytes of
every pre-existing id stay intact and self-validates that block
growth actually occurred.
No production fix was required: the lock-free Get() is safe against
concurrent interns because |blocks_| is a fixed array that never
reallocates (a new block writes a different slot), block bytes are
written forward-only before the Id is published, and large strings are
fully mutex-guarded. Documents this concurrency contract on
set_locking() — including that concurrent interns from multiple threads
are serialized by the mutex and supported (the prior draft contract
wrongly stated otherwise).