Add more cli opts to vm exec bench and optimize account manager #4768
Conversation
- Add CLI switches to control VM2 storage backend, concurrency, and reuse num_accounts for both benchmark modes - Split account creation into smaller persisted batches and cap batch size to avoid cache eviction and missing nodes
WalkthroughAdded a configurable storage mode (VmStorage enum) to benchmark initialization, supporting both in-memory and RocksDB-backed storage. Extended BenchmarkManager to accept account and concurrency overrides. Improved account generation with batch processing to prevent state eviction issues. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
executor/benchmark/src/main.rs(4 hunks)executor/benchmark/src/vm_exec_benchmark.rs(10 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4655
File: cmd/starcoin/src/account/sign_multisig_txn_cmd.rs:61-63
Timestamp: 2025-09-13T14:15:32.756Z
Learning: In Starcoin codebase PR #4655 (Clap v3 to v4 upgrade), maintainer welbon confirmed the preference for minimal configuration changes during the upgrade, specifically declining suggestions to add num_args or value_delimiter attributes to maintain CLI compatibility with existing behavior, even when it might mean less optimal UX compared to new Clap v4 features.
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4655
File: cmd/resource-code-exporter/src/main.rs:42-44
Timestamp: 2025-09-13T14:13:05.713Z
Learning: In the Starcoin codebase Clap v3 to v4 upgrade (PR #4655), the maintainer welbon prefers minimal configuration changes to ensure compatibility, even if it means slight UX differences, rather than adding additional attributes like num_args or value_delimiter.
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4633
File: vm/vm-runtime/Cargo.toml:48-48
Timestamp: 2025-09-01T03:56:58.362Z
Learning: In the Starcoin codebase, vm1 (starcoin-vm1-vm-runtime) may need to expose the same feature flags as vm2 to satisfy cross-version compatibility requirements when downstream projects like genesis depend on features from both versions. Empty feature declarations like `move-unit-test = []` may be intentionally added for compilation compatibility rather than to activate specific functionality.
📚 Learning: 2025-09-01T03:56:58.362Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4633
File: vm/vm-runtime/Cargo.toml:48-48
Timestamp: 2025-09-01T03:56:58.362Z
Learning: In the Starcoin codebase, vm1 (starcoin-vm1-vm-runtime) may need to expose the same feature flags as vm2 to satisfy cross-version compatibility requirements when downstream projects like genesis depend on features from both versions. Empty feature declarations like `move-unit-test = []` may be intentionally added for compilation compatibility rather than to activate specific functionality.
Applied to files:
executor/benchmark/src/main.rsexecutor/benchmark/src/vm_exec_benchmark.rs
📚 Learning: 2025-09-16T01:44:35.596Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4656
File: vm2/vm-runtime/src/starcoin_vm.rs:1622-1624
Timestamp: 2025-09-16T01:44:35.596Z
Learning: In the Starcoin codebase parallel execution optimization, passing `&data_cache.as_move_resolver()` instead of `data_cache` directly to `execute_user_transaction` is an intentional efficiency optimization to reduce multiple VM-Config loading calls, not a bug.
Applied to files:
executor/benchmark/src/main.rsexecutor/benchmark/src/vm_exec_benchmark.rs
📚 Learning: 2025-09-16T01:34:58.198Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4657
File: cmd/starcoin/src/dev/get_coin_cmd.rs:79-90
Timestamp: 2025-09-16T01:34:58.198Z
Learning: In Starcoin's get_coin_cmd.rs batch transaction submission (when repeat > 1), the maintainer lushengguo confirmed the preference for fail-fast behavior using the ? operator - the command should stop on the first transaction failure rather than collecting errors and continuing to submit remaining transactions.
Applied to files:
executor/benchmark/src/main.rsexecutor/benchmark/src/vm_exec_benchmark.rs
📚 Learning: 2025-09-15T13:00:33.502Z
Learnt from: lushengguo
Repo: starcoinorg/starcoin PR: 4659
File: executor/benchmark/src/vm_exec_benchmark.rs:434-435
Timestamp: 2025-09-15T13:00:33.502Z
Learning: In the Starcoin codebase, calling StarcoinVM::set_concurrency_level_once() after running serialized benchmarks is safe, as confirmed by the maintainer. The "once" semantics provide built-in protection against race conditions.
Applied to files:
executor/benchmark/src/vm_exec_benchmark.rs
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4648
File: miner/tests/miner_test.rs:87-93
Timestamp: 2025-09-14T15:08:48.415Z
Learning: In Starcoin test files (like miner/tests/miner_test.rs), jackzhhuang prefers to use std::thread::sleep for intentional blocking delays in test scenarios, even within actor event handlers, rather than using non-blocking ctx.run_later alternatives.
Applied to files:
executor/benchmark/src/vm_exec_benchmark.rs
📚 Learning: 2025-08-08T10:25:49.039Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/mock-service/src/lib.rs:114-120
Timestamp: 2025-08-08T10:25:49.039Z
Learning: In PR starcoinorg/starcoin#4605, txpool/mock-service/src/lib.rs: MockTxPoolService::next_sequence_number_with_header is currently unused; keeping todo!() in the mock is acceptable and won’t affect runtime unless invoked.
Applied to files:
executor/benchmark/src/vm_exec_benchmark.rs
🧬 Code graph analysis (1)
executor/benchmark/src/vm_exec_benchmark.rs (2)
config/src/storage_config.rs (1)
cache_size(189-191)storage/src/storage.rs (2)
new_cache_instance_with_capacity(86-90)new_cache_and_db_instance(95-100)
🔇 Additional comments (5)
executor/benchmark/src/vm_exec_benchmark.rs (3)
4-7: VmStorage enum and storage-related imports/constants look consistentThe added imports, storage-related constants (
CREATE_BATCH_SIZE,TXN_TTL_SECS), andVmStorageenum (withValueEnumderive for CLI integration) are cohesive and align with the new CLI-backed storage selection; no issues from a correctness or style perspective.Also applies to: 11-13, 32-35, 50-54
83-111: Account creation batching and TTL updates are logically correctChunking create-account transactions by
CREATE_BATCH_SIZEwhile computing association sequence numbers as a flatchunk_idx * chunk_size + offsetpreserves a unique, gap-free sequence per txn, and the increased TTL viaTXN_TTL_SECSis applied consistently to both creation and transfer txns without changing semantics otherwise.Also applies to: 133-134
231-235: Storage-mode-aware BenchmarkManager initialization is soundThe new
BenchmarkManager::new(storage_mode: VmStorage)cleanly separates in-memory vs RocksDB-backed setups, uses enlarged cache capacity in both modes, initializes genesis once, and retainsDataDirPathin_data_dirto keep the RocksDB temp dir alive until drop; the updatedDefaultimpl pointing toVmStorage::Memoryis also appropriate.Also applies to: 238-285, 356-359
executor/benchmark/src/main.rs (2)
5-5: CLI option extensions for VM2 benchmarking are well-scopedSwitching
num_accountstoOption<usize>and addingvm_concurrencyplusvm_storage: VmStorage(withvalue_enumand a"memory"default) cleanly extends the CLI surface for VM2 benchmarking without over-configuring clap; the options are narrowly targeted and self-documented.Also applies to: 9-10, 27-34
48-49: Wiring of new options into benchmark paths preserves expected defaultsThe VM2 bench path correctly passes
vm_storage,num_accounts, andvm_concurrencyintoBenchmarkManager::new/run, while the non-bench path keeps the historical default of 200 accounts viaopt.num_accounts.unwrap_or(200)before callingrun_benchmark, so behavior remains predictable with sensible overrides.Also applies to: 61-66, 69-70, 76-77
| pub fn run( | ||
| &mut self, | ||
| serialize_bench_txns: &[usize], | ||
| parallel_bench_txns: &[usize], | ||
| account_override: Option<usize>, | ||
| concurrency_override: Option<usize>, | ||
| ) -> Vec<BenchmarkReport> { | ||
| let mut reports = Vec::new(); | ||
|
|
There was a problem hiding this comment.
Enforce at least 2 accounts to avoid infinite loop in transfer generation
gen_transfer_transactions relies on self.accounts.len() / 2 to drive the inner for loop; if account_num < 2 (e.g., user passes --num-accounts 1), self.accounts.len() / 2 == 0, the for body never runs, and the outer loop never reaches the return, causing an infinite loop when txns_num > 0.
Currently BenchmarkManager::run only bumps account_num from 0 to 1, so this edge case is still reachable via CLI overrides.
Consider clamping to a minimum of 2 accounts:
- let mut account_num = account_override.unwrap_or_else(|| min(max_txns_once * 2, 20000));
- if account_num == 0 {
- account_num = 1;
- }
+ let mut account_num = account_override.unwrap_or_else(|| min(max_txns_once * 2, 20000));
+ if account_num < 2 {
+ account_num = 2;
+ }This keeps existing defaults while preventing hangs for small --num-accounts values.
Also applies to: 304-308, 312-316, 324-329
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
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.