Dio/tps optimization agent loop - #4849
Open
lushengguo wants to merge 27 commits into
Open
Conversation
…JSON output - Port starcoin-execute-bench from align-interface-for-value-serde branch - Add pipeline.rs module for 4-stage timing (TxPool Verify, Block Build, VM Execute, State Commit) - Add JSON export for AI agent loop consumption (benchmark_results.json) - Add StageTiming struct with min/max/avg/median/p95/p99/throughput metrics - Add set_max_per_sender method to TxPoolConfig - Update workspace Cargo.toml to include new crate The framework now supports: - Full-chain benchmark with real node - Transaction latency tracking (Added -> Executed) - Block-level TPS statistics - SVG visualization with plotters - JSON output for automated analysis
Add a global timing collector to measure pipeline stage durations: - TxPool Verify: transaction signature validation - Block Build: DAG block packaging - VM Execute: transaction execution - State Commit: state persistence Integrate timing instrumentation into: - txpool/pool_client.rs: TxPool Verify timing - miner/block_builder_service.rs: Block Build timing - chain/chain.rs: VM Execute and State Commit timing - test-helper: expose timing data after node stop
Add --agent-mode to starcoin-execute-bench with: - BottleneckAnalyzer: identify pipeline bottlenecks by time percentage - OptimizationSuggester: generate actionable optimization suggestions - HistoryStore: track benchmark history for trend analysis - RegressionDetector: detect performance regressions vs baseline - AgentLoop: orchestrate analysis and generate structured output Output includes: - Pipeline stage timing breakdown - Bottleneck severity classification - Prioritized optimization suggestions - Regression alerts with severity levels - Action items for AI agents Usage: cargo run -p starcoin-execute-bench -- --agent-mode
Add advanced optimization loop components: - KnowledgeBase: record optimization history and learn patterns - StrategyTracker: manage optimization attempt lifecycle - ConfigTuner: auto-tune configuration parameters with rollback - ExperimentFramework: A/B testing with statistical significance - IterationController: orchestrate full optimization loop This enables automated optimization cycles: Analyze → Select Strategy → Apply → Verify → Commit/Rollback → Learn Note: Knowledge persistence is designed to be complemented by AI-written documentation in docs/ for team sharing.
Add team-shared optimization documentation: - docs/OPTIMIZATION_KNOWLEDGE.md: learned strategies and patterns - docs/BENCHMARK_HISTORY.md: historical benchmark results - docs/LESSONS_LEARNED.md: successes, failures, and insights - docs/adr/: Architecture Decision Records for major changes Add .github/copilot-instructions.md to guide AI agents: - Read optimization docs before suggesting changes - Update docs after successful optimizations - Create ADRs for architectural decisions This enables knowledge sharing across team members and AI agents through version-controlled documentation.
- Remove unused modules (iteration, config_tuner, experiment, strategy, pipeline) - Remove AI optimization docs (will be regenerated as needed) - Add benchmark README with usage instructions - Add #[allow(dead_code)] for future-use methods - Remove iterate/target-tps CLI options (not needed for CI) - Fix unused import warnings
- Remove CLI parameters: --history-dir, --knowledge-dir, --tags, --agent-output - Delete modules: history.rs, knowledge.rs, regression.rs, suggester.rs - Simplify agent_loop.rs to pure stateless analysis - Benchmark now outputs analysis directly without persisting data
- Add block_count to BenchmarkStats for sample size tracking - Increase default account-count from 20 to 8000 for better sampling - Display sample quality indicator (GOOD/ACCEPTABLE/LOW) - Highlight Median TPS as CI recommended metric (more stable) - Show clear CI-recommended vs raw metrics in output
…lock_count - 8000 accounts caused funding timeout (only 360/8000 funded in 180s) - Add #[serde(default)] to block_count for backward compatibility - 4000 accounts provides good balance of stability and speed
Stable TPS calculation: 1. Exclude first and last blocks (edge effects) 2. Use trimmed mean (remove top/bottom 10% outliers) 3. Only use middle blocks for calculation New fields in BenchmarkStats: - stable_tps: the CI recommended metric - middle_block_count: number of blocks used for stable calculation Expected stability improvement: ±50% -> ±10%
- Add --rounds CLI parameter (default: 3) to repeat benchmark transactions - More rounds = more blocks = better statistical significance - Auto-calculate txpool size based on rounds - Each round reuses same accounts with incrementing sequence numbers
…ntervals When STARCOIN_FIXED_BLOCK_TIME is set, DummyConsensus uses fixed sleep time equal to difficulty (block_time_target) instead of random range. This reduces TPS measurement variance in benchmarks.
Adds a convenient CLI option to enable fixed block time intervals. Equivalent to setting STARCOIN_FIXED_BLOCK_TIME env var.
Root cause of TPS variance identified and fixed: PROBLEM: - TPS calculation used event processing time (exec_ts) as transaction execution time - But exec_ts is recorded when NewHeadBlock event is processed, not when txns execute - Event processing includes try_submit_next_batch() which is slow - This causes event queue delays, making exec_ts unreliable SOLUTION: - New calculate_tps_from_block_timestamps() method - Uses block header timestamps (set when block is created by miner) - TPS = total_txns / (last_block_ts - first_block_ts) - Not affected by event queue delays REMAINING VARIANCE (~25% CV): - Real execution time variance due to system load, GC, I/O - This is actual performance variance, not measurement error - For CI: recommend running 3-5 times and taking median
…istence - Add --prepare-bench mode: funds accounts, signs transactions, saves chain data + signed txns to disk for reuse across benchmark runs - Add --load-bench mode: loads pre-signed txns, copies chain data to temp dir, starts node, imports txns to txpool, runs benchmark - Fix expire_time: use now_secs()+40000 instead of +1 year to stay within on-chain transaction_timeout (86400s) upper bound check - Change --rounds default from 3 to 10, --settle-delay-ms from 10000 to 3000 - Add build_node_config() with disable_seed=true for isolated benchmarks - Add start_node_and_wait() with sync state polling - Add GenerateBlockEvent broadcast after direct txpool import - Remove debug try_read() method from TxPoolService Inner - Remove stale commented-out inner_status code
- Fix clippy::unwrap_or_default: use or_default() instead of or_insert_with(Vec::new) in pipeline-timing - Fix clippy::collapsible_if: merge nested if conditions in results.rs - Fix clippy::useless_conversion: remove redundant .try_into() on TokenCode in main.rs - Fix trailing whitespace in chain.rs, pool_client.rs - Add CI benchmark workflow (tps_benchmark.yml) and comparison script (bench_compare.py) - Add pipeline stage-level regression detection with per-stage thresholds
# Conflicts: # Cargo.lock # sync/starcoin-execute-bench/Cargo.toml # sync/starcoin-execute-bench/src/main.rs # sync/starcoin-execute-bench/src/results.rs
The test passed observed_tps=50.0 but the theoretical max TPS from the slowest stage (Block Build avg_ms=50.0) is only 1000/50=20.0. Since theoretical_max < observed, improvement_potential_pct was 0.0, failing the assertion. Changed observed_tps to 15.0 to match realistic data.
- Fix typo 'Stat)less' -> 'Stateless' in agent_loop.rs doc comment - Add 'text' language tag to fenced code block in README.md (MD040) - Remove trailing '--' from git checkout commands in CI workflow (was incorrectly used as path separator instead of ref checkout)
- Detect supported flags via --help instead of hardcoding - Handle missing benchmark_results.json from older bench binary - Guard PR comment step with has_report output flag - Guard compare step against missing bench_current.json
…imization_agent_loop
📝 WalkthroughWalkthroughModified the TPS benchmark GitHub Actions workflow to increase job timeout from 30 to 60 minutes. Restructured the pipeline to save the current-branch benchmark binary, conditionally checkout and benchmark the base branch ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…imization_agent_loop
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/tps_benchmark.yml (2)
124-161:⚠️ Potential issue | 🟠 MajorOptional base benchmark can hard-fail the whole job.
This step is treated as optional (
has_basegating), butset -euo pipefailplus the benchmark pipeline means any non-zero base run aborts the job before current-run/report generation.Proposed fix
- set -euo pipefail + set -uo pipefail @@ - STARCOIN_FIXED_BLOCK_TIME=1 ./target/release/starcoin-execute-bench \ - $BASE_ARGS \ - 2>&1 | tail -20 + if ! STARCOIN_FIXED_BLOCK_TIME=1 ./target/release/starcoin-execute-bench \ + $BASE_ARGS \ + 2>&1 | tail -20; then + echo "⚠️ Base benchmark command failed — skipping base comparison." + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tps_benchmark.yml around lines 124 - 161, The base benchmark run (STARCOIN_FIXED_BLOCK_TIME=1 ./target/release/starcoin-execute-bench ...) can exit non-zero and, because of set -euo pipefail, will abort the whole job; change the step so the base bench is executed without causing the script to exit on failure (for example run the command in a way that its non-zero exit is tolerated and capture its exit code into a variable), still capture its stdout/stderr (tail -20 or log file) and then check for the existence of benchmark_results.json to set has_base and copy to bench_base.json; update references to HELP_TEXT and BASE_ARGS handling remain unchanged.
103-105:⚠️ Potential issue | 🟠 MajorWorkflow never switches back to PR/current checkout after running base branch.
After Line 104 (
git checkout "$BASE_BRANCH"), subsequent steps (includingscripts/bench_compare.py) execute from base-branch files, not the PR’s files. That can silently compare with the wrong script version.Proposed fix
+ - name: Switch back to PR/current commit + run: | + set -euo pipefail + git checkout --force "${GITHUB_SHA}" + git submodule update --init --recursive || trueAlso applies to: 166-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tps_benchmark.yml around lines 103 - 105, The workflow checks out the base branch with the command git checkout "$BASE_BRANCH" (and updates submodules) but never restores the PR/current checkout, so later steps (e.g., scripts/bench_compare.py) run against base-branch files; after the base-branch work completes, add a step to restore the original PR commit by checking out the pull-request commit (use git checkout "$GITHUB_SHA" or capture the original HEAD before switching and run git checkout "$ORIGINAL_HEAD"), then update submodules for the PR if needed; apply the same restore change to the other block referenced (lines 166-212) so all comparisons run from the PR's files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tps_benchmark.yml:
- Around line 95-101: The script copies ./target/release/starcoin-execute-bench
to ./bench_current_bin before running git stash --include-untracked, which
causes bench_current_bin to be removed and later steps to fail; move the cp
./target/release/starcoin-execute-bench ./bench_current_bin line to after the
git stash --include-untracked call (and apply the same reordering to the second
occurrence around lines 171-172) so bench_current_bin is created after stashing
and remains available for later use.
---
Outside diff comments:
In @.github/workflows/tps_benchmark.yml:
- Around line 124-161: The base benchmark run (STARCOIN_FIXED_BLOCK_TIME=1
./target/release/starcoin-execute-bench ...) can exit non-zero and, because of
set -euo pipefail, will abort the whole job; change the step so the base bench
is executed without causing the script to exit on failure (for example run the
command in a way that its non-zero exit is tolerated and capture its exit code
into a variable), still capture its stdout/stderr (tail -20 or log file) and
then check for the existence of benchmark_results.json to set has_base and copy
to bench_base.json; update references to HELP_TEXT and BASE_ARGS handling remain
unchanged.
- Around line 103-105: The workflow checks out the base branch with the command
git checkout "$BASE_BRANCH" (and updates submodules) but never restores the
PR/current checkout, so later steps (e.g., scripts/bench_compare.py) run against
base-branch files; after the base-branch work completes, add a step to restore
the original PR commit by checking out the pull-request commit (use git checkout
"$GITHUB_SHA" or capture the original HEAD before switching and run git checkout
"$ORIGINAL_HEAD"), then update submodules for the PR if needed; apply the same
restore change to the other block referenced (lines 166-212) so all comparisons
run from the PR's files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b115bbf-224c-414f-af46-2a5476649e4a
📒 Files selected for processing (1)
.github/workflows/tps_benchmark.yml
Comment on lines
+95
to
101
| # Preserve the current-branch binary so the base build can overwrite target/ | ||
| cp ./target/release/starcoin-execute-bench ./bench_current_bin | ||
|
|
||
| BASE_BRANCH="dual-verse-dag" | ||
| CURRENT_SHA=$(git rev-parse HEAD) | ||
|
|
||
| # Stash any untracked files from the current-branch run | ||
| # Stash any untracked files from the current-branch build | ||
| git stash --include-untracked || true |
There was a problem hiding this comment.
bench_current_bin is stashed away before it is used.
At Line 96 you save ./bench_current_bin, then at Line 101 git stash --include-untracked removes it from the working tree. The later run at Line 171 can fail with “No such file or directory”.
Proposed fix
- cp ./target/release/starcoin-execute-bench ./bench_current_bin
+ cp ./target/release/starcoin-execute-bench "${RUNNER_TEMP}/bench_current_bin"
@@
- STARCOIN_FIXED_BLOCK_TIME=1 ./bench_current_bin \
+ STARCOIN_FIXED_BLOCK_TIME=1 "${RUNNER_TEMP}/bench_current_bin" \Also applies to: 171-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/tps_benchmark.yml around lines 95 - 101, The script copies
./target/release/starcoin-execute-bench to ./bench_current_bin before running
git stash --include-untracked, which causes bench_current_bin to be removed and
later steps to fail; move the cp ./target/release/starcoin-execute-bench
./bench_current_bin line to after the git stash --include-untracked call (and
apply the same reordering to the second occurrence around lines 171-172) so
bench_current_bin is created after stashing and remains available for later use.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information
Summary by CodeRabbit