perf: inline cell marker to eliminate heap allocation - #5
Draft
jahfer wants to merge 10 commits into
Draft
Conversation
Replace heap-allocated AtomicPtr<Marker<K,V>> with inline atomic fields (AtomicU8 for state, AtomicIsize for move destination). The K,V data in InsertCell/DeleteCell markers was never read by other threads - only the operation state matters for synchronization. This eliminates all heap allocations from the cell operation hot path, particularly benefiting rebalance-heavy workloads. Benchmark improvements: - Random inserts: +63% to +281% throughput - Point lookups: +5% to +9% at larger dataset sizes - Mixed workloads: +8% to +14% - Sequential inserts: +2% to +32%
Relax SeqCst atomic orderings to Acquire on read paths for improved performance, especially on ARM/AArch64 where SeqCst requires full memory barriers. Changes: - CellGuard::from_raw(): version, marker_state, move_dest loads → Acquire - CellGuard::cache(): version load → Acquire - btree_map rebalance: source/dest cell reads → Acquire The seqlock validation re-read in from_raw() remains SeqCst to prevent reordering of data reads past the validation check on weakly-ordered architectures. Acquire ordering is sufficient because we only need pairwise synchronization between a writer and its readers for each cell, not total ordering across unrelated cells. Added 6 tests to verify Acquire semantics work correctly.
Replace expensive key.clone() in CellGuard::from_raw() with a simple is_some() check. The actual key/value cloning is now deferred to cache() when the data is actually needed. This optimization reduces overhead in the hot iteration path where code frequently checks is_empty() to find empty slots or skip cells without needing their actual contents. The seqlock validation still protects against torn reads since the is_some() check happens within the version-validated window. Before: clone key on every from_raw() call (~10% overhead) After: check is_some() in from_raw(), clone only in cache() Added tests verifying deferred cloning behavior.
Change version.store() and marker_state.store() calls after UnsafeCell writes from SeqCst to Release ordering in the insert paths of btree_map.rs. Release ordering is sufficient because it ensures all preceding writes (including UnsafeCell writes to key/value) are visible to any thread that subsequently performs an Acquire load on these atomics. This is the "publication" side of the Release/Acquire pattern - we don't need total ordering with writes to other cells, only pairwise synchronization with readers of this cell. Changes: - 6 stores relaxed: lines ~140, ~144, ~174, ~178, ~223, ~227 - Added 5 tests verifying Release/Acquire synchronization semantics All 116 tests pass. Step 2 of atomic ordering relaxation plan.
Change compare_exchange and compare_exchange_weak orderings from SeqCst/SeqCst to AcqRel/Acquire for marker_state CAS operations. Changes: - cell.rs: update_marker_state() CAS now uses AcqRel/Acquire - btree_map.rs: Move marker CAS uses AcqRel/Acquire - btree_map.rs: Best-effort cleanup CAS (version + marker) uses AcqRel/Acquire Rationale: - Success (AcqRel): Acquire synchronizes with prior writer's Release, Release publishes our claim to subsequent readers - Failure (Acquire): Observes the blocking value; no write on failure so Release adds nothing Added 5 tests verifying AcqRel/Acquire CAS semantics: - test_cas_acqrel_synchronizes_with_prior_writes - test_cas_failure_observes_blocking_value - test_rebalance_cas_publishes_move_marker - test_best_effort_cleanup_cas_correctness - test_cas_release_sequence_with_move_dest
Step 4 of atomic ordering relaxation plan: Change move_dest.store() from SeqCst to Release in both btree_map.rs and cell.rs. Release ordering is sufficient because move_dest is stored before the CAS that sets marker_state to Move. The CAS uses AcqRel ordering, creating a release sequence that readers synchronize with via Acquire on marker_state. Changes: - btree_map.rs: move_dest.store() in rebalance loop - cell.rs: move_dest.store() in update_marker_state() Tests added: - test_move_dest_release_visible_after_marker_acquire - test_move_dest_release_prevents_reorder_after_cas - test_update_marker_state_move_dest_release - test_concurrent_readers_observe_move_dest
Change destination cell version.store() and marker_state.store() from SeqCst to Release ordering in rebalance (lines ~502-509). Release ensures preceding UnsafeCell writes (key/value) are visible to readers who Acquire-load these atomics.
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.
Summary
This PR eliminates heap allocation for cell markers by inlining atomic fields directly into the
Cellstruct. Previously, each cell stored a pointer to a heap-allocatedMarker<K, V>enum, requiring an indirection on every read/write operation. Now, the marker state is stored as inline atomic fields within the cell itself.Motivation
Profiling revealed that the heap-allocated marker was a significant source of overhead:
AtomicPtrto read the marker stateBox::new()andBox::into_raw()callsChanges
Before
After
Key Insight
The
InsertCell(version, K, V)andDeleteCell(version, K)marker variants stored key/value data that was never read by other threads—they only served as intent markers. Only the operation state and themove_destindex (for rebalancing) need to be atomically visible. This allows us to replace the polymorphic heap-allocated enum with fixed-size inline atomics.Benchmark Results
All benchmarks run on the same hardware, comparing before/after this change.
Point Lookups (Read Performance)
Sequential Inserts
Random Inserts (Most Dramatic Improvement)
Mixed Workload (80% Read / 20% Write)
Strided Access
Why Random Inserts Improved So Dramatically
Random inserts trigger frequent rebalancing operations. The old implementation:
Marker::Move(version, dest_index)viaBox::new()The new implementation:
compare_exchangeon theAtomicU8marker stateAtomicIsizeThis eliminates all heap allocations from the hot path of cell operations.
Testing
All 103 existing tests pass. New tests added for:
MarkerStateenum conversion and methodsCell::load_marker()reconstructionCellGuardversion change detection on cache accessBreaking Changes
Cell::new()no longer takes a marker pointer argumentCellGuard::update()replaced withupdate_marker_state(MarkerState, isize)Marker<K, V>enum variantsInsertCellandDeleteCellnow usePhantomDatainstead of storing actual key/value data (backwards compatibility preserved for pattern matching)