Skip to content

perf(filter): chunk bitmap_bytes iteration to bound read-lock-hold - #227

Merged
JustMaier merged 1 commit into
mainfrom
perf/bitmap-bytes-chunked-clean
Apr 25, 2026
Merged

perf(filter): chunk bitmap_bytes iteration to bound read-lock-hold#227
JustMaier merged 1 commit into
mainfrom
perf/bitmap-bytes-chunked-clean

Conversation

@JustMaier

Copy link
Copy Markdown
Contributor

Summary

FilterField::bitmap_bytes() is called by the bitmap memory scanner thread every time a field is flagged stale. The previous single-lock impl held the bitmaps read lock for the entire iteration: ~50 ms on userId (769K entries), ~1–2 s on postId (22.5M).

Pre-#224 the merge thread's mark_stale calls were unreachable (the stray } bug), so the scanner had nothing to re-scan and bitmap_bytes was effectively dormant. Post-#224 the merge thread runs the full cycle and the scanner re-reads at full cadence — turning bitmap_bytes into the dominant write-lock blocker for the apply path.

Root cause (instrumented bisect)

Apply path's remove_bulk was logging lock=80-235ms mut=4-15μs. The work was microseconds; apply was waiting on a reader.

Added 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 remove_bulk itself. Only bitmap_bytes's iteration showed the long hold; everything else was under 5 ms.

Fix

Walk the field in 16K-key chunks, releasing the read lock between chunks. Same chunked-iter pattern that PR #225 uses for save_snapshot and range_scan. Single-line semantic change to the iteration shape.

Wall-clock is unchanged (same per-VB work in aggregate). The win is bounded max-continuous-lock-hold for queued writers.

Measurements (local, sustained PG-sync stream, 100 s)

Metric Pre-fix Post-fix
[remove_bulk_slow] lock_us distribution 8–235 ms (mean 76 ms) 5–11 ms (mean 8.6 ms)
Apply mean (small batches) 146 ms < 10 ms
Apply mean (WAL catch-up batches: 30K–150K ops) 146 ms 110–160 ms (legitimate insert work, not contention)
[read_slow] for_each_versioned_chunked.chunk hold_us (new visible) n/a ~10 ms per 16K-entry chunk

Stacking

Test plan

  • Build clean (cargo build --profile fast --features server,pg-sync)
  • Live measurement with PG-sync streaming + bitmap memory scanner active — apply lock_us dropped 10-20x
  • Stacked re-measurement after merge (pending)
  • Time-to-first-warm-restore (pending — separate task)

Future work (deferred)

Cache the byte count with a TTL or update incrementally on mutation, eliminating the periodic re-scan entirely. Filed as a follow-up.

🤖 Generated with Claude Code

`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>
@JustMaier

Copy link
Copy Markdown
Contributor Author

Reviewed. Approved.

Diff matches diagnosis: snapshot keys upfront under brief read-lock, then chunked iteration with per-16K-chunk read-lock acquire/release. Apply's writers get to interleave between chunks instead of blocking the full scan window.

Concerns / things to watch (none blocking):

  1. Key-snapshot allocation costr.keys().copied().collect() allocates O(N × 8) bytes upfront. For postId at 22.5M entries that's ~180 MiB per scan call. Bounded (memory scanner runs every few seconds, not hot path), and trade is memory spike for lock-hold reduction. Acceptable. Your TODO to cache bitmap_bytes with TTL or update incrementally is the right longer-term fix.

  2. Approximate vs exact — between key-snapshot and chunk iteration, keys can be added (not counted) or removed (r.get returns None — properly handled). bitmap_bytes is now approximate under concurrent mutation, but this is a metrics scrape, exact isn't required.

  3. Why this differs from the abandoned per-key-lock variant — old comment captured that pattern. Donovan's chunked impl correctly differentiates (16K chunks vs per-key) so it doesn't recreate the writer-starvation issue. Comment update is clear about that distinction.

  4. Per-chunk lock-hold ~10 ms × ~1400 chunks (postId 22.5M / 16K) = ~14 s wall-clock per scan. Long total, but acceptable since each chunk releases. Apply writers no longer queue behind a single 1-2 s reader.

Approved. Same self-approve caveat — treat this comment as approval. Merging next + tagging v1.0.169-jemalloc, then merging #224 paired.

@JustMaier
JustMaier merged commit 4b4028d into main Apr 25, 2026
4 of 5 checks passed
@JustMaier
JustMaier deleted the perf/bitmap-bytes-chunked-clean branch April 25, 2026 09:15
JustMaier added a commit that referenced this pull request Apr 25, 2026
Captures the full causal chain + measurement data for the post-#224
apply-mean regression and its resolution via PRs #226/#227/#228:

- Bisect (mut_us 4-15μs vs lock_us 8-235ms in [remove_bulk_slow])
  showed apply was waiting for a long-held FilterField read.
- bitmap_memory_cache::scan_tick → FilterField::bitmap_bytes() iterating
  769K userId entries under one read lock was the holder.
- Pre-#224 the merge thread's mark_stale calls were unreachable, so
  the scanner had nothing to re-read and bitmap_bytes was effectively
  dormant. Post-#224 it ran at full cadence.
- PR #227 chunked bitmap_bytes to bound the read-lock-hold; lock_us
  dropped 80-235ms → 5-11ms.
- PR #228 fixed a separate latent bug in the warm-persist path where
  query.filters had been overwritten with the post-prefilter-substitution
  form (containing FilterClause::BucketBitmap, #[serde(skip)]) and
  serialization was silently failing every batch.
- Final time-to-first-warm-restore: 163s total (159s dump restore
  + 4.6s auto-warm of 187 shapes), down from the handoff's reported
  30-60min — a 100-300x reduction.

Full session ship list captured at the bottom (v1.0.165 through
v1.0.171 — seven perf PRs all stacked cleanly).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JustMaier added a commit that referenced this pull request Apr 25, 2026
…eared

11-min sustained mixed-load window on full perf stack v1.0.165-175 + relay V1
(post #232). Setup: local BitDex on 110.5M dump + replay-prod-via-relay
consumer subscribed to prod /events/ops + replay-captured at 3K QPS local
query load.

Headline:
- 1,724,131 queries served
- P50 < 50μs / P95 < 25ms / P99 ~25ms / max < 500ms
- Mission gate (P99 < 1s) cleared by ~40x margin
- 0 flush-slow / 0 ops-trace / 0 insert_bulk_slow events
- 8 sort-top_n SLOW events (max 65ms — separate surface, non-blocking)

Hypothesis verification:
- #1 userId-Eq apply contention: confirmed mitigated by #222/#227/#224 stack
- #2 tagIds-In multi-value contention: no contention observed
- #3 sortAtUnix-Gte tolerance miss: debunked for corpus (all snap to canonical buckets)

Caveats:
- Cache-saturated due to 232-shape captured corpus; absolute hit rate not
  representative of prod's diverse load. Real diverse shadow query traffic
  requires V2 model-share tee_mode skip PR.
- Sort-layer fusion under concurrent sort-field writes is a separate surface
  surfaced during measurement. Filed as task #17 follow-up.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant