[Store] Make batch_evict_bench scale and eviction ratios configurable#2855
[Store] Make batch_evict_bench scale and eviction ratios configurable#2855jacklin78911-collab wants to merge 3 commits into
Conversation
Add three optional environment variables to the BatchEvict benchmark so the 10M-objects / 5%-eviction workload discussed in the eviction optimization work can be reproduced directly from the main-branch benchmark: - MOONCAKE_EVICT_BENCH_OBJECTS: run a single custom scale - MOONCAKE_EVICT_BENCH_TARGET_RATIO: eviction target ratio (default 0.50) - MOONCAKE_EVICT_BENCH_LOWERBOUND_RATIO: lower-bound ratio (default 0.25) Defaults are unchanged: without these variables the benchmark runs the same scales (10k, 100k, +1M with MOONCAKE_EVICT_BENCH_LARGE=1) with the same ratios and identical CSV output. Invalid values fail loudly with a non-zero exit instead of silently falling back. Benchmark-only change.
There was a problem hiding this comment.
Code Review
This pull request introduces dynamic configuration of eviction ratios and benchmark object counts via environment variables in the batch eviction benchmark. It adds helper functions to parse and validate these environment variables. The feedback highlights a critical issue in ReadStrictEnvSize where std::getenv can return nullptr, leading to undefined behavior when constructing a std::string_view without a prior null check.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| return false; | ||
| } | ||
|
|
||
| if (std::getenv("MOONCAKE_EVICT_BENCH_OBJECTS") != nullptr || |
There was a problem hiding this comment.
Could we expose these three new benchmark parameters as gflags command-line options instead of environment variables, for example --num_objects, --evict_ratio_target, and --evict_ratio_lowerbound. This benchmark already uses gflags, and CLI flags are consistent with the other Store benchmarks, discoverable through --help, and avoid unnecessary custom environment-variable parsing.
e050988 to
10a6dda
Compare
yokinoshitayoki
left a comment
There was a problem hiding this comment.
Thanks for your contribution, this PR looks good to me now.
Description
Adds three optional gflags CLI flags to the BatchEvict benchmark so the 10M-objects / 5%-eviction workload discussed in the eviction-optimization work can be reproduced directly from the main-branch benchmark:
--num_objects: run a single custom scale when greater than 0;0preserves the default scale list--evict_ratio_target: eviction target ratio (default 0.50)--evict_ratio_lowerbound: lower-bound ratio (default 0.25)Switched from environment variables to gflags CLI flags per review.
Defaults are unchanged: without these flags, the benchmark runs the same scales (10k, 100k, +1M with
MOONCAKE_EVICT_BENCH_LARGE=1) with the same ratios and identical CSV output. Invalid ratio combinations and invalid flag values fail with a non-zero exit. The existing lock-probe environment variables and their parsing are untouched. This is a benchmark-only change; no production code is modified.Follow-up to the benchmark added in #2584; relates to the eviction-cycle discussion in #2560.
Module
mooncake-store)Type of Change
How Has This Been Tested?
Test commands: