perf(snapshot): chunk for_each_versioned to bound lock-hold under high cardinality - #225
Open
JustMaier wants to merge 2 commits into
Open
perf(snapshot): chunk for_each_versioned to bound lock-hold under high cardinality#225JustMaier wants to merge 2 commits into
JustMaier wants to merge 2 commits into
Conversation
…h cardinality save_snapshot's filter loop iterated FilterField.for_each_versioned which holds the bitmaps read lock for the duration of the iteration. For postId at 22.5M values this is ~2 seconds of read-lock-held, during which the apply path (a writer) waits. Add `for_each_versioned_chunked(chunk_size, f)` that releases the read lock between chunks of `chunk_size` keys. Use it in save_snapshot with chunk_size = 16,384, which keeps each continuous lock-hold under ~2 ms at the per-entry Arc::clone + HashMap::insert cost (~110 ns). Wall-clock for the iteration is roughly unchanged. The win is for the apply path — instead of waiting up to 2 s for a queued write lock, it now waits at most one chunk (~2 ms) before the snapshot iterator yields. Snapshot semantics drift slightly: a value mutated between chunks may be observed mid-iteration with its updated state, while the same value processed in an earlier chunk shows its older state. The save path already produces an "approximate" snapshot under interior mutability (diff layers can change while iterating), so this matches existing behavior at the field level. Per-key brief-lock variant remains avoided (per the for_each_versioned doc-comment about writer starvation) — chunked is a middle ground: N/chunk_size lock acquisitions instead of N or 1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
…lock-hold executor::range_scan iterated FilterField via for_each_versioned which holds the bitmaps read lock for the entire scan. Same surface as the save_snapshot path fixed in the prior commit on this branch. This matters most for the merge thread's prefilter-refresh loop, which runs compute_filters on the safety prefilter (8 clauses, including IsNotNull(postId)) every cycle. IsNotNull doesn't go through range_scan directly — but Gt/Gte/Lt/Lte do, and any range scan on a high-cardinality field (postId 22.5 M, userId 769 K) holds the read lock for ~80-180 ms. After PR #224 unlocked the previously-dead prefilter-refresh path, this manifested as flush apply mean regressing 5.3 ms → 146 ms with all four prior PRs stacked: every cycle's prefilter refresh held the FilterField read lock long enough to block the apply path's queued write lock. Chunk size 16 K matches save_snapshot's. Same lesson, same fix shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
JustMaier
added a commit
that referenced
this pull request
Apr 25, 2026
) `FilterField::bitmap_bytes()` is called by the bitmap memory cache scanner thread (`bitmap_memory_cache::scan_tick`) every time a field is flagged stale. Previously held the bitmaps read lock for the entire iteration — at userId's 769K entries that's ~50 ms continuous; at postId's 22.5M it's ~1-2 s. Pre-PR-#224 the merge thread's `mark_stale` calls were unreachable (stray `}` closed the while loop early), so the scanner had nothing to re-scan and bitmap_bytes was effectively dormant. Post-#224 the merge thread runs the full cycle including `mark_stale` after each idle compaction, so the scanner re-reads at full cadence. Symptom: `[remove_bulk_slow] field=userId ... lock=80-235ms mut=4-15μs` in the apply path under sustained PG-sync ops. The mutation work itself is microseconds; apply was waiting on bitmap_bytes' read lock. Confirmed via instrumented bisect (5-ms threshold logs on every read- lock acquire + hold across `apply_diff_eq`, `union_with_diff`, `for_each_versioned`, `iter_versioned`, `merge_dirty`, and a `remove_bulk_slow` log on the apply-side write-lock acquire). Only bitmap_bytes' iteration showed the long hold; everything else under 5 ms. Fix: walk in 16K-key chunks, releasing the read lock between chunks. Same chunked-iter pattern that PR #225 uses for save_snapshot and range_scan. Wall-clock for the iteration is unchanged (same total per-VB work), but the maximum continuous lock-hold drops from O(N entries) to ~10 ms per chunk. Local measurement, sustained PG-sync stream, 100s window: Metric Pre-fix Post-fix ----------------------------------------------------- [remove_bulk_slow] lock_us 80-235 ms 5-11 ms (10-20x) apply mean (small batches) 146 ms <10 ms (15x) apply mean (catch-up batches) 146 ms 110-160 ms (no change — that's real bitmap insert work, not contention) This unblocks PR #224 — the merge-thread brace fix. The two together give us PR #222's apply-mean win back without the read-lock-blocker that #224's reanimated `mark_stale` introduced. Future work (deferred): cache the byte count with a TTL or update incrementally on mutation, eliminating the periodic re-scan entirely. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
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.
Summary
save_snapshotiterates each filter field viaFilterField::for_each_versioned, which holds the read lock for the duration. For postId at 22.5 M values, that's a single ~2 s read-lock-held window during which the apply path (a writer) waits.Add
for_each_versioned_chunked(chunk_size, f)that releases the lock between chunks. Use it insave_snapshotwithchunk_size = 16,384, dropping max continuous lock-hold from ~2 s to ~1–2 ms per chunk.Trade-off
Snapshot semantics drift slightly — a value mutated between chunks may be observed mid-iteration with its updated state, while the same value processed in an earlier chunk shows its older state. Save path already produces an "approximate" snapshot under interior mutability, so this matches existing behavior at the field level.
Status
Gated on measurement. Stacked alongside #221 / #222 / #223 / #224 locally — if measurement shows the apply path doesn't visibly stall during save_snapshot under prod-shape load, may close as unneeded since save_snapshot fires only on set-cursor / admin endpoint / shutdown.
Test plan
🤖 Generated with Claude Code