perf(shard): precompute per-shard offload paths — no per-tick format!/join (#45) by pilotspacex-byte · Pull Request #384 · pilotspace/moon

pilotspacex-byte · 2026-07-18T02:35:47Z

Summary

Closes #45 (P1-2). 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 <offload>/shard-{id} via format! + PathBuf::join on every firing, per shard — and effective_disk_offload_dir() itself clones a PathBuf per call. Three allocations per recurring tick per shard on paths CLAUDE.md declares allocation-free.

Fix

The event loop already builds a per-shard disk_offload_dir: Option<PathBuf> once at init (Some iff disk-offload is enabled — the same gate that nulls spill_thread and shard_manifest). This PR threads it into run_eviction_tick and handle_memory_pressure as Option<&Path> and uses 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. handle_memory_pressure's now-unused server_config parameter is removed.

Out of scope (event-gated, not recurring): snapshot-trigger builders (fire only on save-epoch advance), reclamation-schedule persist (is_dirty-gated), startup/recovery one-time joins.

Verification

Full lib suite (4392), clippy --all-targets on both feature matrices, fmt --check.
VM offload battery on the refactored Linux binary: kill-9 cold-multidb / no-AOF / spill-batch, orphan sweep 5/5, collection visibility 4/4, DBSIZE 2/2, SCAN visibility, shadow resurrection — all green. The battery round-trips spill → cold read-through → recovery through the exact paths whose construction moved.

Summary by CodeRabbit

Performance
- Improved disk-offload eviction and memory-pressure handling by reducing repeated path calculations and temporary allocations.
- Optimized maintenance checks for warm transitions and cold orphan cleanup.
- These changes reduce overhead during periodic eviction, memory-pressure cascades, and disk-offload maintenance.

…/join (#45) 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 "<offload>/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<PathBuf>` 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 <tindang.ht97@gmail.com>

qodo-code-review · 2026-07-18T02:35:50Z

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

coderabbitai · 2026-07-18T02:36:06Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d79278b1-cac7-427f-8cd0-f945086fb7fe

📥 Commits

Reviewing files that changed from the base of the PR and between 55a189a and 8d40296.

📒 Files selected for processing (3)

CHANGELOG.md
src/shard/event_loop.rs
src/shard/persistence_tick.rs

📝 Walkthrough

Walkthrough

Disk-offload shard directories are precomputed and reused across Tokio and monoio maintenance ticks, eviction, memory-pressure handling, warm transitions, orphan sweeps, and spill context creation.

Changes

Disk-offload path reuse

Layer / File(s)	Summary
Eviction and pressure-path directory threading `src/shard/persistence_tick.rs`, `CHANGELOG.md`	`run_eviction_tick` and `handle_memory_pressure` receive the optional precomputed shard directory and use it for warm transitions and async or synchronous spill handling.
Maintenance tick wiring `src/shard/event_loop.rs`	Tokio and monoio warm-transition, orphan-sweep, and eviction timers reuse `disk_offload_dir.as_deref()` instead of rebuilding shard paths per tick.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

pilotspace/moon#232: Changes the same eviction tick signature and event-loop wiring.
pilotspace/moon#312: Overlaps with disk-offload and SpillContext wiring in the eviction path.
pilotspace/moon#270: Modifies the async KV eviction flow in handle_memory_pressure.

Suggested reviewers: tindang97

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR addresses the recurring event-loop and memory-pressure paths, but it leaves some linked cold-tier and recovery construction sites unmodified.	Also update the remaining linked sites in cold_tier, warm_segment, and recovery, or narrow the issue scope to match this PR.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately captures the main perf change to precompute offload paths.
Description check	✅ Passed	The description is detailed and on-topic, with the main summary, fix, and verification covered despite some template sections being omitted.
Out of Scope Changes check	✅ Passed	The changes stay focused on the offload-path optimization and changelog update, with no unrelated code changes evident.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch perf/45-precomputed-offload-paths

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

pilotspacex-byte · 2026-07-18T02:53:11Z

Adversarial review outcome: CONFIRMED-EQUIVALENT. The reviewer proved the load-bearing invariant: disk_offload_dir, shard_manifest, and spill_thread are all init-time products of the same immutable server_config.disk_offload_enabled() gate (CONFIG SET only touches RuntimeConfig), so for the shard's lifetime disk_offload_dir.is_some() ≡ offload-enabled, and when Some it is byte-identical to what the removed per-tick format!+join would have built. All four replaced tick arms, both run_eviction_tick call sites, and the pressure-cascade branches verified argument-by-argument; enforce_warm_mmap_budget still runs unconditionally in both loop flavors; Option<&Path> → SpillContext<'a> borrow sound. No reachable behavioral change found.

Post-review gates: VM crash trio re-run on the final binary (cold-multidb, no-AOF, spill-batch kill-9) all green.

pilotspacex-byte merged commit 3281654 into main Jul 18, 2026
8 checks passed

TinDang97 deleted the perf/45-precomputed-offload-paths branch July 18, 2026 02:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

perf(shard): precompute per-shard offload paths — no per-tick format!/join (#45)#384

perf(shard): precompute per-shard offload paths — no per-tick format!/join (#45)#384
pilotspacex-byte merged 1 commit into
mainfrom
perf/45-precomputed-offload-paths

pilotspacex-byte commented Jul 18, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

qodo-code-review Bot commented Jul 18, 2026

Uh oh!

coderabbitai Bot commented Jul 18, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

pilotspacex-byte commented Jul 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

pilotspacex-byte commented Jul 18, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fix

Verification

Summary by CodeRabbit

Uh oh!

qodo-code-review Bot commented Jul 18, 2026

Qodo reviews are paused for this user.

Uh oh!

coderabbitai Bot commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

pilotspacex-byte commented Jul 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pilotspacex-byte commented Jul 18, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 18, 2026 •

edited

Loading