Repository navigation
perf(timing): reduce native replay evidence overhead - #295
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: ai-dynamo/aisimulate/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (21)
🧰 Additional context used📓 Path-based instructions (4)Review scheduler state transitions and event ordering, including cancel, preempt, drain, retry, duplicate, empty, and terminal paths.⚙️ CodeRabbit configuration file Files:
Treat top-level exports and bindings as public and release boundaries.⚙️ CodeRabbit configuration file Files:
Read REVIEW.md before commenting.⚙️ CodeRabbit configuration file Files:
Before making any change under: `python/aisimulate/src/aiconfigurator/generator/**` MUST read: `python/aisimulate/.claude/rules/generator-development.md` Before making any change under `python/aisimulate/collector/**` MUST read: `python/ais...📄 CodeRabbit inference engine (AGENTS.md) Files:
🔀 Multi-repo context ai-dynamo/dynamo, ai-dynamo/aiconfiguratorLinked repositories findingsai-dynamo/dynamo
ai-dynamo/aiconfigurator
🔇 Additional comments (3)
📝 SummaryRisk level: Medium. Human attention should focus on:
Changes
Public serialization, diagnostics, fallback implementations, and builder contracts are intended to remain unchanged. Evidence supplied
Evidence still missing
Technical quality and merge readinessThe implementation adds validation before commit, retains fallback behavior, and includes targeted parity and failure coverage. The main risks are divergence between native and Python evidence generation and incorrect behavior for uncommon operation layouts. The supplied evidence supports continued review. It does not establish merge readiness without independent review and CI or deployment results. WalkthroughThe change adds validated timing phases and atomic evidence accumulation. Python timing predictions use native providers, cached validated phases, and accumulator snapshots. Python conversion remains available for test parity and validation. ChangesNative timing evidence
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to Native replay timing now uses the in-process performance engine and a faster evidence accumulator. The existing checks show the same latency, energy, and coverage results as before. No concrete correctness, data, or availability problem was found at this head. Earlier suggestions about warnings in default-feature builds, duplicated logic, and test coverage remain optional follow-ups. 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
Comment |
7c0e0e8 to
2019d04
Compare
jthomson04
left a comment
jthomson04
left a comment
There was a problem hiding this comment.
Review: perf(timing) native replay evidence — 13 findings
Posting as a comment-only review: the PR is a draft, so no approve / request-changes.
What I actually verified (please read this first)
The fast paths are not wrong, as far as I can tell. The central risk in this PR is that the private accumulator and the name-folded builder must stay bit-identical to the checked implementations, so I tried to break that empirically rather than by reading alone. Everything below ran against head 2019d04c:
| Check | Result |
|---|---|
cargo check --workspace --offline (default features) |
compiles, 7 new warnings — see finding 1 |
cargo check -p aisimulate-core --offline --features embed-python --tests |
clean |
cargo test -p aisimulate-core --offline --lib engine::timing |
19/19 pass |
Randomized differential fuzz of TimingEvidenceAccumulator::record vs TimingPhaseEvidence::try_accumulate — 400 seeds × 40 steps |
no divergence |
Randomized differential fuzz of ValidatedTimingPhase::from_name_folded_operations vs try_from_operations — 3000 cases |
no divergence |
The accumulator fuzzer compared struct equality and to_bits() on every phase and per-operation latency_ms / energy_wms / covered_latency_ms, plus source, details and error strings, across: None/Some(0.0)/Some(-0.0)/subnormal/f64::MAX * 0.4 energies, signed-zero and overflowing latencies, all five TimingEvidenceSource variants including Other, ops carrying and not carrying details, and randomly varying op counts and name sets so that layout changes, duplicate-name folding and the checked-fallback branch were all hit. The builder fuzzer covered unique-name inputs up to 5 ops with the same value classes including f64::MAX.
So: none of the 13 findings below is an observed numeric regression. They are contract, hygiene and structural issues. I also independently re-derived the equivalence of the two implementations by hand (error ordering, reduce-vs-sum energy reduction, the canonicalized() energy filter, operation_totals_match(), phase-only fallback, source merge) and agree with the PR's reasoning.
I separately confirmed that run_static_per_op's PerOpFold really does name-fold in first-encounter order, that reset_provenance / parse_mode ordering matches the pyclass method exactly, that reset_provenance is an atomic on a shared PerfDatabase (so dropping the GIL introduces no new race), and that AicPhaseProvider::Native and self.engine wrap the same Arc<Engine>.
Confidence
All 13 are marked CONFIRMED inline — each is either verified by a command I ran or directly readable from the code. Three are flagged latent (findings 2, 3 and the details aspect of 4): the defect is real in the code but not reachable today, and would be triggered by a plausible future change rather than by current inputs. Nothing is posted as speculative.
Where I proposed a concrete change I offered a suggestion block only after applying it and re-running the builds and tests. Four qualified: the cfg gate here, the record early-return, the OnceLock empty phase, and DEFAULT_STATIC_STRIDE. Each was checked under default features, under --features embed-python --tests, against engine::timing's 19 tests, against my fuzzer, and with cargo fmt --check. The from_provider delegation is prose only, because the lines it would replace fall outside this PR's diff hunks.
Findings outside the diff
None — all 13 anchor to lines inside the changed hunks.
Retracted during review
I had a 14th finding claiming the provenance assertion at python.rs:2255 was vacuous in the missing-data half of native_phase_evidence_matches_python_bridge, on the theory that raw and missing are different engines. That is wrong and I withdraw it: missing is built with Arc::clone(raw.database()), and Engine::last_provenance() reads self.db.worst_provenance(), so the two engines share the atomic and the assertion is meaningful. Recording it here rather than dropping it silently.
Two things I'd genuinely push on
Of the 13, the two worth arguing about rather than just fixing are finding 4 (the private accumulator duplicates bit-exact arithmetic that has no compile-time tie to the original — #272's details field already forced a hand-patch, and the next one gets no warning; a try_accumulate_ref(&mut self, &Self) on TimingPhaseEvidence would deliver the same win from one implementation) and finding 13 (the test helper now defaults to a cfg(test)-only provider, so ten existing tests no longer cover the code that ships).
The measured 34% CPU reduction on AgentX looks well-earned; none of this is a reason to hold the performance work.
de81c65 to
4a95f4e
Compare
Harrilee
left a comment
Harrilee
left a comment
There was a problem hiding this comment.
Reviewed 4a95f4ea3f8208a96289698769890232dd55a328 against e8828036e3d32d8361e2abb819e0ee60e569f518. No blocking findings after tracing provider construction, cache ownership, checked/fast accumulation, energy/coverage/source handling, error precedence and atomicity, empty work, and diagnostics fallback.
Independent correctness evidence on this head:
- Fresh optimized native builds with identical Cargo/uv locks and the same 1,537 performance-data files.
- Embedded-Python Rust suite: 1,585 passed, one existing ignored test; native-versus-Python bridge parity explicitly executed. Power tests: 164 passed.
- Additional standalone differential harness: 100,000 randomized inputs; 94,756 successful updates matched the checked accumulator bitwise, 2,087 constructor rejections, and 3,157 matching atomic overflow rejections. All 28 included timing/support/differential tests passed.
- Public AgentX: all 4,198 per-request results and all 105 report keys, including timing evidence, matched exactly except the three host-speed metrics (
wall_time_ms,processed_tokens_per_s,processed_output_tokens_per_s). The deterministic synthetic fixture likewise matched all 256 requests and 104 report keys. Every counted performance pair also had zero prediction/evidence differences. - Diagnostics enabled (
--detail time,source) also matched exactly on a separate 162-request trace. These benchmark profiles have zero positive power coverage; positive-energy arithmetic is covered separately by the power tests and randomized differential checks. - Independently downloaded the exact-head CI old/new prediction artifacts: all 30 CSVs were byte-identical at their serialized precision (170,800 rows, including 83,379 successful predictions; remaining misses/invalid/error outcomes were also unchanged). No goldens changed.
Independent end-to-end measurement: optimized Linux x86_64 builds on an i9-14900K, CPU 4 pinned, warm filesystem caches, default diagnostics/capture off, fixed five alternating revision pairs per workload. CPU includes the CLI supervisor and waited descendants; subprocess accounting was separately verified. All 20 counted runs are retained.
| Workload | Base CPU median | PR CPU median | CPU reduction | Base wall median | PR wall median | Wall reduction |
|---|---|---|---|---|---|---|
| Public AgentX, 32 plays | 74.040 s | 41.743 s | 43.62% (1.774x) | 74.077 s | 41.795 s | 43.58% |
| Synthetic long decode | 1.807 s | 1.596 s | 11.70% (1.133x) | 1.821 s | 1.669 s | 8.33% |
AgentX uses public Weka dataset revision 8fecd2fc56694469f758f0afbbb6335ad3043740, first 32 rows, 4,198 requests / 4,072,673 output tokens, Qwen3.8-2.4T-A95B-FP8 / GB300 / SGLang 0.5.17 / TP16 / MoE TP4 EP4, one client-play lane, explicit trace/KV block size 64. Input SHA-256: ba7a19e045828db8d8299e71305d046b2ed3259e515907711a9a15e05d6c140c. Synthetic uses 256 independent one-request plays, 32 lanes, 1,024 input / 4,096 output tokens, stable identities, and the same model/hardware topology.
The AgentX CPU reduction's nominal paired-log Student-t 95% interval is 38.49–48.39%; synthetic is 10.59–12.40%. The host is shared and brief unrelated compiler activity was observed; all samples and contemporaneous load records were retained. These intervals are small-sample estimates, not isolation guarantees. This is an independent, similarly sized public workload, not the author's original 4,265-request trace. The claimed speedup direction and magnitude are credible.
Setup receipts are retained: an initial resource precheck overestimated trace memory when source block size was omitted; explicitly matching the corpus's block size 64 admitted both revisions without changing token lengths. The initial non-agentic synthetic fixture generated random UUIDs and showed only tiny aggregation-order differences (about 6e-11 maximum) after requests were aligned; the stable-identity fixture above eliminates those differences without changing production code.
I also reviewed the five open, explicitly deferred author self-review threads. Current callers uphold the private builder's unique-name contract, and the independent checked reference and existing error mapper preserve the intended compatibility boundary. Those maintenance discussions do not establish a reachable blocking defect in this head. Their resolution and branch currency remain repository merge requirements.
Full CI run 35649460908 passed on this exact head. Approval is scoped to this commit.
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Follow the existing embedded round-trip test policy: skip an unavailable SDK when optional and fail when the test is required. Keep later construction, data, and assertion failures visible. Clarify the private provider-input contract used by the empty-update shortcut. Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
4a95f4e to
50ab2ab
Compare
3 participants
Summary
Native replay added per-operation timing evidence in #211. Each prediction then copied and revalidated evidence, and each cache miss converted operator results through Python before constructing evidence again. This PR reduces that CPU work while preserving latency, power, energy, coverage, source attribution, diagnostics, defaults, and errors.
Arc, and borrow them during accumulation.run_static_per_opon the native engine returned by the canonical constructor. Keep the Python bridge as a test-only differential reference.The five original implementation commits remain separate, followed by three signed review-fix commits. The full diff is four Rust files, +1093/-89 lines. No public API, model data, golden, estimator selection, cache key/capacity, mutex, scheduler, or reporting changes.
Latest rebase: head
50ab2abb591c1714d4a3cabc0bbb4bdce89dc478is based on maine95d37863fb30232e089b21f058bc28d1dfa1729. All eight signed commits remain. The rebase preserves main's evidence-reset test, updates the reset method to useTimingEvidenceAccumulator, and supplies the neworiginal_fmha_quant_modefield in the missing-data test fixture.git range-diffconfirms that the remaining patch changes are context adjustments. No benchmarks were rerun after this rebase; all measurements below describe the explicitly pinned earlier revisions.The fresh comparison below measures the full PR against its pinned base. It does not isolate the two review-fix commits or make a separate performance claim for the zero-work changes.
Review map
crates/core/src/engine/timing.rs: the immutable wrapper, private accumulator, and native-row builder. Reviewcrates/core/src/python.rsfor native-provider ownership, conversion, and cache publication.Validation
Local checks for the latest rebase (
50ab2abb):embed-python,replay-bench: 1,788 passed, one existing ignored test.AIC_REQUIRE_EMBEDDED_ROUND_TRIP=1was set; native-versus-Python parity, the embedded round trip, and the preserved evidence-reset test all executed and passed.Local rebase checks at
4a95f4ea:AIC_REQUIRE_EMBEDDED_ROUND_TRIP=1was set, so native-versus-Python parity executed.Local checks at review head
7fddbd927f760ce3122992eede314a3be91fbc5d:embed-python. Each has one existing ignored test. Debug contract assertions are exercised.The initial embedded-Python run used a stale library path in the relocated Python installation and failed importing
_contextvars. After correcting the test environment, the complete suite passed without a source change.The differential tests compare numeric fields by bits against the unchanged checked implementations. They cover repeated/changed layouts, duplicate-name folding, missing and positive energy, partial coverage, unknown sources, signed zero, invalid inputs, overflow, error precedence, and unchanged state after failure. Native-versus-bridge checks cover prefill/decode, prefixes, changing contexts, cache hits, provenance, invalid-mode errors, and missing data. No goldens were refreshed.
Fresh release-build qualification at
7fddbd92has exact per-request results and exact summaries/evidence on AgentX and the fixed-ID synthetic fixture. Exclusions are onlywall_time_ms,processed_tokens_per_s,processed_output_tokens_per_s, and generated request UUIDs. The original random-ID synthetic run has exact request results and evidence, with three retained aggregation-order differences:mean_e2e_latency_ms(147950.05294399973 vs 147950.05294399976),std_ttft_ms(363.6550848106251 vs 363.65508481062517), andstd_tpot_ms(0.08880466051541563 vs 0.08880466051541555). The fixed-ID fixture matches exactly.Matched measurements before the September 21 rebase
Pinned main
7ae483e942b0a2593d55e4e30957f94640580441versus PR7fddbd927f760ce3122992eede314a3be91fbc5d. Both fresh builds include #272 and #289. Separate worktrees, Python environments and Cargo targets use matching dependencies and measured data. Native--stack engine, CPU 4, optimized builds with debug symbols, warm filesystem caches, and no concurrent builds or profiles. Optional timing/source-detail diagnostics are off.Values are medians. CPU includes the supervisor and descendants. Both workloads improve in both pairs; all eight timed runs are included, with no exclusions. Expected completion and output-token counts pass in every run. Revision order reverses in the second pair.
The local confirmation worktree retains
REPORT.md,RESULTS.md,REPRODUCE.md, full provenance, qualification comparisons and every raw run. No new profile or microbenchmark was collected; the profiles below remain historical.Matched measurements before the final #272 rebase
Previous main
7cfa4f20versus the earlier rebased PR7c0e0e89. These measurements do not include #272 or describe the post-#272 review head. Both builds include the same later main changes, including #289. These results measure the full PR, not only its last two commits.AgentX: nominal paired 95% CPU interval [-43.70%, -22.70%]; wall interval [-43.61%, -22.86%]. CPU improves in 2/2 pairs.
Synthetic: nominal paired 95% CPU interval [-13.65%, -11.97%]; wall interval [-17.34%, -5.63%]. CPU improves in 2/2 pairs.
That pre-#272 two-pair comparison passed the CPU/wall regression checks. Its AgentX interval crosses the original 25% improvement target. That earlier sample did not establish the 25% target; its stopped third pair remains excluded. Pairs alternate revision order. Paired Student-t log-ratio intervals are nominal small-sample intervals, not adjusted for adaptive stopping. All trials, ranges, per-token costs, replay time and both memory measures are retained in
RESULTS.md.No new profile or microbenchmark was collected for either rebase. Profiles and diagnostic values below describe the preserved historical revisions only.
Historical stage measurements (before rebase)
Two separate campaigns measure the stages. Do not add their percentages or treat them as one matched baseline-to-final measurement.
a2eae639→92ea946a(cache, accumulation, native bridge)a2eae639→92ea946a92ea946a→0e35fae1(source conversion and builder)92ea946a→0e35fae1The first campaign's AgentX nominal paired 95% interval is a 25.42–27.51% reduction; all six pairs improve. The builder campaign's interval is an 8.28–11.83% reduction; both pairs improve. Its AgentX wall time falls 10.08%, and synthetic wall time falls 2.69%. All four synthetic pairs improve. Both campaigns meet their respective 25% and 5% CPU gates and the 3% CPU/wall regression limits.
Both campaigns start with two alternating pairs and add pairs only to resolve a threshold. In the builder campaign, only the synthetic CPU interval needed two more pairs. Intervals use paired Student-t log ratios and assume independent, approximately normal pair log ratios. These are nominal small-sample intervals, not adjusted for adaptive stopping.
Native
--stack engine, serial performance CPU 4, warm filesystem caches, matching Python/Rust/dependencies/data, separate optimized builds with debug symbols, and no concurrent builds or profiles during measurement. Process CPU includes waited and adopted descendants, verified by accounting probes. Detailed request capture is off during timing.The harness, configurations, traces, complete reports, raw profiles, and flamegraphs remain in the profiling worktrees and are not part of the production diff. Exact commands for the pre-rebase confirmation:
These commands refer to the retained local harness. It refuses to replace existing run directories; use unused round numbers to repeat measurements.
Modeling or data provenance
No estimation or measured-data changes. The pre-rebase comparison uses matching 132 external Python packages, Cargo/uv locks, and 1,483 measured-data files (126,134,657 bytes). Full identities and config hashes are retained in
analysis/provenance.json.5b017086065f0923f7db054e2aa685276f5bad2a1df6da0bc3685732591b2410.d156d09055ce272e3e490bb8490ed3379e579435947a4fedd76beaed16818abe.7cc9fcf593f8e7b36e9076066ccc516d97f738815c87f75f71b757bf2944c10b.c955adb4395ddc8c61c7a431643fb118a793f2d1ac5cbda9515eaa544406d4c7.f2d378b30231e8957e8ba2294a67ad4f55e9518827942d41ccc0963357aa98d0.c364db87b30914c885afd29c6162f1c51da71012176fe1c832544dc42e23458e.028c123ea7e63b8005d720157ac8aacf6d87ca9e2ddfec1f33ae67ea718d12cb.The historical ~60% slowdown is context for this work, not a claim that all historical overhead has been removed. The historical stage comparisons exclude later main changes, including #289; the pre-rebase comparison includes them in both variants.
Tracking