Repository navigation
feat(bench): add trace-driven throughput benchmark for PositionalIndexer - #636
Conversation
Add a Dynamo-compatible throughput benchmark that measures block ops/sec using the same methodology as Dynamo's mooncake_bench: - Synthetic trace generation with shared prefixes and multi-turn sessions - Concurrent tokio task replay (256 workers) with sleep_until pacing - Block throughput metric: total_blocks = request_blocks + event_blocks - Sweep mode: logarithmic duration compression to find peak throughput - Early stop when system keeps up for 5 consecutive steps New file: kv_index/benches/throughput_bench.rs - Standalone binary with clap CLI (not criterion) - Configurable: num_workers, jump_size, blocks_per_request, event_blocks, num_sessions, requests_per_session, duplication_factor, seed - Default params match Dynamo: 256 workers, jump_size=8, 128 blocks/req Also: - Add [profile.bench] with opt-level=3 to workspace Cargo.toml (benchmarks were inheriting opt-level="z" from release profile) - Add clap, tokio dev-deps to kv_index/Cargo.toml - Add throughput benchmark step to CI workflow Local results (macOS laptop, 200K sessions, 256 workers): Peak: 33.1M blocks/sec at 616ms sweep duration Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
github-actions
Bot
added
dependencies
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new trace-driven throughput benchmark for the Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a trace-driven block throughput benchmark for kv-index (new bench binary), bench-specific Cargo profile and dev-deps, and CI workflow steps to run, persist, and summarize throughput results alongside existing benchmarks. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Benchmark CLI
participant Gen as Trace Generator
participant Sweep as Sweep Controller
participant Runner as Benchmark Runner
participant Indexer as PositionalIndexer
participant Agg as Metrics Aggregator
CLI->>Gen: generate_traces(workers, seed)
Gen-->>CLI: per-worker traces
alt Sweep Mode
CLI->>Sweep: start_sweep(params)
loop each duration
Sweep->>Gen: rescale_traces(target_duration)
Gen-->>Sweep: rescaled traces
Sweep->>Runner: run_benchmark(rescaled_traces)
Runner->>Indexer: replay trace entries concurrently
Indexer-->>Runner: per-op latencies/results
Runner->>Agg: aggregate metrics
Agg-->>Runner: run results
end
else Single Run
CLI->>Gen: rescale_traces(target_duration)
Gen-->>CLI: rescaled traces
CLI->>Runner: run_benchmark(rescaled_traces)
Runner->>Indexer: replay trace entries concurrently
Indexer-->>Runner: per-op latencies/results
Runner->>Agg: aggregate metrics
Agg-->>Runner: run results
end
Runner-->>CLI: print results and write `throughput_output.txt`
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d26762171
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| args.benchmark_duration_ms, | ||
| args.sweep_steps, | ||
| ); | ||
| let durations_high_to_low: Vec<u64> = durations.into_iter().rev().collect(); |
There was a problem hiding this comment.
The sweep currently reverses durations (durations.into_iter().rev()), so it benchmarks longest durations first (lowest offered load) and then applies an early-stop after 5 consecutive keeping_up results. Because low-load points are most likely to satisfy keeping_up, the loop can terminate before it ever reaches the short-duration, high-load steps where peak throughput/saturation should be observed. This can systematically under-report throughput in the new CI benchmark path that runs sweep mode.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive throughput benchmark for the PositionalIndexer, which is a valuable addition for performance analysis. The benchmark is well-structured, featuring synthetic trace generation, concurrent replay, and a sweep mode to identify peak throughput. I've identified a potential panic in the trace generation logic that should be fixed to ensure benchmark stability, and I have a suggestion to improve memory efficiency during the benchmark setup by avoiding a deep clone of trace data.
![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 the current code and only fix it if needed.
Inline comments:
In `@kv_index/benches/throughput_bench.rs`:
- Around line 575-588: The function compute_sweep_durations can call ln(0) when
min_ms == 0; change it to guard or normalize min_ms before taking ln by clamping
it to at least 1 (e.g., let safe_min = if min_ms == 0 { 1 } else { min_ms }) and
use safe_min for computing log_min, keeping the existing steps<=1 early return
and the rest of the computation in compute_sweep_durations unchanged so no
negative infinity is produced.
- Around line 161-166: The call to indexer.apply_stored currently discards its
Result (let _ = ...); change this to check the Result and record failures
instead of ignoring them: capture the Result from apply_stored in the processing
loop (where process/throughput bench invokes apply_stored), increment a local or
shared error counter (e.g., a mutable usize in the benchmark struct or an
AtomicUsize like self.error_count) on Err(_) and optionally log the error via
the existing logger; ensure apply_stored call site (the block in
throughput_bench.rs around the worker loop) updates the counter so the benchmark
can assert or report error counts after running.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 16245ef6-11c3-464c-80d3-ecb49a1ec069
📒 Files selected for processing (4)
.github/workflows/benchmark-radix-tree.ymlCargo.tomlkv_index/Cargo.tomlkv_index/benches/throughput_bench.rs
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kv_index/benches/throughput_bench.rs`:
- Around line 79-85: Remove the redundant `sweep: bool` clap argument and use
only the opt-out `no_sweep: bool` field; update all code that currently reads
`args.sweep` to instead use `!args.no_sweep` (or equivalent) so sweep mode is
true by default when `no_sweep` is false. Specifically, delete the `sweep` field
declaration and replace references to the `sweep` symbol (e.g., in benchmark
selection or control flow) with the negation of the `no_sweep` symbol; ensure
the clap documentation/comments are updated to reflect that `--no-sweep`
disables sweep mode.
- Around line 216-219: The current computation of prefix_start can panic when
args.shared_prefix_blocks == 0 because you end up doing a modulo by zero; change
the divisor to never be zero by computing a safe_divisor =
prefix_pool.len().saturating_sub(args.shared_prefix_blocks).max(1) and use that
for the modulo (i.e., (session_id * 7) % safe_divisor) so prefix_start is always
valid; keep the remaining logic that computes prefix_end and session_prefix the
same so slicing (session_prefix = &prefix_pool[prefix_start..prefix_end])
remains safe.
- Around line 556-560: The p99 computation using lats[lats.len() * 99 / 100] is
biased toward the max for many sizes; change the index calculation for
latency_p99_us to use the last-valid-index adjusted formula such as
(lats.len().saturating_sub(1)) * 99 / 100 so you index lats safely and get the
proper 99th percentile; keep the same empty check for lats, compute the index as
usize from that expression, and divide the selected latency by 1000.0 as before.
- Line 428: Remove the redundant sorting call: the vector results is populated
by iterating durations_high_to_low (which already yields descending durations),
so the call to results.sort_by(|a, b| b.0.cmp(&a.0)) is a no-op; delete that
sort_by line (referencing the results variable and the durations_high_to_low
iteration) to avoid unnecessary work and keep the code simpler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee019451-8b8a-4f39-8532-334a412b94df
📒 Files selected for processing (1)
kv_index/benches/throughput_bench.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1626c016c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| StoredBlock { | ||
| seq_hash: SequenceHash(global_seq_counter), | ||
| content_hash: compute_content_hash(&[rng.random::<u32>()]), | ||
| } |
There was a problem hiding this comment.
The trace generator says shared prefixes should simulate multi-turn cache hits, but event blocks are populated from fresh random tokens instead of the request/session prefix hashes, so apply_stored rarely inserts the same content queried by later find_matches calls. In practice this turns the benchmark into a mostly cache-miss workload, which can materially skew throughput and invalidate the intended Dynamo-style comparison whenever shared_prefix_blocks > 0.
Useful? React with 👍 / 👎.
…ration With only 1000 sessions (960K total blocks), the sweep early-stops because the system trivially keeps up — the workload is too small to find the throughput ceiling. Bump default to 200K sessions (192M blocks) and lower sweep_min_ms to 10ms / 20 steps so the default invocation produces meaningful peak throughput numbers without extra flags. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
f1626c0 to
53d5897
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
kv_index/benches/throughput_bench.rs (5)
414-414: 🧹 Nitpick | 🔵 TrivialRemove redundant sort in sweep summary.
resultsis already appended in descending duration order (fromdurations_high_to_low), so this sort is unnecessary work.🧹 Proposed fix
- results.sort_by(|a, b| b.0.cmp(&a.0)); - for (dur_ms, result) in &results {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kv_index/benches/throughput_bench.rs` at line 414, The sweep summary performs an unnecessary sort: remove the redundant results.sort_by(|a, b| b.0.cmp(&a.0)); call since results are already appended in descending order by durations_high_to_low; locate the usage in the throughput_bench sweep summary code (look for the results variable and durations_high_to_low) and delete that sort to avoid wasted work while preserving existing ordering logic.
163-171:⚠️ Potential issue | 🟠 MajorDo not discard
apply_storederrors; they invalidate throughput accounting.Line 163 currently ignores failures, and Line 169 still counts event blocks, which can over-report throughput when events fail to apply.
🔧 Proposed fix
struct TaskState { worker_blocks: WorkerBlockMap, req_blocks: u64, evt_blocks: u64, + apply_errors: u64, latencies: Vec<u64>, count_events: bool, } @@ Self { worker_blocks: WorkerBlockMap::default(), req_blocks: 0, evt_blocks: 0, + apply_errors: 0, latencies: Vec::new(), count_events, } @@ - let _ = indexer.apply_stored( + if indexer + .apply_stored( worker_id, blocks, *parent_seq_hash, &mut self.worker_blocks, - ); - if self.count_events { + ) + .is_ok() + { + if self.count_events { + self.evt_blocks += blocks.len() as u64; + } + } else { + self.apply_errors += 1; + } - self.evt_blocks += blocks.len() as u64; - } } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kv_index/benches/throughput_bench.rs` around lines 163 - 171, The call to indexer.apply_stored is silently ignoring errors, which can make throughput accounting (evt_blocks) incorrect when application fails; change the invocation of apply_stored (indexer.apply_stored(..., &mut self.worker_blocks)) to handle its Result instead of discarding it — if it returns Err, do not increment self.evt_blocks and propagate or log/return the error as appropriate for the benchmark harness; ensure worker_id, blocks, and *parent_seq_hash handling remains the same but only count blocks into evt_blocks when apply_stored succeeds.
542-546:⚠️ Potential issue | 🟡 Minorp99 index is off by one for common lengths.
Line 545 uses
len * 99 / 100, which frequently selects the max element instead of the 99th percentile index.📏 Proposed fix
let latency_p99_us = if lats.is_empty() { 0.0 } else { - lats[lats.len() * 99 / 100] as f64 / 1000.0 + let p99_idx = (lats.len().saturating_sub(1)) * 99 / 100; + lats[p99_idx] as f64 / 1000.0 };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kv_index/benches/throughput_bench.rs` around lines 542 - 546, The p99 calculation uses lats[lats.len() * 99 / 100] which for many lengths picks the max rather than the 99th percentile; change the index to use the zero-based upper-bound formula e.g. lats[(lats.len() - 1) * 99 / 100] when computing latency_p99_us so the percentile maps correctly (keep the existing empty check for lats before subtracting 1).
213-219:⚠️ Potential issue | 🔴 CriticalGuard zero divisors in trace generation to prevent panics.
Line 213 can panic with
--num-workers 0(session_id % args.num_workers), and Line 217 can panic with--shared-prefix-blocks 0(% 0aftersaturating_sub).🛡️ Proposed fix
fn generate_traces(args: &Args) -> Vec<Vec<TimedEntry>> { + if args.num_workers == 0 { + return Vec::new(); + } + let mut rng = StdRng::seed_from_u64(args.seed); @@ - let prefix_start = - (session_id * 7) % prefix_pool.len().saturating_sub(args.shared_prefix_blocks); + let prefix_span = prefix_pool + .len() + .saturating_sub(args.shared_prefix_blocks) + .max(1); + let prefix_start = (session_id * 7) % prefix_span;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kv_index/benches/throughput_bench.rs` around lines 213 - 219, Guard against zero divisors before computing worker_id and prefix slice: check args.num_workers and args.shared_prefix_blocks are non-zero (or coerce to 1) prior to using session_id % args.num_workers and prior to the saturating_sub(...) % operation. Specifically, add a precheck before the worker_id calculation (referencing worker_id, session_id, args.num_workers) and before computing prefix_start (referencing prefix_pool, args.shared_prefix_blocks) to avoid dividing/modulo by zero — either return an error, use a safe default like 1, or skip generation when those args are zero.
567-568:⚠️ Potential issue | 🟡 MinorClamp sweep bounds before logarithms to avoid invalid values.
ln(0)at Line 567/568 yields-inf, which can produce invalid/degenerate sweep durations.🛡️ Proposed fix
fn compute_sweep_durations(min_ms: u64, max_ms: u64, steps: usize) -> Vec<u64> { if steps <= 1 { return vec![max_ms]; } - let log_min = (min_ms as f64).ln(); - let log_max = (max_ms as f64).ln(); + let safe_min = min_ms.max(1); + let safe_max = max_ms.max(safe_min); + let log_min = (safe_min as f64).ln(); + let log_max = (safe_max as f64).ln(); (0..steps)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kv_index/benches/throughput_bench.rs` around lines 567 - 568, Clamp min_ms and max_ms to a small positive value before taking their natural logarithms to avoid ln(0) producing -inf; update the computation that sets log_min and log_max in throughput_bench.rs (the variables named min_ms, max_ms, log_min, log_max) to use something like let safe_min = min_ms.max(EPS) and let safe_max = max_ms.max(EPS) (choose EPS = 1e-9 or similar) and then compute log_min = (safe_min as f64).ln() and log_max = (safe_max as f64).ln().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kv_index/benches/throughput_bench.rs`:
- Around line 393-400: The sweep loop currently iterates all durations; add
early-stop logic: introduce a counter (e.g., consecutive_keeps) before the loop,
and after each run_benchmark(...) check the benchmark result for the "keeps up"
condition (use the result's existing predicate or field, e.g., result.keeps_up()
or result.is_kept_up). If the predicate is true increment consecutive_keeps
otherwise reset it to zero; after incrementing, if consecutive_keeps >= 5 then
still call print_results(&result), push the (dur_ms, result) into results, and
break out of the loop. Update the loop surrounding durations_high_to_low to use
this counter and the predicate so the sweep stops after five consecutive
keeps-up results.
---
Duplicate comments:
In `@kv_index/benches/throughput_bench.rs`:
- Line 414: The sweep summary performs an unnecessary sort: remove the redundant
results.sort_by(|a, b| b.0.cmp(&a.0)); call since results are already appended
in descending order by durations_high_to_low; locate the usage in the
throughput_bench sweep summary code (look for the results variable and
durations_high_to_low) and delete that sort to avoid wasted work while
preserving existing ordering logic.
- Around line 163-171: The call to indexer.apply_stored is silently ignoring
errors, which can make throughput accounting (evt_blocks) incorrect when
application fails; change the invocation of apply_stored
(indexer.apply_stored(..., &mut self.worker_blocks)) to handle its Result
instead of discarding it — if it returns Err, do not increment self.evt_blocks
and propagate or log/return the error as appropriate for the benchmark harness;
ensure worker_id, blocks, and *parent_seq_hash handling remains the same but
only count blocks into evt_blocks when apply_stored succeeds.
- Around line 542-546: The p99 calculation uses lats[lats.len() * 99 / 100]
which for many lengths picks the max rather than the 99th percentile; change the
index to use the zero-based upper-bound formula e.g. lats[(lats.len() - 1) * 99
/ 100] when computing latency_p99_us so the percentile maps correctly (keep the
existing empty check for lats before subtracting 1).
- Around line 213-219: Guard against zero divisors before computing worker_id
and prefix slice: check args.num_workers and args.shared_prefix_blocks are
non-zero (or coerce to 1) prior to using session_id % args.num_workers and prior
to the saturating_sub(...) % operation. Specifically, add a precheck before the
worker_id calculation (referencing worker_id, session_id, args.num_workers) and
before computing prefix_start (referencing prefix_pool,
args.shared_prefix_blocks) to avoid dividing/modulo by zero — either return an
error, use a safe default like 1, or skip generation when those args are zero.
- Around line 567-568: Clamp min_ms and max_ms to a small positive value before
taking their natural logarithms to avoid ln(0) producing -inf; update the
computation that sets log_min and log_max in throughput_bench.rs (the variables
named min_ms, max_ms, log_min, log_max) to use something like let safe_min =
min_ms.max(EPS) and let safe_max = max_ms.max(EPS) (choose EPS = 1e-9 or
similar) and then compute log_min = (safe_min as f64).ln() and log_max =
(safe_max as f64).ln().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d438ff9-c9b4-4d10-9209-c74cfa9079a7
📒 Files selected for processing (1)
kv_index/benches/throughput_bench.rs
- Remove redundant `--sweep` flag; sweep is now on by default, use `--no-sweep` to disable - Guard zero divisor in generate_traces when shared_prefix_blocks=0 causes modulo-by-zero panic - Fix p99 index off-by-one: use (len-1)*99/100 instead of len*99/100 which was biased toward max - Remove redundant results.sort_by() in sweep summary (already in descending order from iteration) - Clamp min_ms/max_ms to >=1 in compute_sweep_durations to avoid ln(0) producing -inf - Track apply_stored errors instead of discarding Result; only count blocks on success; report errors in output - Take traces by value in run_benchmark to avoid unnecessary deep clone, use into_iter().map(Arc::new) instead - Remove now-invalid --sweep flag from CI workflow Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
kv_index/benches/throughput_bench.rs (1)
397-404:⚠️ Potential issue | 🟠 MajorSweep loop still lacks the 5-consecutive “keeps up” early stop.
The loop currently runs every duration unconditionally, which contradicts the stated sweep behavior and increases benchmark/CI time.
♻️ Proposed fix
async fn run_sweep(args: &Args, base_traces: &[Vec<TimedEntry>]) { @@ let mut results: Vec<(u64, BenchmarkResults)> = Vec::new(); + let mut keep_up_streak = 0usize; @@ let result = run_benchmark(args, traces).await; print_results(&result); + let keeps_up = result.block_throughput >= result.offered_block_throughput * 0.98; + if keeps_up { + keep_up_streak += 1; + } else { + keep_up_streak = 0; + } results.push((dur_ms, result)); + if keep_up_streak >= 5 { + break; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kv_index/benches/throughput_bench.rs` around lines 397 - 404, The sweep loop in throughput_bench.rs must stop early after 5 consecutive "keeps up" results; modify the loop that iterates over durations_high_to_low to track a consecutive_keeps counter, increment it when the benchmark result indicates a "keeps up" outcome (e.g., check result.keeps_up() or result.status == KeepsUp), reset it to 0 when the result is not a keep-up, and break the loop once consecutive_keeps >= 5; leave the calls to rescale_traces, run_benchmark, print_results and pushing into results unchanged except for adding this counter/check immediately after obtaining result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kv_index/benches/throughput_bench.rs`:
- Around line 410-412: The sweep summary printing uses result.block_throughput
for both the "Actual" and "Block Throughput" columns; update the print
statements that fill the "Actual" column (the formatted lines using the format
string and the later block at lines similar to 431-437) to use the actual
throughput field (e.g., result.actual_throughput or result.actual) instead of
result.block_throughput so the columns show Offered, Actual, and Block
Throughput correctly; search for occurrences of result.block_throughput in the
printing code and replace the ones that correspond to the "Actual" column with
the correct result.actual_* field.
- Around line 11-15: Add the clippy::disallowed_methods expectation at crate
level by adding #![expect(clippy::disallowed_methods)] alongside the existing
top-of-file attributes (the lines with #![expect(clippy::print_stdout)] and
#![expect(clippy::expect_used)]), and remove any redundant per-function or local
#![expect(clippy::disallowed_methods)] occurrences elsewhere in the file (the
instances flagged in the review). This keeps the benchmark-wide lint expectation
consistent and avoids duplicated local attributes.
- Around line 46-47: The num_workers field can be zero causing a modulo-by-zero
at the site that computes session_id % args.num_workers; validate and refuse
zero. After parsing args (where the struct with num_workers is constructed,
e.g., in main() or the benchmark entry), check that args.num_workers > 0 and
return an error/exit if not (or use clap validation on the num_workers arg to
require a minimum of 1). Update the declaration/validation for num_workers (the
num_workers arg) and ensure the runtime check prevents proceeding when
num_workers == 0 before the code that does session_id % args.num_workers.
- Around line 91-93: The count_events boolean field is currently a switch
(ArgAction::SetTrue) with default_value_t = true, so users cannot pass false;
update the field declaration for count_events to accept an explicit value by
adding action = clap::ArgAction::Set (e.g. #[arg(long, default_value_t = true,
action = clap::ArgAction::Set)]) so the CLI will parse true/false values,
allowing users to pass --count-events=false; locate the count_events field in
the bench args struct and apply this attribute change.
---
Duplicate comments:
In `@kv_index/benches/throughput_bench.rs`:
- Around line 397-404: The sweep loop in throughput_bench.rs must stop early
after 5 consecutive "keeps up" results; modify the loop that iterates over
durations_high_to_low to track a consecutive_keeps counter, increment it when
the benchmark result indicates a "keeps up" outcome (e.g., check
result.keeps_up() or result.status == KeepsUp), reset it to 0 when the result is
not a keep-up, and break the loop once consecutive_keeps >= 5; leave the calls
to rescale_traces, run_benchmark, print_results and pushing into results
unchanged except for adding this counter/check immediately after obtaining
result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d733a7c6-2841-4982-858a-a4e0cff18f31
📒 Files selected for processing (2)
.github/workflows/benchmark-radix-tree.ymlkv_index/benches/throughput_bench.rs
Remove all mentions of Dynamo from code comments across kv_index crate. The designs stand on their own merit without external attribution. - kv_index/src/event_tree.rs: 6 comment references removed - kv_index/benches/throughput_bench.rs: 1 comment reference removed Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (5)
kv_index/benches/throughput_bench.rs (5)
46-47:⚠️ Potential issue | 🟠 MajorValidate
num_workersto prevent modulo-by-zero panic.Line 212 computes
session_id % args.num_workers. If--num-workers 0is passed, this will panic at runtime.🛡️ Proposed fix
/// Number of workers (concurrent replay tasks). - #[arg(long, default_value_t = 256)] + #[arg(long, default_value_t = 256, value_parser = clap::value_parser!(usize).range(1..))] num_workers: usize,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kv_index/benches/throughput_bench.rs` around lines 46 - 47, The code allows args.num_workers to be zero which causes a panic at the modulo operation (session_id % args.num_workers); add validation right after parsing the args to ensure args.num_workers > 0 and fail fast with a clear error message (or adjust the clap parser for the num_workers field to only accept values >=1). Locate the struct field num_workers and the code performing session_id % args.num_workers and implement the check (or change the clap validator) so the program exits or returns an error when num_workers == 0 instead of performing the modulo.
11-15: 🧹 Nitpick | 🔵 TrivialPrefer crate-level clippy expect for benchmark-wide disallowed methods.
The
clippy::disallowed_methodsexpectation at line 452 should be moved to crate level for consistency with the existing bench lint policy at lines 11-15.♻️ Proposed refactor
// Benchmark binary — println is the intended output mechanism. #![expect(clippy::print_stdout)] // Benchmark binary — panicking on task join failure is acceptable. #![expect(clippy::expect_used)] +// Benchmark binary — tokio::spawn is required for concurrent benchmark replay. +#![expect(clippy::disallowed_methods)] @@ -#[expect(clippy::disallowed_methods)] // tokio::spawn is required for concurrent benchmark replay async fn run_benchmark(args: &Args, traces: Vec<Vec<TimedEntry>>) -> BenchmarkResults {Based on learnings: In Rust benchmark files, prefer crate-level Clippy attributes over per-function scoping for consistent lint behavior.
Also applies to: 452-453
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kv_index/benches/throughput_bench.rs` around lines 11 - 15, Add a crate-level Clippy expect for clippy::disallowed_methods (similar to the existing crate attributes for clippy::print_stdout and clippy::expect_used) and remove the current per-function/per-block expect for clippy::disallowed_methods; specifically, add a new top-of-file attribute like the existing #![expect(...)] attributes and delete the localized #[expect(clippy::disallowed_methods)] instance so the lint applies consistently across the benchmark file.
409-412:⚠️ Potential issue | 🟡 MinorSweep summary prints duplicate throughput columns.
"Actual" and "Block Throughput" columns (lines 411, 435-436) both print
result.block_throughput, making the table redundant.🧹 Proposed fix — remove duplicate column
println!( - "{:<12} | {:<18} | {:<18} | {:<18}", - "Duration", "Offered", "Actual", "Block Throughput" + "{:<12} | {:<18} | {:<18}", + "Duration", "Offered", "Actual" ); @@ println!( - "{:<12} | {:<18} | {:<18} | {:<18}{}", + "{:<12} | {:<18} | {:<18}{}", dur_label, format_throughput(result.offered_block_throughput), format_throughput(result.block_throughput), - format_throughput(result.block_throughput), if is_peak && results.len() > 1 {Also applies to: 431-437
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kv_index/benches/throughput_bench.rs` around lines 409 - 412, The table header and row printing duplicate the same value: remove the redundant "Actual" / duplicate column by updating the header printf and the corresponding row printf(s) so result.block_throughput is only printed once; locate the println that defines the header (currently printing "Duration", "Offered", "Actual", "Block Throughput") and any subsequent printlns that print result.block_throughput twice, then delete the extra column label and the extra argument/format specifier that prints result.block_throughput so the table shows only the intended columns (e.g., "Duration", "Offered", "Block Throughput").
397-404:⚠️ Potential issue | 🟡 MinorSweep loop is missing the advertised early-stop behavior.
The PR objectives state "early stop after 5 consecutive steps where the system keeps up," but the loop runs all durations unconditionally.
♻️ Proposed fix
async fn run_sweep(args: &Args, base_traces: &[Vec<TimedEntry>]) { @@ let mut results: Vec<(u64, BenchmarkResults)> = Vec::new(); + let mut keep_up_streak = 0usize; for &dur_ms in &durations_high_to_low { @@ let result = run_benchmark(args, traces).await; print_results(&result); + + // Early stop: if system keeps up (actual >= 98% of offered) for 5 consecutive steps + let keeps_up = result.block_throughput >= result.offered_block_throughput * 0.98; + if keeps_up { + keep_up_streak += 1; + if keep_up_streak >= 5 { + results.push((dur_ms, result)); + println!("Early stop: system kept up for 5 consecutive steps"); + break; + } + } else { + keep_up_streak = 0; + } results.push((dur_ms, result)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kv_index/benches/throughput_bench.rs` around lines 397 - 404, The sweep loop over durations_high_to_low currently runs all durations unconditionally; add an early-stop mechanism that counts consecutive successful steps and breaks after 5 in a row. Inside the for &dur_ms in &durations_high_to_low loop, introduce a consecutive_successes counter; after computing let result = run_benchmark(args, traces).await, test the result for "keeps up" (e.g., a boolean like result.keeps_up or result.is_within_sla or equivalent metric on the returned struct), increment consecutive_successes when true and reset to 0 when false, and when consecutive_successes >= 5 call break (and optionally print a message) instead of continuing; update any surrounding state (results vector, print_results call) accordingly so successful runs are still recorded before the potential break.
91-93:⚠️ Potential issue | 🟠 Major
count_eventscannot be disabled from the command line due to clap's default bool behavior.With clap v4 derive, a
boolfield defaults toArgAction::SetTrue. Combined withdefault_value_t = true, both when the flag is absent and present, the value remainstrue—making it impossible for users to passfalse.🔧 Proposed fix
-use clap::Parser; +use clap::{ArgAction, Parser}; @@ - #[arg(long, default_value_t = true)] + #[arg(long, action = ArgAction::SetFalse, default_value_t = true)] count_events: bool,Alternatively, add a
--no-count-eventsflag if the intent is opt-out semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kv_index/benches/throughput_bench.rs` around lines 91 - 93, The bool field count_events currently uses default_value_t = true which with clap v4 makes the flag always true; remove the default and change the clap attributes to explicit flag actions and add an opt-out flag so users can disable counting: keep the existing count_events bool but replace #[arg(long, default_value_t = true)] with #[arg(long, action = ArgAction::SetTrue)] (so it only becomes true when passed) and add a new no_count_events: bool with #[arg(long = "no-count-events", action = ArgAction::SetTrue)]; then where the code uses count_events compute an effective flag (e.g., let effective_count_events = if no_count_events { false } else { count_events } ) so users can pass --no-count-events to disable counting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@kv_index/benches/throughput_bench.rs`:
- Around line 46-47: The code allows args.num_workers to be zero which causes a
panic at the modulo operation (session_id % args.num_workers); add validation
right after parsing the args to ensure args.num_workers > 0 and fail fast with a
clear error message (or adjust the clap parser for the num_workers field to only
accept values >=1). Locate the struct field num_workers and the code performing
session_id % args.num_workers and implement the check (or change the clap
validator) so the program exits or returns an error when num_workers == 0
instead of performing the modulo.
- Around line 11-15: Add a crate-level Clippy expect for
clippy::disallowed_methods (similar to the existing crate attributes for
clippy::print_stdout and clippy::expect_used) and remove the current
per-function/per-block expect for clippy::disallowed_methods; specifically, add
a new top-of-file attribute like the existing #![expect(...)] attributes and
delete the localized #[expect(clippy::disallowed_methods)] instance so the lint
applies consistently across the benchmark file.
- Around line 409-412: The table header and row printing duplicate the same
value: remove the redundant "Actual" / duplicate column by updating the header
printf and the corresponding row printf(s) so result.block_throughput is only
printed once; locate the println that defines the header (currently printing
"Duration", "Offered", "Actual", "Block Throughput") and any subsequent printlns
that print result.block_throughput twice, then delete the extra column label and
the extra argument/format specifier that prints result.block_throughput so the
table shows only the intended columns (e.g., "Duration", "Offered", "Block
Throughput").
- Around line 397-404: The sweep loop over durations_high_to_low currently runs
all durations unconditionally; add an early-stop mechanism that counts
consecutive successful steps and breaks after 5 in a row. Inside the for &dur_ms
in &durations_high_to_low loop, introduce a consecutive_successes counter; after
computing let result = run_benchmark(args, traces).await, test the result for
"keeps up" (e.g., a boolean like result.keeps_up or result.is_within_sla or
equivalent metric on the returned struct), increment consecutive_successes when
true and reset to 0 when false, and when consecutive_successes >= 5 call break
(and optionally print a message) instead of continuing; update any surrounding
state (results vector, print_results call) accordingly so successful runs are
still recorded before the potential break.
- Around line 91-93: The bool field count_events currently uses default_value_t
= true which with clap v4 makes the flag always true; remove the default and
change the clap attributes to explicit flag actions and add an opt-out flag so
users can disable counting: keep the existing count_events bool but replace
#[arg(long, default_value_t = true)] with #[arg(long, action =
ArgAction::SetTrue)] (so it only becomes true when passed) and add a new
no_count_events: bool with #[arg(long = "no-count-events", action =
ArgAction::SetTrue)]; then where the code uses count_events compute an effective
flag (e.g., let effective_count_events = if no_count_events { false } else {
count_events } ) so users can pass --no-count-events to disable counting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3a354837-4b04-4578-8c45-6c31b35db930
📒 Files selected for processing (2)
kv_index/benches/throughput_bench.rskv_index/src/event_tree.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e0a5bdb9f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let mut timestamp_us: u64 = 0; | ||
|
|
||
| for session_id in 0..args.num_sessions { | ||
| let worker_id = session_id % args.num_workers; |
There was a problem hiding this comment.
Trace generation derives worker_id with session_id % args.num_workers but the CLI accepts --num-workers 0, which triggers a divide-by-zero panic before the benchmark starts. This turns a bad user input into a crash instead of a clear validation error, which is brittle for scripted benchmark runs.
Useful? React with 👍 / 👎.
Change throughput output from "blocks/sec" to "block ops/sec" to match the standard terminology used in KV indexer benchmarks. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 130f72b7de
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let num_total_workers = args.num_workers * args.duplication_factor; | ||
| for w in 0..num_total_workers { | ||
| indexer.intern_worker(&format!("worker-{w}")); | ||
| } |
There was a problem hiding this comment.
num_total_workers is derived from user input and used directly to intern workers, but there is no guard for invalid values. When --duplication-factor is 0, the benchmark runs zero tasks and reports a misleading 0-throughput success; when num_workers * duplication_factor exceeds the indexer capacity (MAX_WORKERS = 2048 in PositionalIndexer::intern_worker), this loop panics at runtime instead of failing with a clear CLI error. This can break or silently invalidate scripted benchmark sweeps.
Useful? React with 👍 / 👎.
- Move clippy::disallowed_methods expect to crate level for consistency with other benchmark-wide lint suppression attributes - Validate num_workers >= 1 at startup to prevent modulo-by-zero panic - Replace count_events bool (unusable default_value_t=true) with --no-count-events opt-out flag matching the --no-sweep pattern - Remove duplicate "Actual" column from sweep summary table — it was printing the same value as "Block Throughput" - Preserve initial timestamp in replay: sleep to each entry's target time before processing, not after. This prevents workers with non-zero first timestamps from shifting their stream earlier and overstating throughput Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 170de05c83
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| tasks.push(tokio::spawn(async move { | ||
| let mut state = TaskState::new(count_events); | ||
|
|
||
| let base_time = tokio::time::Instant::now(); |
There was a problem hiding this comment.
The trace timestamps are generated on a single global clock (generate_traces fills timestamp_us monotonically), but each spawned worker task resets its own base_time with tokio::time::Instant::now(). Because tasks are spawned/scheduled at slightly different times, every worker trace gets a different offset, which changes cross-worker ordering and lowers/warps offered load—especially in short sweep points (e.g., 10ms) where a few milliseconds of skew is material. This makes throughput results non-comparable to the intended global replay schedule.
Useful? React with 👍 / 👎.
Add support for replaying real-world Mooncake production traces (from kvcache-ai/Mooncake) in addition to synthetic trace generation. New CLI flags: - --trace-path: path to a Mooncake JSONL trace file - --trace-length-factor: stretch each request's hash sequence (default: 1) - --trace-duplication-factor: duplicate traces with offset hash_ids (default: 1) Trace pipeline (matching Dynamo's mooncake_bench methodology): - load_mooncake_trace: parse JSONL with hash_ids, timestamp, output_length - expand_trace_lengths: stretch hash sequences by factor - duplicate_traces: create structurally identical copies with disjoint hashes - partition_trace: randomly partition across workers - convert_mooncake_traces: convert to TimedEntry format with paired Request (find_matches) + Event (apply_stored) entries per request Each request's hash_ids become content_hashes for find_matches, and the same hashes are stored via apply_stored to populate the cache, creating realistic prefix sharing patterns for future lookups. Files changed: - kv_index/Cargo.toml: add serde + serde_json dev-dependencies - kv_index/benches/throughput_bench.rs: add Mooncake trace loading section, update main() to branch on --trace-path, add content_hash_from_id helper Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8552b85bdb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| #[derive(serde::Deserialize, Clone)] | ||
| struct MooncakeRequest { | ||
| #[serde(default)] | ||
| timestamp: u64, |
There was a problem hiding this comment.
timestamp
The loader documents timestamp as optional, but MooncakeRequest.timestamp defaults to 0, so timestamp-less traces produce only two distinct times (0 for requests and 1µs for events). After rescale_traces, that collapses into "all requests at start, all events at end of the benchmark window," which destroys the intended request/event interleaving and can materially distort hit rate and throughput comparisons for valid JSONL inputs that omit timestamps.
Useful? React with 👍 / 👎.
Download all three public Mooncake production traces (conversation, synthetic, toolagent) from kvcache-ai/Mooncake and run the throughput benchmark against each with 20x duplication to generate sufficient load for saturation testing. CI now runs both synthetic and trace-driven benchmarks: - Synthetic: 200K sessions, 256 workers, sweep mode - Mooncake: 3 traces × 20x duplication, 256 workers, sweep mode Trace source: kvcache-ai/Mooncake FAST25-release/traces/ Files changed: - .github/workflows/benchmark-radix-tree.yml: add trace download step, add Mooncake benchmark step, update summary and artifact upload Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2987192251
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let parent = prev_last_seq; | ||
| prev_last_seq = blocks.last().map(|b| b.seq_hash); |
There was a problem hiding this comment.
Using prev_last_seq as the parent for every subsequent request chains all apply_stored events on a worker into one ever-growing sequence, so after the first request blocks are written at high positions that later find_matches calls (which start from position 0) typically never probe. On real Mooncake traces with many independent requests, this turns most inserted blocks into unreachable cache state and materially skews the benchmark toward misses/under-reported overlap instead of the intended replay behavior.
Useful? React with 👍 / 👎.
| run: | | ||
| mkdir -p traces | ||
| for trace in conversation_trace synthetic_trace toolagent_trace; do | ||
| curl -sL "https://raw.githubusercontent.com/kvcache-ai/Mooncake/main/FAST25-release/traces/${trace}.jsonl" \ |
There was a problem hiding this comment.
The workflow downloads traces from the upstream repository's main branch, so benchmark inputs can change without any change in this repo; this makes historical throughput comparisons non-reproducible and can create apparent regressions/improvements caused only by upstream trace edits. Pinning to a commit SHA (or vendored artifact) is needed to keep CI benchmark results stable across runs.
Useful? React with 👍 / 👎.
cargo bench runs the binary with a different working directory than the shell step, so relative paths like traces/foo.jsonl fail with "No such file or directory". Use $GITHUB_WORKSPACE for absolute paths. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d5f546563
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .map(|e| e.timestamp_us) | ||
| .max() | ||
| .unwrap_or(1); | ||
| let trace_duration_ms = max_ts_us / 1000; |
There was a problem hiding this comment.
trace_duration_ms is derived via integer division from max_ts_us, so any non-millisecond-aligned trace window is rounded down before computing offered_block_throughput. In short sweep points this can materially overstate offered load (e.g., ~10% error when ~10ms windows truncate to 9ms), and sub-millisecond traces are forced to 0 offered throughput, which skews the offered-vs-achieved interpretation in benchmark output.
Useful? React with 👍 / 👎.
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
1 participant
Summary
Add a Dynamo-compatible throughput benchmark that measures block ops/sec using the same methodology as Dynamo's
mooncake_bench, enabling direct performance comparison.What changed
kv_index/benches/throughput_bench.rs(new) — standalone benchmark binary with:sleep_untilpacingtotal_blocks = request_blocks + event_blocksCargo.toml— added[profile.bench]withopt-level = 3(benchmarks were inheritingopt-level = "z"from release profile, losing 10-30% perf)kv_index/Cargo.toml— addedclap,tokiodev-deps and[[bench]]entry.github/workflows/benchmark-radix-tree.yml— added throughput benchmark step to CI (200K sessions, 256 workers, sweep mode)Why
Dynamo claims 170M block ops/sec for their KV indexer. Our PositionalIndexer uses the same DashMap architecture but our benchmarks measured different things (criterion micro-benchmarks, ops/s instead of block throughput). To prove SMG matches or exceeds Dynamo's performance, we need a benchmark that measures the same metric with comparable inputs.
How
Ported Dynamo's
mooncake_benchmethodology:sleep_untilpacingfind_matches(D blocks)= D block-ops, eachapply_stored(N blocks)= N block-opsDefault parameters match Dynamo: 256 workers,
jump_size=8, 128 blocks/request, 64 blocks/event.Test plan
cargo test -p kv-index— all 158 tests passcargo clippy -p kv-index --bench throughput_bench -- -D warnings— cleancargo bench -p kv-index --bench throughput_bench -- --no-sweep --num-sessions 500 --num-workers 64— runs successfullySummary by CodeRabbit
New Features
Chores
Documentation