From 8d402964a4e0bbffeed8f45c364b7c04e253ffb9 Mon Sep 17 00:00:00 2001 From: Tin Dang Date: Sat, 18 Jul 2026 09:35:09 +0700 Subject: [PATCH] =?UTF-8?q?perf(shard):=20precompute=20per-shard=20offload?= =?UTF-8?q?=20paths=20=E2=80=94=20no=20per-tick=20format!/join=20(#45)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 100ms eviction tick (run_eviction_tick), the memory-pressure cascade (handle_memory_pressure, two sites), the 10s warm-transition check, and the cold-orphan sweep each rebuilt "/shard-{id}" via format! + PathBuf::join on every firing, per shard — and effective_disk_offload_dir() itself clones a PathBuf per call. That is three allocations per recurring tick per shard on paths CLAUDE.md declares allocation-free, cumulative across N shards forever. Fix, per the issue's suggested shape: the event loop already builds a per-shard `disk_offload_dir: Option` once at init (Some iff disk-offload is enabled — the same gate that nulls spill_thread and shard_manifest). Thread it into run_eviction_tick and handle_memory_pressure as Option<&Path> and use it directly in the warm-check and orphan-sweep tick arms of both loop flavors (tokio select! and monoio tick counter). Every consumer pairs the dir's Some-guard with its existing disk-offload-implied guard, so behavior is unchanged in every reachable state; the one impossible state (spill machinery live without an offload dir) fails safe to the pre-existing plain-drop branch. Out of scope (event-gated, not recurring): snapshot-trigger path builders (fire only when a save epoch advances), reclamation-schedule persist (is_dirty-gated), and all startup/recovery one-time joins. Verified: full lib suite, both clippy matrices (--all-targets), and the VM offload battery (kill-9 cold-multidb / no-AOF / spill-batch, orphan sweep, collection visibility, DBSIZE, SCAN visibility, shadow resurrection) green with the refactored binary — the battery round-trips spill -> cold read-through -> recovery through the exact paths whose construction moved. Fixes #45 author: Tin Dang --- CHANGELOG.md | 12 +++++++++ src/shard/event_loop.rs | 49 ++++++++++++++++++----------------- src/shard/persistence_tick.rs | 44 ++++++++++++++++++------------- 3 files changed, 63 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fb7f56b..b1808215 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Performance +- **Shard offload paths are precomputed at shard init — the recurring + tick paths no longer allocate (#45).** The 100ms eviction tick, the + memory-pressure cascade, the 10s warm-transition check, and the + cold-orphan sweep each rebuilt `/shard-{id}` via + `format!` + `PathBuf::join` (plus a `PathBuf` clone inside + `effective_disk_offload_dir()`) on every firing, per shard — a + CLAUDE.md hot-path-allocation violation. The event loop's existing + per-shard `disk_offload_dir` (built once at init, `Some` iff + disk-offload is enabled) is now threaded into `run_eviction_tick` + and `handle_memory_pressure` as `Option<&Path>` and used directly by + the warm/orphan tick arms. Cold event-gated builders (save trigger, + reclamation-schedule persist, startup/recovery) are unchanged. - **SCAN cold-plane pages now range-resume from the cursor — no full cold-index filter per page (#368).** The in-RAM cold index (spilled keys under disk-offload) is now ordered by the same `(hash48, key)` diff --git a/src/shard/event_loop.rs b/src/shard/event_loop.rs index 1b3217d1..ae0682e8 100644 --- a/src/shard/event_loop.rs +++ b/src/shard/event_loop.rs @@ -1719,14 +1719,15 @@ impl super::Shard { } // Warm tier transition check (10s interval, disk-offload only) _ = warm_check_interval.0.tick() => { - if server_config.disk_offload_enabled() { + // task/issue #45: `disk_offload_dir` (Some iff offload + // enabled) is precomputed at shard init — no per-tick + // format!/join. + if let Some(shard_dir) = disk_offload_dir.as_deref() { if let Some(ref mut manifest) = shard_manifest { - let shard_dir = server_config.effective_disk_offload_dir() - .join(format!("shard-{}", shard_id)); crate::shard::slice::with_shard(|s| { persistence_tick::check_warm_transitions( &s.vector_store, - &shard_dir, + shard_dir, manifest, server_config.segment_warm_after, server_config.engine_offload_idle_secs, @@ -1758,16 +1759,17 @@ impl super::Shard { std::future::pending::<()>().await; } }, if orphan_sweep_interval.is_some() && server_config.disk_offload_enabled() => { - let shard_dir = server_config - .effective_disk_offload_dir() - .join(format!("shard-{}", shard_id)); - timers::run_cold_orphan_sweep( - &shard_databases, - shard_id, - &shard_dir, - shard_manifest.as_mut(), - cached_clock.ms(), - ); + // task/issue #45: precomputed shard dir; the select guard + // already requires disk-offload, so this is always Some. + if let Some(shard_dir) = disk_offload_dir.as_deref() { + timers::run_cold_orphan_sweep( + &shard_databases, + shard_id, + shard_dir, + shard_manifest.as_mut(), + cached_clock.ms(), + ); + } } // Expire timed-out blocked clients every 10ms _ = block_timeout_interval.0.tick() => { @@ -1812,6 +1814,7 @@ impl super::Shard { &mut wal_writer, &script_cache_rc, &spill_file_id, + disk_offload_dir.as_deref(), &repl_backlog, &mut replica_txs, &repl_offsets, aof_pool.as_ref(), match wal_kv_log_mode { @@ -2392,6 +2395,7 @@ impl super::Shard { &mut wal_writer, &script_cache_rc, &spill_file_id, + disk_offload_dir.as_deref(), &repl_backlog, &mut replica_txs, &repl_offsets, @@ -2466,15 +2470,15 @@ impl super::Shard { } // Warm tier check: every warm_poll_ms ticks if monoio_tick_counter % (warm_poll_ms as u64) == 0 { - if server_config.disk_offload_enabled() { + // task/issue #45: `disk_offload_dir` (Some iff offload + // enabled) is precomputed at shard init — no per-tick + // format!/join. + if let Some(shard_dir) = disk_offload_dir.as_deref() { if let Some(ref mut manifest) = shard_manifest { - let shard_dir = server_config - .effective_disk_offload_dir() - .join(format!("shard-{}", shard_id)); crate::shard::slice::with_shard(|s| { persistence_tick::check_warm_transitions( &s.vector_store, - &shard_dir, + shard_dir, manifest, server_config.segment_warm_after, server_config.engine_offload_idle_secs, @@ -2527,15 +2531,12 @@ impl super::Shard { // Matches the tokio select! branch above. Disabled when interval is 0. if orphan_sweep_interval_secs > 0 && monoio_tick_counter % (orphan_sweep_interval_secs * 1000) == 0 - && server_config.disk_offload_enabled() + && let Some(shard_dir) = disk_offload_dir.as_deref() { - let shard_dir = server_config - .effective_disk_offload_dir() - .join(format!("shard-{}", shard_id)); timers::run_cold_orphan_sweep( &shard_databases, shard_id, - &shard_dir, + shard_dir, shard_manifest.as_mut(), cached_clock.ms(), ); diff --git a/src/shard/persistence_tick.rs b/src/shard/persistence_tick.rs index 92f4161b..d878f68c 100644 --- a/src/shard/persistence_tick.rs +++ b/src/shard/persistence_tick.rs @@ -357,6 +357,13 @@ pub(crate) fn run_eviction_tick( wal_v3_writer: &mut Option, script_cache: &std::rc::Rc>, spill_file_id: &std::rc::Rc>, + // task/issue #45: `/shard-{id}`, precomputed ONCE at shard init + // (event_loop's `disk_offload_dir`) instead of a per-tick + // `format!` + `join` allocation pair on this 100ms path. `None` iff + // disk-offload is disabled — the same gate that nulls `spill_thread` + // and `shard_manifest`, so every consumer below is already + // Some-guarded. + offload_shard_dir: Option<&std::path::Path>, // task #34 (Wave A): `record_reason_del` handles for plain-dropped // eviction victims — threaded straight through to `timers::run_eviction` // and `handle_memory_pressure`'s sync-spill-fallback branch. @@ -514,11 +521,11 @@ pub(crate) fn run_eviction_tick( shard_databases, shard_id, runtime_config, - server_config, shard_manifest, next_file_id, wal_v3_writer, spill_thread, + offload_shard_dir, repl_backlog, replica_txs, repl_state, @@ -537,15 +544,13 @@ pub(crate) fn run_eviction_tick( // its pre-existing fail-close plain-drop (policy-aware: `noeviction` // still OOMs, an evicting policy still frees RAM, just with no cold // copy -- matches PR #273's fail-close discipline). - let eviction_shard_dir = server_config - .effective_disk_offload_dir() - .join(format!("shard-{}", shard_id)); let mut spill_ctx: Option> = None; if server_config.disk_offload_enabled() + && let Some(shard_dir) = offload_shard_dir && let Some(ref mut manifest) = *shard_manifest { spill_ctx = Some(crate::storage::eviction::SpillContext { - shard_dir: &eviction_shard_dir, + shard_dir, manifest, next_file_id, // Placeholder — `run_eviction` restamps per database (#139). @@ -750,11 +755,12 @@ pub(crate) fn handle_memory_pressure( shard_databases: &std::sync::Arc, shard_id: usize, runtime_config: &std::sync::Arc>, - server_config: &std::sync::Arc, shard_manifest: &mut Option, next_file_id: &mut u64, wal_v3: &mut Option, spill_thread: Option<&crate::storage::tiered::spill_thread::SpillThread>, + // task/issue #45: see `run_eviction_tick`. + offload_shard_dir: Option<&std::path::Path>, // task #34 (Wave A): see `run_eviction_tick`. repl_backlog: &crate::replication::backlog::SharedBacklog, replica_txs: &mut Vec, @@ -786,13 +792,12 @@ pub(crate) fn handle_memory_pressure( // stub), which actually returns RAM — and stays reloadable-on-touch. We // pass `warm_after = u64::MAX` so the age-based HOT->WARM path stays // disabled here; only the idle->COLD path fires. - if let Some(ref mut manifest) = *shard_manifest { - let shard_dir = server_config - .effective_disk_offload_dir() - .join(format!("shard-{}", shard_id)); + if let Some(ref mut manifest) = *shard_manifest + && let Some(shard_dir) = offload_shard_dir + { let count = crate::shard::slice::with_shard(|s| { s.vector_store.try_warm_transitions_all_idle( - &shard_dir, + shard_dir, manifest, u64::MAX, PRESSURE_OFFLOAD_IDLE_SECS, @@ -843,11 +848,12 @@ pub(crate) fn handle_memory_pressure( }; if total_mem > budget { let db_count = shard_databases.db_count(); - let shard_dir = server_config - .effective_disk_offload_dir() - .join(format!("shard-{}", shard_id)); + // task/issue #45: `offload_shard_dir` is precomputed at shard + // init. `spill_thread` and `shard_manifest` share its + // disk-offload gate, so both branches below pair their + // existing Some-guard with the dir's. - if let Some(spill_t) = spill_thread { + if let (Some(spill_t), Some(shard_dir)) = (spill_thread, offload_shard_dir) { // Async spill path: background thread does pwrite under // `--appendonly yes` (AOF-backstopped fast path). Under // `--appendonly no` there is no AOF backstop, so @@ -865,7 +871,7 @@ pub(crate) fn handle_memory_pressure( db, &rt, &sender, - &shard_dir, + shard_dir, next_file_id, total_mem, i, @@ -899,9 +905,11 @@ pub(crate) fn handle_memory_pressure( // Sync spill fallback for i in 0..db_count { crate::shard::slice::with_shard_db(i, |db| { - if let Some(ref mut manifest) = *shard_manifest { + if let Some(ref mut manifest) = *shard_manifest + && let Some(shard_dir) = offload_shard_dir + { let mut ctx = crate::storage::eviction::SpillContext { - shard_dir: &shard_dir, + shard_dir, manifest, next_file_id, // #139: this fallback runs inside the