Skip to content

perf: restore #173 ArcSwap time_buckets — fix .177 cold P99 regression - #248

Merged
JustMaier merged 1 commit into
mainfrom
fix/time-buckets-arcswap
Apr 30, 2026
Merged

perf: restore #173 ArcSwap time_buckets — fix .177 cold P99 regression#248
JustMaier merged 1 commit into
mainfrom
fix/time-buckets-arcswap

Conversation

@JustMaier

Copy link
Copy Markdown
Contributor

Summary

Cherry-picks 25a91c8 (PR #173, original Justin authorship preserved) onto current main. PR #173 was developed on a parallel branch line that never merged into main; the .177-jemalloc release line forked off v1.0.150 before #173 landed and built parallel work (#220, #224, #226, #233, #234, #237, #238, #244, #246) without ever picking it up.

Result: the cold-path Mutex serialization fix the 2026-04-10 PR #173 commit message describes is missing from the deployed code path.

Why this matters (validated 2026-04-29 cold-burst rig)

Local v1.0.177-jemalloc reproduces the exact bug PR #173 originally fixed:

"The time_buckets field was Arc<Mutex<TimeBucketManager>>. Every query acquired this Mutex at the start of execute_query_with_collector and held it for the ENTIRE function execution. This serialized all queries through a single lock."
— PR #173 commit message

Diagnostic instrumentation on a v1.0.177 build confirmed:

DIAG-setup total=290352us | lazy=1us shard=0us snap=1us tb_lock=290341us exec_build=0us snap_range=6us
DIAG-setup total=251083us | tb_lock=251065us
DIAG-setup total=204513us | tb_lock=204501us

The Mutex wait IS the entire cold P99 budget on this branch line. At conc=32 cold burst, all 32 queries serialize behind one lock — each waits behind ~10-25ms of prior queries' real engine work, summing to 200-300ms tb_lock_us.

Validation curve (400 prod-shape cold queries, conc=32, prod MALLOC_CONF)

metric pre-cherry-pick post-cherry-pick delta
wall QPS 103 189 1.8×
wall P50 295 ms 8 ms 37×
engine P50 (server-side) ~290 ms ~250 µs ~1100×
le=50us bucket count 6 / 409 54 / 405
le=100us bucket count 10 105 10×
le=1ms bucket count 11 225 20×

filter_us and sort_us totals roughly tripled in absolute time post-fix — that's because they're now actually parallel (CPU contention, not Mutex contention), so the engine is doing more work per second.

Tail residue (~1% in le=5s)

4 of 405 queries land in le=5s. These are first-touch cold lazy-loads (e.g. modelVersionIds shards on never-queried values). Real engine work, not serialization. Acceptable under shadow flip-on burst with #61 (jemalloc decay tuning) handling RSS retention.

Ship sequence

This unblocks v1.0.178 cold-path safety:

Combined: prod shadow flip-on no longer causes Mutex queueing × cap=32 saturation × jemalloc retention → no RSS ratchet to 110 GB.

Test plan

  • Cherry-pick clean (no conflicts on concurrent_engine.rs)
  • cargo check --features server,pg-sync --bin bitdex-server passes
  • Full release build passes (~9 min cold, target/release/bitdex-server.exe written)
  • Cold-burst rig validation: P50 295→8ms, throughput 1.8×, le=50us bucket 9× more queries
  • Tom builds v1.0.178 image
  • Adam stages canary (talos-wjh-tgy server-mode flip-back)

Refs

🤖 Generated with Claude Code

ROOT CAUSE FIX for the P50 regression Justin identified.

The time_buckets field was `Arc<Mutex<TimeBucketManager>>`. Every
query acquired this Mutex at the start of execute_query_with_collector
and held it for the ENTIRE function execution. This serialized all
queries through a single lock. When one query was slow (100ms+ from
lazy load or cache miss), all concurrent queries queued behind it.

v1.0.85 didn't have this issue because time buckets were added later.
The feature introduced a global serialization point that regressed
query P50 from sub-2ms to 60-85ms on ~30% of queries (confirmed
by pre_cache_us trace data from PR #172/v1.0.168).

Fix: Replace `Mutex<TimeBucketManager>` with `ArcSwap<TimeBucketManager>`.

- Query path (3 sites): `tb.load_full()` — lock-free Arc snapshot,
  no contention, queries run fully concurrent.
- Flush/refresh path (~8 sites): load → clone → mutate → store.
  Clone is cheap (4 buckets, Arc'd bitmaps = refcount bumps only).
  Store is atomic, readers never see partial state.
- TimeBucket + TimeBucketManager now derive Clone.

Local validation: 32-worker 30s load test, 2286 queries, no crashes,
query_duration avg = 31µs (same as pre-fix baseline).

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@JustMaier

Copy link
Copy Markdown
Contributor Author

LGTM — Code review (Scarlet via team channel)

Diff is textbook ArcSwap conversion pattern:

  • Struct field: Mutex<TimeBucketManager>ArcSwap<TimeBucketManager>
  • Read path: .lock().load() (zero-cost guard)
  • Write path: .lock()(*tb_arc.load_full()).clone() + mutate + tb_arc.store(Arc::new(tb)) (clone-on-write, single writer = no contention)

Net +40 / -31, single file (src/concurrent_engine.rs).

Validation (Donovan local rig, 400 prod-shape cold queries, conc=32, prod MALLOC_CONF, fresh boot)

metric pre-fix (.177) post-fix (#173 cherry) delta
Wall P50 295 ms 8 ms 37× faster
Engine P50 (server) ~290 ms ~250 μs ~1160×
Throughput 103 qps 189 qps 1.8×
Filter mean (real work) hidden 90 ms surfaced
Sort mean (real work) hidden 46 ms surfaced

80% of cold queries finish in <1ms engine time. Real cold work (~136ms) was hidden behind 290ms Mutex serialization.

Tail residue ~1% in le=5s bucket — first-touch cold lazy-loads, not a serialization bug, expected.

Context

PR #173 (commit 25a91c8) authored by @JustMaier was on an orphan branch that never merged. Both main and v1.0.177-jemalloc tag descend from v1.0.150 along a parallel line that never picked it up. Cherry-pick is clean (40 ins / 31 del, no conflicts) onto current main.

Original commit message says it directly:

The time_buckets field was Arc<Mutex<TimeBucketManager>>. Every query acquired this Mutex at the start of execute_query_with_collector and held it for the ENTIRE function execution. This serialized all queries through a single lock... regressed query P50 from sub-2ms to 60-85ms on ~30% of queries.

Investigation today: shadow flip on v1.0.177-jemalloc OOM-killed pod in 30s (RSS 11.7→110 GB). Root cause = exactly this serialization: cold queries pile up holding working set × jemalloc dirty_decay_ms:0 retention = ratchet to OOM. Fix at the source.

Ready to merge. Tom can build v1.0.178-jemalloc from main HEAD post-merge.

@JustMaier

Copy link
Copy Markdown
Contributor Author

CI status: pre-existing rot on main, not introduced by this PR

PR #244 — test fail (merged anyway, on main)
PR #245 — test fail (merged anyway, on main)
PR #246 — test fail (merged anyway, on main)
PR #247 — test fail (open, fan-out cap)
PR #248 — test fail (this PR, time_buckets ArcSwap)

All five hit the same error: function <X> is never used (src/shard_store_doc.rs) + mismatched-lifetime-syntaxes warnings. CI uses -D warnings. The 88 errors are all -D warnings rejections, none of which my cherry-pick touches (single file diff: src/concurrent_engine.rs).

Filed as follow-up: warning hygiene PR to clean the 88 stale-warning items + restore green CI. Tracked.

Local cargo build (release, full features) passes cleanly. Validation curve in PR body holds.

@JustMaier
JustMaier merged commit 586ae5c into main Apr 30, 2026
4 of 5 checks passed
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