feat: improve mooncake_bench sweep logic and throughput accounting#6631
Conversation
- Reduce default num_gpu_blocks from 1M to 16K to stress cache eviction - Add count_events flag so indexers that don't support remove aren't credited for silently dropped events in throughput numbers - Sweep high-to-low for multi-threaded indexers with early stop at 95% sustained throughput; low-to-high for single-threaded with early stop at 5x saturation - Export sweep results to JSON alongside the plot - Simplify dynamo_mocker import paths Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: PeaBrane <[email protected]>
Bench improvements extracted to rupei/mooncake-bench-improvements (#6631). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: PeaBrane <[email protected]>
WalkthroughBenchmark configuration enhancements that add conditional multi-threaded vs. single-threaded sweep behavior with custom early stopping criteria, event counting controls, new JSON serialization output, and an adjusted default GPU block count parameter. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/kv-router/benches/mooncake_bench.rs`:
- Around line 1168-1171: Replace fragile substring replacements on
args.sweep_output with path-based extension replacement: use
std::path::Path::with_extension (e.g.
Path::new(&args.sweep_output).with_extension("json").to_string_lossy().into_owned())
to build json_path from args.sweep_output instead of .replace(".png", ...) /
.replace(".svg", ...); apply the same change at the other occurrence mentioned
(around line with the second replacement).
- Around line 1079-1084: The sweep generation code uses args.sweep_steps as n
and divides by (n - 1), which is invalid when sweep_steps <= 1; update the logic
around n and the durations_low_to_high computation to guard against degenerate
inputs by checking args.sweep_steps (or n) first: if n <= 1 produce a sensible
single-entry durations vector (e.g., one value derived from log_min/log_max or
return an error/early exit), otherwise perform the existing interpolation using
(n - 1); ensure this change touches the block that computes
durations_low_to_high and any callers that expect multiple steps so downstream
throughput math/plotting remains stable.
…i-dynamo#6631) Signed-off-by: PeaBrane <[email protected]> Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
Summary
num_gpu_blocksfrom 1M to 16K to stress cache evictioncount_eventsflag so indexers that don't support remove aren't credited for silently dropped events in throughput numbersdynamo_mockerimport pathsTest plan
cargo bench --bench mooncake_benchsingle-indexer mode and verify throughput numbers exclude events for non-remove indexers🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores