[tools] add bench_hit_rate: multi-turn cache hit rate benchmark for MetaService - #117
[tools] add bench_hit_rate: multi-turn cache hit rate benchmark for MetaService#117wangxiyu191 wants to merge 1 commit into
Conversation
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
👋 Review Summary
Nice addition — a multi-turn hit-rate benchmark is a useful tool for validating MetaService prefix matching behavior under realistic workloads. The code is well-structured, follows existing patterns in the repo (e.g. the success_blocks.set_offset() convention from bench_meta_service.cpp), and the configuration surface is thoughtfully designed with separate RNG seeds for input vs. output tokens.
🛡️ Key Risks & Issues
-
Barrier deadlock on early thread exit (critical): The round barrier is initialized with
cfg.threadsparticipants, but if any worker failsWaitForConnectedand returns early (or exits the outer loop due tog_stop_flag), the remaining threads will deadlock permanently atbarrier->Wait(). SIGINT cannot break it because the barrier predicate doesn't checkg_stop_flag. See inline comment for suggested fixes. -
Rate-limiting RNG coupled to input token RNG (medium): The Poisson/Uniform sleep draws from the same
rngengine used for deterministic sub-question token generation, meaning the-R/-Dflags silently alter the token workload even under the same-sseed. This breaks the reproducibility contract. See inline comment.
🧪 Verification Advice
- The manual test plan covers the happy path well. Before merge, it would be worth also verifying: (1) the
-Bbarrier mode with a small thread count, (2) SIGINT during an active-Brun to confirm it doesn't hang, and (3) bothpoissonanduniformdistribution modes produce reasonable inter-request timing. - The hit rate calculation (
locations_size() * block_size) is an approximation that over-counts partial trailing blocks. The cap attokens_queriedprevents >100% reporting, but it's worth a mental note that the per-round numbers are block-granularity approximations, not exact token counts.
💡 Thoughts & Suggestions
- The
ComputeStatspercentile computation useslats[n * 0.99]which is a nearest-rank approximation — perfectly fine for a benchmark tool, just noting it. - The
%ldprintf format specifier forint64_tvalues (lines 679-690, 753-754) is technically non-portable;PRId64from<cinttypes>would be more correct, though in practice this is unlikely to matter on the target platforms.
🤖 Generated by Qoder • View workflow run
| if (cfg.distribution == "poisson") { | ||
| std::exponential_distribution<double> exp_dist(cfg.request_rate); | ||
| sleep_s = exp_dist(rng); | ||
| } else { | ||
| double avg_interval = 1.0 / cfg.request_rate; | ||
| std::uniform_real_distribution<double> uni_dist(0, 2 * avg_interval); | ||
| sleep_s = uni_dist(rng); | ||
| } | ||
| std::this_thread::sleep_for(std::chrono::microseconds(static_cast<int64_t>(sleep_s * 1e6))); |
There was a problem hiding this comment.
The rate-limiting logic here draws from rng — the same RNG engine used for deterministic input token generation (sub-question tokens at line 407 via token_dist(rng)). This means changing -R or -D flags will shift the RNG state and produce different token sequences even with the same -s seed, silently breaking the reproducibility guarantee documented for -s.
Consider using a dedicated RNG for rate-limiting sleeps, or reuse output_rng since it's already the non-deterministic engine.
🤖 Generated by Qoder • Fix in Qoder
![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: fb0c86bb86
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| metrics.per_round[round].fail_count++; | ||
| continue; |
There was a problem hiding this comment.
If FinishWriteCache fails after StartWriteCache succeeded, the code just increments fail_count and continues, leaving that write session unfinalized. Because each start call sets a 60s write timeout, these abandoned sessions can remain pending long enough to affect later rounds for the same keys (extra misses/failures), which distorts the hit-rate and latency results under transient RPC errors.
Useful? React with 👍 / 👎.
…etaService Add a new benchmark tool that simulates multi-turn conversations to measure per-round cache hit rates against MetaService gRPC interface. Key features: - Simulates GetCacheLocation + StartWriteCache + FinishWriteCache per round - Configurable client count, round count, prompt/output/sub-question token sizes - Auto-detects block_size via GetInstanceInfo for accurate token-level hit rate - Supports round barrier mode for synchronized multi-client benchmarking - Separate RNG seeds for input tokens (-s) and output tokens (-S) - Poisson/uniform request rate distribution with configurable pacing 🤖 Generated with [Qoder][https://qoder.com]
fb0c86b to
9390acd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9390acd913
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| bool get_ok = get_status.ok() && get_resp.header().status().code() == proto::OK; | ||
| int64_t tokens_hit = 0; | ||
| int64_t tokens_queried = static_cast<int64_t>(cs.token_ids.size()); |
There was a problem hiding this comment.
GetCacheLocation/StartWriteCache with token_ids only operate on full blocks, because CacheManager::GenKeyVector truncates to tokens.size() / block_size (see kv_cache_manager/manager/cache_manager.cc, GenKeyVector). Here tokens_queried is set to cs.token_ids.size(), so any tail tokens that do not form a complete block are counted in the denominator even though they are never queryable/hittable. This systematically underreports hit rate and can make short-prompt runs look like 0% hit despite no full blocks being queried.
Useful? React with 👍 / 👎.
| clients.push_back(std::move(cs)); | ||
| } | ||
|
|
||
| for (int round = 0; round < cfg.num_rounds && !g_stop_flag.load(std::memory_order_relaxed); ++round) { |
There was a problem hiding this comment.
Barrier can stall remaining threads ~200ms x N on SIGINT when threads
diverge between rounds:
When g_stop_flag fires between rounds (after a barrier releases but
before all threads re-enter the loop), some threads may exit at the
outer loop check (!g_stop_flag) without calling barrier->Wait(),
while others enter the next round's body, process clients, and then call
barrier->Wait(). The waiting threads poll every 200ms and eventually
see g_stop_flag, so there's no deadlock — but the barrier's internal
count_ becomes permanently out of sync.
Impact: Graceful shutdown under -B mode may be delayed by up to 200ms
(one poll cycle). No deadlock or data corruption. Functionally benign
for a benchmark tool.
Fix: After the outer for loop exits, call barrier->Withdraw() if the
thread didn't complete all rounds, or accept this as a known ~200ms
shutdown delay and document it.
| PrintUsage(argv[0]); | ||
| return 1; | ||
| } | ||
| if (cfg.num_clients <= 0 || cfg.threads <= 0 || cfg.tokens_per_request <= 0 || cfg.num_rounds <= 0) { |
There was a problem hiding this comment.
Trivial: the validation block checks num_clients, threads,
tokens_per_request, and num_rounds for > 0, but output_tokens and
sub_question_tokens are not validated. Since atoi() is used for
parsing, -o -5 produces output_tokens = -5. The loop simply doesn't
execute, but it silently produces misleading 100% hit rates from round
1 onward.
Fix:
if (cfg.output_tokens < 0 || cfg.sub_question_tokens < 0) {
fprintf(stderr, "Error: output_tokens and sub_question_tokens must be >= 0\n");
return 1;
}
| // ── Rate limiting ── | ||
| // Per-thread rate = total target QPS / num_threads, so that the | ||
| // aggregate rate across all threads equals cfg.request_rate. | ||
| if (cfg.request_rate > 0) { |
There was a problem hiding this comment.
Trivial: atof() for -R can return negative values. The guard
if (cfg.request_rate > 0) treats negative rate the same as zero (no
rate limiting), so there's no crash. But it silently disables pacing
when the user likely intended to set a rate, which can overwhelm the
target server with unbounded QPS.
Fix:
if (cfg.request_rate < 0) {
fprintf(stderr, "Error: request_rate must be >= 0 (0 = unlimited)\n");
return 1;
}
| " distribution: %s\n" | ||
| " block_size: %d\n" | ||
| " round_barrier: %s\n" | ||
| " seed: %ld\n" |
There was a problem hiding this comment.
Trivial: use %PRId64 instead of %ld for int64_t?
oldsharp
added
review ready
2 participants
Summary
bench_hit_ratetool undertools/bench_meta_servicethat simulates multi-turn conversations to measure per-round cache hit rates against MetaService gRPC interfaceGetCacheLocation(prefix match) →StartWriteCache→FinishWriteCache, then extends token history with output + sub-question tokens for the next roundblock_sizeviaGetInstanceInfofor accurate token-level hit rate calculation (locations are block-granularity)Key features
-B) for synchronized multi-client benchmarking-s, deterministic) and output tokens (-S, random by default)Example usage
Test plan
bazel build //tools/bench_meta_service:bench_hit_rate🤖 Generated with [Qoder][https://qoder.com]