perf(shard): re-pin auxiliary threads off pinned shard cores (O5)#388
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds Linux auxiliary-core affinity management, initializes it during startup, and applies it to AOF, listener, autosave, manifest-sync, WAL-sync, spill, and embedded threads. Vector reload workers remain scheduler-placed, with tests and changelog documentation added. ChangesAuxiliary thread CPU pinning
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Startup
participant numa
participant AuxiliaryThreads
participant ShardThreads
Startup->>numa: init_aux_pinning(num_shards)
Startup->>ShardThreads: start pinned shard event loops
ShardThreads->>AuxiliaryThreads: spawn manifest, WAL, spill, and AOF workers
AuxiliaryThreads->>numa: pin_current_aux_thread(label)
numa-->>AuxiliaryThreads: apply non-shard core affinity
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shard/numa.rs`:
- Around line 144-158: Update aux_core_set_for and aux_core_set to derive
candidates from the process’s actual allowed CPU ID set rather than the
contiguous system_parallelism() count. Preserve the allowed affinity mask,
subtract CPUs assigned to shards, and return the remaining IDs; if affinity is
unavailable, retain the inherited shard mask instead of attempting to construct
or apply invalid CPU IDs.
- Around line 169-198: Update init_aux_pinning and the AUX_CORES lifecycle so a
completed run_embedded invocation cannot leave its shard-core configuration
permanently reused by later invocations with different shard counts. Prefer
resetting the state at shutdown or making it instance-scoped; at minimum, detect
and reject conflicting initialization rather than silently accepting the stale
core set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db5aeaad-34f1-42d8-b218-97cdb0a57c10
📒 Files selected for processing (8)
CHANGELOG.mdsrc/main.rssrc/persistence/manifest_sync.rssrc/persistence/wal_v3/sync_agent.rssrc/server/embedded.rssrc/shard/numa.rssrc/storage/tiered/spill_thread.rssrc/vector/reload_pool.rs
| /// Non-shard core IDs for `num_shards` shards (pinned to cores | ||
| /// `0..num_shards`) out of `total_cores` machine cores. `None` when there is | ||
| /// no remainder (`num_shards >= total_cores`) — pure function, unit-testable | ||
| /// without touching real system topology. | ||
| fn aux_core_set_for(num_shards: usize, total_cores: usize) -> Option<Vec<usize>> { | ||
| if num_shards >= total_cores { | ||
| return None; | ||
| } | ||
| Some((num_shards..total_cores).collect()) | ||
| } | ||
|
|
||
| /// Non-shard core IDs for `num_shards` shards on this machine | ||
| /// (`system_parallelism()` cores). See [`aux_core_set_for`]. | ||
| pub fn aux_core_set(num_shards: usize) -> Option<Vec<usize>> { | ||
| aux_core_set_for(num_shards, system_parallelism()) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Derive auxiliary cores from actual allowed CPU IDs, not a count.
system_parallelism() can count CPUs such as 0-3,8-11, but this constructs IDs num_shards..8, including offline 4-7. A cpuset-constrained container has the same problem: affinity calls fail and workers retain the inherited shard mask. Preserve/intersect the actual affinity IDs and subtract the CPUs assigned to shards.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/shard/numa.rs` around lines 144 - 158, Update aux_core_set_for and
aux_core_set to derive candidates from the process’s actual allowed CPU ID set
rather than the contiguous system_parallelism() count. Preserve the allowed
affinity mask, subtract CPUs assigned to shards, and return the remaining IDs;
if affinity is unavailable, retain the inherited shard mask instead of
attempting to construct or apply invalid CPU IDs.
| /// Process-wide non-shard core set, computed once. `None` means "don't pin" | ||
| /// (shards >= cores, `MOON_NO_AUX_PIN=1`, or [`init_aux_pinning`] was never | ||
| /// called — e.g. library-embedded tests that skip startup wiring). | ||
| static AUX_CORES: OnceLock<Option<Vec<usize>>> = OnceLock::new(); | ||
|
|
||
| /// Round-robin cursor spreading auxiliary threads evenly across the | ||
| /// non-shard core set instead of piling every one of them onto its first | ||
| /// entry. | ||
| static AUX_RR: AtomicUsize = AtomicUsize::new(0); | ||
|
|
||
| /// Record the non-shard core set for this process. Call once at startup, | ||
| /// after `num_shards` is resolved and BEFORE spawning any auxiliary thread | ||
| /// (the AOF pool, the vector reload/search pools, and the shard threads | ||
| /// themselves — the last of which spawn manifest-sync/spill/wal-sync | ||
| /// threads internally). First call wins (idempotent), matching the | ||
| /// `OnceLock` init pattern used by `reload_pool::init_global` / | ||
| /// `search_pool::init_global` elsewhere in this module's callers. | ||
| /// | ||
| /// Safe to call before any thread is spawned even though shard threads | ||
| /// inherit whatever affinity the calling thread has at spawn time: every | ||
| /// shard thread calls `pin_to_core` as the first act of its own closure, | ||
| /// unconditionally overriding whatever it inherited. | ||
| pub fn init_aux_pinning(num_shards: usize) { | ||
| let _ = AUX_CORES.get_or_init(|| { | ||
| if aux_pinning_disabled() { | ||
| None | ||
| } else { | ||
| aux_core_set(num_shards) | ||
| } | ||
| }); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Do not make embedded affinity configuration permanently first-call-wins.
After one run_embedded invocation exits, another invocation with a different shard count reuses this stale core set. On eight cores, starting with two shards and later four leaves auxiliary threads eligible for shard cores 2–3. Make this state instance-scoped or lifecycle-reconfigurable; at minimum reject conflicting initialization.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/shard/numa.rs` around lines 169 - 198, Update init_aux_pinning and the
AUX_CORES lifecycle so a completed run_embedded invocation cannot leave its
shard-core configuration permanently reused by later invocations with different
shard counts. Prefer resetting the state at shutdown or making it
instance-scoped; at minimum, detect and reject conflicting initialization rather
than silently accepting the stale core set.
5bfe216 to
17867ff
Compare
Mechanism (confirmed on real HW via ps -eLo psr): manifest-sync-N, spill-N, and moon-wal-sync-N are spawned FROM the shard's own pinned event-loop thread, and Linux threads inherit the parent's affinity mask at pthread_create — these aux threads weren't merely sharing a shard core, they were BORN wearing its exact single-core mask and could never leave (digest finding 6: ~12% CPU steal on shard cores under load; tmp/CPU-CACHE-DIGEST.md). Fix: when shards < cores, every auxiliary thread re-pins itself to the non-shard remainder (round-robin over cores shards..cores) as the first act of its closure — the same "escape the inherited mask" idiom the parallel-HNSW build workers already use. Covered: main accept/dispatch listener, manifest-sync-N, spill-N, moon-wal-sync-N, aof-writer[-N], auto-save, embedded AOF writer. No-op when shards >= cores, on non-Linux, or with MOON_NO_AUX_PIN=1 (bench A/B knob + operator escape hatch). Two deliberate exclusions: - The vector segment-reload pool stays scheduler-placed: reloads are latency-critical (queries await them) and the cold-start reload storm wants ALL cores while shards are idle — confining it to the small remainder risks the time-to-green regression PR #237 fixed. - The MAIN thread re-pins at the LAST moment (right before entering the accept loop, per-runtime), not at init: children spawned from main inherit its mask, so an early self-pin would have confined the reload/search pools and every other main-spawned thread to ONE aux core with no escape. Ordering reviewed and documented at both call sites. Adversarial review round widened coverage to every other pinned-parent spawn site in the tree: moon-heap-orphan-sweep-N (~40s crash-recovery I/O burst previously confined to its shard's core), the lazy moon-cold-read-N pool (previously born on whichever ONE shard core served the first cold read, stuck there for the process lifetime while serving ALL shards), moon-vec-snapshot-N, moon-vec-idx-gc, and moon-vec-compact-N (its last-core-downward heuristic landed workers on shard cores whenever shards ~= cores; now prefers the non-shard set via new numa::aux_core_at, heuristic kept as fallback). cpuset caveat documented: machine-wide core IDs can fail to pin under a restrictive cgroup cpuset - the thread keeps its inherited mask (debug-logged), never worse than before. jemalloc background-reclaim threads are a documented gap (spawned internally by jemalloc, no Rust-side affinity hook). Validation (GCE t2a-standard-8, 8 dedicated ARM vCPUs, shards=4, appendonly yes, 4 interleaved rounds, benchmark client tasksetted to cores 6-7 — conservative: pinned aux threads share those cores): - SET median 730.3K (pin) vs 719.6K (no-pin): +1.5% - GET median 918.5K vs 936.8K: -1.9%, inside the pre-registered -2% gate; GET spread tightened 23% -> 7% under pinning - Placement proof: pinned legs put all 12 aux threads on cores 4-7; unpinned legs show manifest-sync-N/spill-N exactly on shard cores 0-3 - A/B harness traps burned and fixed along the way: SO_REUSEPORT lets an orphan server silently split benchmark traffic (added pgrep clean-state aborts); redis-benchmark -q emits NO final summary once a run exceeds ~1s (switched to --csv); appendfsync everysec at 700K+ pipelined writes/s trips AOF backpressure and aborts the SET bench (A/B runs appendfsync no; the aof-writer threads still carry the full append stream). Refs tmp/CPU-CACHE-DIGEST.md (O5), task #26 author: Tin Dang <tindang.ht97@gmail.com>
|
Adversarial review round complete. The review REFUTED the original coverage claim — five pinned-parent spawn sites were missed ( |
17867ff to
f66e254
Compare
2 participants
Summary
manifest-sync-N/spill-N/moon-wal-sync-Nare spawned FROM the shard's pinned event-loop thread and inherit its single-core affinity mask atpthread_create— they were born locked to shard cores (digest finding 6: ~12% CPU steal;tmp/CPU-CACHE-DIGEST.md). Whenshards < cores, every auxiliary thread now re-pins itself round-robin onto the non-shard remainder as the first act of its closure. No-op whenshards >= cores, on non-Linux, or withMOON_NO_AUX_PIN=1.Deliberate exclusions
Validation (GCE t2a-standard-8, 8 dedicated ARM vCPUs, shards=4, AOF on, 4 interleaved rounds vs
MOON_NO_AUX_PIN=1)ps -eLo psrplacement proof: pinned legs put all 12 aux threads on cores 4-7; unpinned legs showmanifest-sync-N/spill-Nsitting exactly on shard cores 0-3Refs
tmp/CPU-CACHE-DIGEST.md(O5)Summary by CodeRabbit
Performance
MOON_NO_AUX_PIN=1to disable auxiliary-thread CPU pinning.Documentation