Fix LeapMap migration bugs and add concurrent benchmarks#22
Open
mandrean wants to merge 3 commits intorobclu:mainfrom
Open
Fix LeapMap migration bugs and add concurrent benchmarks#22mandrean wants to merge 3 commits intorobclu:mainfrom
mandrean wants to merge 3 commits intorobclu:mainfrom
Conversation
Add benches/leapmap.rs with three benchmark groups, each comparing
LeapMap against DashMap and the appropriate lock-based baseline:
concurrent_insert
LeapMap vs DashMap vs Mutex<HashMap>
N threads (available_parallelism), each inserting 10 000 unique keys
into a fresh map.
concurrent_get
LeapMap vs DashMap vs RwLock<HashMap>
N threads performing 10 000 random reads on a map pre-populated with
16 384 entries (setup excluded from timing).
resize/{1k,10k,100k}
LeapMap vs DashMap, single-threaded fill from empty to N elements
with no pre-allocated capacity, exercising the full resize/migration
path including Drop cleanup.
DashMap is added as a dev-dependency so results are directly comparable
to a popular alternative. The existing benches/hashmap.rs is unchanged.
Three fixes for the LeapMap concurrent migration machinery: 1. **Livelock fix**: `prepare_retry_after_overflow` no longer stores `status = INITIALIZATION_MASK`. Previously, non-last workers spinning on `while status >= 1` would never exit because INITIALIZATION_MASK (0x06) >= 1. The last worker now drives the retry migration single-handedly via `drive_retry_migration` while other workers spin, then stores `status = 0` to release them. 2. **TOCTOU race fix**: Added an initialization-flag guard in `perform_migration`'s CAS join loop. Without this, a thread could pass `initialization_complete()` in `participate_in_migration`, but by the time it CAS'd `status + 8`, the migration had already completed (status = 0). The CAS `0 → 8` would succeed, creating a phantom worker that corrupts the worker count (prev_status != 15) and leads to heap corruption. 3. **Infinite spin fix**: The `while migrator.finishing()` loop in `participate_in_migration` now checks the completion flag directly instead of calling `finishing()` (which returns true for status == 0, causing an infinite loop when the migration completes while a thread is in the cleanup spin).
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.
Initially I just wanted to benchmark
LeapMapagainstDashMap(similar to https://github.com/robclu/leapfrog/blob/main/benches/hashmap.rs), but that actually crashed. So I started digging further into it, and found some bugs:Summary
bucket_slice/bucket_slice_mut(slices created 4x past allocation boundary)dashmapas a dev-dependency for side-by-side comparisonBug fixes
Memory ordering (
be22a87)Multiple
Relaxedatomics in theMigratorupgraded to properAcquire/Release/AcqRel:set_initialization_begin_flagset_initialization_complete_flagfinishing(),initialization_complete(),in_process()status.fetch_or(1)(overflow and completion)overflowedflag and migration resultsstale_sourcescleanupswap(AcqRel)current_stale_sourceCASbucket_sliceUB fix (be22a87)Table::bucket_slice()andbucket_slice_mut()created slices withself.size()elements (number of cells), but onlyself.size() >> 2buckets are allocated. Fixed toself.size() >> 2.Note:
hashmap.rshas the identical bug at lines 967-972, left for a follow-up.Migration retry livelock (
be22a87introduced,f8cd76cfixed)prepare_retry_after_overflowstoredstatus = INITIALIZATION_MASK (0x06)to signal a retry round, but non-last workers spin onwhile status >= 1expectingstatus == 0. Since0x06 >= 1, they spin forever under sustained concurrent inserts.Fix: removed the status store from
prepare_retry_after_overflow. The last worker now drives the retry migration single-handedly viadrive_retry_migration()while other workers spin, then storesstatus = 0to release them.TOCTOU race in
perform_migration(f8cd76c)A thread could pass
initialization_complete()inparticipate_in_migration, but by the time it reached the CASstatus + 8inperform_migration, the migration had completed (status = 0). The CAS0 -> 8succeeded, creating a phantom worker that corrupted the count (prev_status = 0x08instead of0x0F), leading to heap corruption and SIGABRT.Fix: added an
INITIALIZATION_MASKguard in the CAS join loop. Bail out if init flags are not set.Infinite spin in
participate_in_migration(f8cd76c)while migrator.finishing()looped forever whenstatus == 0becausefinishing()returnstrueforstatus == 0. Threads entering this path after a migration completed would never exit.Fix: check
status & MIGRATION_COMPLETE_FLAGdirectly instead of callingfinishing().Benchmarks
benches/leapmap.rsadds three benchmark groups, each comparing LeapMap against DashMap and the appropriate lock-based baseline:Results with my M1 Max 64GB (10 cores):
LeapMap is roughly 2.6x faster than DashMap for concurrent reads and competitive for concurrent inserts. Single-threaded resize is slower as expected (lock-free migration overhead amortizes under concurrency).
Test plan
overflow_retry_keeps_migration_live)hashmapbenchmark