fix(sync): drain AccessBuffer in insert() so reads carry eviction weight + docs - #24
Merged
Merged
Conversation
…on weight ConcurrentPulseMap::get() pushed access events into AccessBuffer, but nothing in production ever called drain() — reads had no effect on eviction, so concurrent maps evicted as if every key were cold. Measured cost at equal 16,384-entry capacity (key space 10x, Zipf 1.3, 99% reads): TypedPulseMap 96.76% vs ConcurrentPulseMap 95.60%, a 1.16-point gap; the peek() control (reads deliberately unweighted) matched the concurrent map exactly, confirming the read path was the only difference. Fix: a multi-consumer drain() that claims a range with one strong CAS on tail (no retry — a failed claim means another drain owns the range; skipped events stay queued, within the buffer's existing lossy contract), wired into insert_internal with DRAIN_BATCH = 64 so each insert applies up to 64 queued reads before its own eviction decision. The drain runs before the inserting thread takes its BucketGuard, so a drained event never locks the target bucket while another lock is held — no new lock ordering. ShardedPulseMap wraps ConcurrentPulseMap shards, each with its own buffer, so it inherits the fix unchanged. After: TypedPulseMap and ConcurrentPulseMap both 95.372% ± 0.011 — the gap is gone, and the drain is what closed it: the peek() control stays at 94.456% ± 0.012, so read weight now adds the +0.92 points it should. examples/drain_latency.rs measures the insert-path cost (mixed 99% get 4.38 Mops/s vs 99% peek 3.08 — cost-negative but bounded); the drain is on the write path, which the concurrent maps were already losing at, so read latency is untouched. tests/loom_access_buffer.rs gains concurrent_drains_deliver_every_event_ exactly_once: two threads drain the same buffer under loom; every pushed event is delivered to exactly one consumer. Both new examples get required-features = ["std"] like the other ten; without the guard, cargo test --no-default-features tries to build them and fails, which would have broken CI.
ConcurrentPulseMap::get() pushes access events into the AccessBuffer; since the insert-path drain landed, those reads actually reach the eviction policy. Document the mechanism where it was previously only implied, and the measured effect where nothing was published: - concurrency.md / api-concurrent.md: state when deferred reads are applied (insert drains up to DRAIN_BATCH = 64 events before its eviction decision, before taking the bucket spinlock), and that the buffer is lossy under drain races — skipped events stay queued. - README.md: new subsection under Eviction Quality with the concurrent harness numbers (TypedPulseMap 95.372% vs ConcurrentPulseMap 95.372% +/- 0.011, peek control 94.456% +/- 0.012), explicitly marked as not comparable to the single-threaded table above it — different workload shape and concurrency; the benchmark's point is the gap between the two maps, which was 1.16 points before the drain and is now zero.
The drain applied on_access CAS without the target bucket's lock, so a concurrent insert/get/remove holding a &mut Bucket on the same bucket raced it (Miri: data race on the Bucket allocation). Every &mut retag site takes BucketGuard, so the CAS must too. No ABBA: the drain runs before insert's own guard and AccessBuffer is lock-free.
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
1 participant
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.
What
ConcurrentPulseMap::get()pushes access events into the AccessBuffer, butnothing ever drained it — so the concurrent map evicted as if every key were
cold, sitting 1.16 hit-rate points below TypedPulseMap. This PR wires a
multi-consumer
drain()(one strong CAS on the tail, lossy contract) intoinsert_internal, draining up to DRAIN_BATCH = 64 events before the bucketspinlock is taken — no new lock ordering, read path untouched. ShardedPulseMap
inherits it via its shards.
Carries two commits:
96c82c3the fix itselfba21b28docs recording the mechanism (concurrency.md, api-concurrent.md)and the measured hit-rate effect (README: new concurrent hit-rate subsection)
Measured effect
Dedicated harness (
examples/hitrate_16384.rs, capacity 16384, keyspace163840, Zipf 1.3, 2M ops, 5 trials):
get()peek()controlRead weight itself is worth +0.92 points (get vs peek), and the drain runs on
the write path only — read latency untouched (
examples/drain_latency.rs,measured separately).
Single-threaded behavior is unchanged: the published Scenario D table
(96.73% ± 0.01%) reproduces exactly on this build, and the read-ratio sweep
(80/20, 99/1) holds at 96.74–96.75%.
Notes
for a later insert to pick up — documented in api-concurrent.md.
non-comparable (different workload shape and concurrency) and was not
overwritten.