Repository navigation
[CI/Build] Gate CPU and T-One RDMA performance regressions - #4639
stmatengss wants to merge 9 commits into
Conversation
github-actions
Bot
added
documentation
There was a problem hiding this comment.
🟡 Changes recommended
Timeout handling can prevent artifact publication, and workload freezing and malformed-policy handling are incomplete.
3 open findings
What changed in this PR
Adds a CI performance gate for allocator CPU regressions.
Changes:
- Compares paired baseline/candidate benchmark measurements.
- Publishes summaries, diagnostics, and artifacts.
- Adds policy, tests, workflow, and contributor documentation.
| File | Description |
|---|---|
.github/workflows/performance.yml |
Builds revisions and runs the gate. |
CONTRIBUTING.md |
Links the performance-gate documentation. |
scripts/ci/performance/README.md |
Documents behavior and calibration. |
scripts/ci/performance/perf_gate.py |
Collects, validates, and compares results. |
scripts/ci/performance/policy.json |
Defines workloads and thresholds. |
scripts/ci/tests/test_perf_gate.py |
Tests collection and comparison behavior. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
stmatengss
changed the title
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
@ykwd PTAL |
|
@copilot resolve the merge conflicts in this pull request |
# Conflicts: # mooncake-transfer-engine/benchmark/utils.cpp Co-authored-by: stmatengss <11641725+stmatengss@users.noreply.github.com>
Merged |
|
Hi, I got few questions regarding this PR: Second is, would it be better to add a queue under the current concurrency configurations? eg., maximum 100 parallel waiting tasks, and I think |
Aionw
left a comment
Aionw
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-up in a9cea2c. Dropping the inner timeout fixes the double SIGTERM on target shutdown, and queue: max fixes the pending-run replacement on the RDMA pair. Both look good to me.
I re-ran the harness tests on the current head (39 pass). Inline comments are below. The one I'd like resolved before this becomes a check on every PR is the frozen CMake recipes (freeze_build.py). The others are workflow hygiene or questions.
| for name, path in destinations.items(): | ||
| if name not in sources: | ||
| path.unlink(missing_ok=True) | ||
| for name, content in sources.items(): |
There was a problem hiding this comment.
Freezing every tracked CMakeLists.txt from the baseline breaks the CPU gate for any PR that adds, removes or renames a source file under mooncake-store/src. offset_allocator_concurrency_bench links the whole mooncake_store, whose sources are listed explicitly in MOONCAKE_STORE_*_SOURCES. A newly added .cpp is never compiled, so the link fails with undefined symbols. A deleted one fails at configure time with "Cannot find source file".
The README does say an incompatible build layout fails explicitly, but this case is common: 21 of the 234 commits on main in the last month added or removed a .cpp under mooncake-store/src or mooncake-common. Every one of those would have got a red ERROR here.
Could we narrow what gets frozen? For example, keep the benchmark source and mooncake-store/benchmarks/CMakeLists.txt frozen but use the candidate's library recipes. Or, if the candidate fails to build under the frozen recipes, report a neutral "not comparable" result instead of ERROR. (The RDMA gate is mostly unaffected because the TE recipes use file(GLOB ...).)
| @@ -0,0 +1,151 @@ | |||
| name: Performance regression | |||
|
|
|||
| on: | |||
There was a problem hiding this comment.
There's no paths filter, so docs-only or Python-only PRs also pay for two Release builds plus up to 15 minutes of benchmarking. Hosted-runner noise can then turn into an ERROR on a change that cannot affect the allocator. Could we limit this to mooncake-store/**, mooncake-common/**, top-level CMake and scripts/ci/performance/**? If the check is meant to become required, a small no-op job for skipped paths keeps it reporting.
| performance: | ||
| name: Performance Gate | ||
| needs: tests | ||
| if: ${{ always() }} |
There was a problem hiding this comment.
always() also runs after the run is cancelled. With cancel-in-progress: true, two quick pushes leave the superseded run still taking a runner for this job (which matters on PERF_RUNNER), failing "Require harness tests" and publishing an ERROR summary. if: ${{ !cancelled() }} keeps the "run even if tests failed" behavior without that. The same applies to the two always() steps below it.
| parser.add_argument( | ||
| "--result-origin", default=os.environ.get("TONE_RESULT_ORIGIN", "") | ||
| ) | ||
| parser.add_argument("--timeout", type=int, default=2700) |
There was a problem hiding this comment.
Question about the time budget. This 2700s deadline starts at job submission, so it includes T-One queueing. The remote worker's 2700s watchdog starts at SSH launch. The worker has to fit a clone, two tebench builds at -j4 (each self.command also has a 900s default), and 48 tebench runs (40 measured plus 8 verify). The measured runs alone take about 10–13 minutes. Do you have the wall-clock time of a full run on the target pair? If the builds take more than about 10 minutes each, routine runs will hit the deadline with nothing wrong in the code.
| for key, value in sample.items(): | ||
| values[key]["samples"].append(value) | ||
| finally: | ||
| remotes[1].request("stop") |
There was a problem hiding this comment.
Minor: if stop raises here, it replaces the exception from the sample, and only the stop error lands in errors[]. Since a9cea2c this mostly happens when the target actually died, so it's usually the more useful message anyway. Still, it may be worth catching it and recording both errors.
|
|
||
|
|
||
| def environment(cpu_count): | ||
| cpus = sorted(os.sched_getaffinity(0))[:cpu_count] |
There was a problem hiding this comment.
Nit: taking the first cpu_count CPUs from the affinity mask often gives two SMT siblings of the same core on hosted 4-vCPU VMs. The baseline and candidate share the pinning, so the comparison stays fair, but the -2t cases would measure SMT contention and show more spread. Picking CPUs from distinct cores (via topology/thread_siblings_list) would make the 2-thread cases more meaningful. Fine to leave as a follow-up.
OffsetAllocationHandle stores only a std::weak_ptr<OffsetAllocator>, but OffsetAllocator::allocate() and createHandleAtNode() construct it from shared_from_this(). Every successful allocation therefore promotes the allocator's internal weak reference to a shared_ptr (a compare-and-swap on the use count), converts that temporary into the stored weak_ptr (a weak-count increment) and destroys the temporary (a use-count decrement). Take a std::weak_ptr in the constructor and pass weak_from_this(), so an allocation performs only the weak-count increment. The handle still does not keep the allocator alive, and its destructor still locks the weak_ptr before freeing. The two calls differ only for an allocator that no shared_ptr owns: shared_from_this() throws std::bad_weak_ptr, while weak_from_this() returns an empty weak_ptr and the handle would never free. That cannot happen here, because OffsetAllocator's constructors are private and both create() and deserialize_from() wrap the new object in a shared_ptr. Existing tests fail if allocate() hands out an empty weak_ptr, and the new CreateHandleAtNodeAdoptsRestoredNode test fails if createHandleAtNode(), which restart recovery uses, does. In offset_allocator_concurrency_bench (the benchmark behind the CPU performance gate in #4639), successful_allocate_free of 4 KiB rose in median qps by 16.7% and 16.8% with one thread, by 5.9% to 11.2% with two (four runs) and by 5.8% with sixteen, on upstream 0afc417 with GCC 13.3. A/A runs of the base binary moved the same median by 0.6% or less. Co-authored-by: Daejun Park <daejun7.park@samsung.com>
Description
Add CI performance gates that compare a baseline and candidate, show per-case medians, regression percentages, thresholds and noise in the Actions Summary, and fail when a threshold is exceeded. The CPU suite covers the existing allocator benchmark. The opt-in RDMA suite measures TENT DRAM read/write on a fixed T-One host pair.
For RDMA, the GitHub controller submits exact baseline/candidate/harness SHAs, downloads a JSON result envelope, checks its provenance, and independently enforces the policy. The installable
mooncake-rdma-perftone-cli adapter leases both hosts, builds both revisions with the same benchmark source and provisioned RDMA libraries, runs an integrity smoke and five alternating pairs, and preserves raw logs in T-One. A small--dram_numa_nodebenchmark option fixes DRAM placement for this suite. Bandwidth loss above 5% or P99 growth above 10% fails; incomplete, invalid, unstable, stale, or failed executions also fail.Both suites freeze the baseline's tracked CMake recipes as well as the benchmark source, including target definitions, parent flags and included modules; candidate-only recipes are removed from disposable checkouts. The build manifest is preserved and RDMA results must carry matching configuration hashes. This compares implementation changes under a fixed build configuration; build-system changes require separate validation.
RDMA targets run directly in their own process groups so normal shutdown delivers one SIGTERM; the worker watchdog and forced cleanup still bound their lifetime. The shared GitHub concurrency group uses
queue: maxto retain up to 100 pending runs without replacing earlier waiting runs.Collection stops at the first failed, timed-out or malformed sample and preserves partial results. A 15-minute CPU collection budget, separate setup/build time limits, scoped process-group cleanup and the T-One stop API leave time for comparison/artifact publication. Invalid policy containers and malformed returned errors produce an ERROR report rather than an uncaught exception. Existing SGLang/vLLM T-One jobs are unchanged.
CI prerequisite: update the test DSL's lease-free
BatchProbeKeycall to pass the defaultGrantLeasePolicy, matching the current main API. The stale call was blocking CTest and four TENT build configurations.Deployment boundary: the adapter and example host configuration are included, but no live T-One template or host configuration has been changed. Automatic RDMA execution is off until
RDMA_PERF_ENABLED=true; the template, SSH/NIC/GID/NUMA configuration and HTTPS result-download origin must be commissioned first. Real-hardware A/A calibration is required before choosing production thresholds or making the check required. Fork PRs lack secrets and fail closed when enabled; a reviewed manual run does not automatically become a required check on the fork PR.Design/RFC: #4734. Overlap searches found transport/benchmark work but no existing T-One RDMA regression gate. This reuses native
tebenchand extends the existing CPU gate rather than adding a competing benchmark implementation.Suggested Merge Priority
Module
mooncake-transfer-engine)mooncake-store) — test helper compatibility onlyType of Change
How Has This Been Tested?
Test commands:
Test results:
a9cea2c5(14.7 seconds), including the native CMake/C++ workload-freezing fixture, policy/error reporting, fail-fast collection and budget limits, RDMA provenance/control fixtures, descendant cleanup, duplicate-SIGTERM detection, and rejection of dead or unresponsive targets. A separate process-fixture reproduction with real GNU timeout failed 20/20 times with the original target wrapper and 0/20 with the fix; this is control-path evidence, not a native RDMA measurement. CodeRabbit reviewed the four changed files and raised 0 issues.a9cea2c5; RDMA execution is disabled and its workflow is skipped.queuekey diagnostic explicitly excluded and shellcheck disabled (the unchanged CPU workflow has an SC2129 style diagnostic). GitHub accepted the RDMA workflow at this head.queue: maxis documented in GitHub concurrency controls.Checklist
./scripts/code_format.shvia the changed-line pre-commit hookAI Assistance Disclosure
Codex implemented the workflows, collectors/comparator, T-One adapter, NUMA benchmark flag, frozen build recipes, failure handling, test helper compatibility fix, tests and documentation, and ran the recorded local checks. The human submitter still needs to review every changed line and defend the change end-to-end. Real T-One/RDMA commissioning is explicitly pending; no new hardware performance results are claimed.