vm2: add layout identifier-mapping cache and benchmark probe - #4839
Open
jackzhhuang wants to merge 11 commits into
Open
vm2: add layout identifier-mapping cache and benchmark probe#4839jackzhhuang wants to merge 11 commits into
jackzhhuang wants to merge 11 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThese changes introduce a caching mechanism for delayed-field identifier mappings in the VM runtime, update RPC block queryability tests, adjust materialization control flow to account for delayed fields, and refine batch index computation in benchmarking utilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13d5389217
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jackzhhuang
changed the base branch from
align-interface-for-value-serde
to
dual-verse-dag
March 31, 2026 01:57
jackzhhuang
force-pushed
the
spike-layout-cache-benchmark
branch
from
March 31, 2026 01:58
13d5389 to
cdbb6d4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
rpc/client/tests/chain_get_block_txn_infos_in_seq_test.rs (1)
218-218: Consider initializinglast_errfor clarity.While the Rust compiler can verify that
last_erris always assigned before use (every loop iteration either returns early or assigns tolast_err), explicit initialization improves readability:✨ Suggested change
- let mut last_err: Option<anyhow::Error>; + let mut last_err: Option<anyhow::Error> = None;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rpc/client/tests/chain_get_block_txn_infos_in_seq_test.rs` at line 218, Initialize the variable `last_err` to improve readability: change the declaration of `last_err` in the test `chain_get_block_txn_infos_in_seq_test` from an uninitialized `let mut last_err: Option<anyhow::Error>;` to an explicit `let mut last_err: Option<anyhow::Error> = None;` so callers of `last_err` clearly see its initial state.vm2/starcoin-transactional-test-harness/Cargo.toml (1)
26-32: Consider centralizing the Move git pin.The same
rev = "7d37ed75c76f34abd2ea71774dcf784408625887"is now repeated across several vm2 manifests. Moving thesemove-*definitions intoworkspace.dependencieswould make future bumps a single edit and reduce the chance of partial version skew.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm2/starcoin-transactional-test-harness/Cargo.toml` around lines 26 - 32, Multiple move-* dependencies in Cargo.toml repeat the same git rev; centralize by moving these crate entries (move-binary-format, move-command-line-common, move-compiler, move-core-types, move-transactional-test-runner, move-table-extension, move-resource-viewer) into workspace.dependencies with a single git = "https://github.com/starcoinorg/move" and rev = "7d37ed75c76f34abd2ea71774dcf784408625887" so all workspace members reference the same pin, then remove the duplicate per-crate git pins from the individual vm2 package manifests so future rev bumps are done in one place.test-helper/src/node.rs (1)
17-21: Keep a single node-launch path.
run_node_with_all_serviceduplicates the logger/setup sequence fromrun_node_by_config. Havingrun_node_by_configcall the new helper would keep the two entry points from drifting the next time launch behavior changes.♻️ Proposed refactor
pub fn run_node_by_config(config: Arc<NodeConfig>) -> Result<NodeHandle> { - let logger_handle = starcoin_logger::init_for_test(); - let node_handle = NodeService::launch(config, logger_handle)?; + let node_handle = run_node_with_all_service(config)?; block_on(async { node_handle.node_service().stop_pacemaker().await })?; Ok(node_handle) }Also applies to: 24-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-helper/src/node.rs` around lines 17 - 21, The two launch paths duplicate logger/setup and launch logic; refactor so run_node_by_config delegates to the single helper run_node_with_all_service: remove duplicated logger and NodeService::launch code from run_node_by_config, and instead call run_node_with_all_service(config) (or the common helper you created) to obtain the NodeHandle, then perform the existing stop_pacemaker call; ensure you preserve initialization via starcoin_logger::init_for_test() and the stop_pacemaker await logic so run_node_with_all_service (or the shared helper) becomes the single source of truth for NodeService::launch and logger setup.sync/starcoin-execute-bench/Cargo.toml (1)
18-18: Consider using workspace-managed version forplotters.The
plotters = "0.3"dependency uses an explicit version while other dependencies useworkspace = true. For consistency and easier version management, consider addingplottersto the workspace dependencies.Optional: Add to workspace Cargo.toml
In root
Cargo.tomlunder[workspace.dependencies]:plotters = "0.3"Then update this file:
-plotters = "0.3" +plotters = { workspace = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sync/starcoin-execute-bench/Cargo.toml` at line 18, The crate currently pins plotters = "0.3" in its Cargo.toml; move this to the workspace-managed dependencies to match other deps. Add plotters = "0.3" under [workspace.dependencies] in the root Cargo.toml, then remove the explicit plotters version line from this crate's Cargo.toml and replace it with plotters = { workspace = true } (or simply remove the entry if inheritance is sufficient) so the workspace controls the version; update Cargo.lock by running cargo update if needed.vm2/vm-runtime/src/parallel_executor/storage_wrapper.rs (1)
186-214: Consider using a more stable cache key than pointer address.While the current design uses
layout as *const MoveTypeLayout as usizeas a cache key, relying on pointer addresses is fragile. Although layouts are currently stored inArcvalues withinResourceReadInfoandGroupReadInfo(keeping addresses stable within a transaction) and the cache is recreated per-transaction, a more robust approach would use a content-based or type-based identifier. This prevents potential issues if the code path changes or layouts come from different sources in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm2/vm-runtime/src/parallel_executor/storage_wrapper.rs` around lines 186 - 214, The cache in LayoutIdentifierMappingCache::has_identifier_mappings currently uses the raw pointer key (layout as *const MoveTypeLayout as usize), which is fragile; change the cache key to a stable identifier derived from the layout's content or type (for example a fingerprint/hash of MoveTypeLayout, a canonical type ID, or another stable unique field) and use that instead of the pointer for last_key, entries, and comparisons; update has_identifier_mappings, the entries HashMap key type, and the last_key Cell to the new key type, and continue to call compute_layout_has_identifier_mappings(layout) when a key is missing so behavior remains the same.sync/starcoin-execute-bench/src/main.rs (3)
1080-1090: Consider checking the result ofadd_txns_multi_signedfor import failures.Unlike
transfer_to_accounts()(lines 573-587) which validates import results, this function ignores any import failures. For benchmark accuracy, at least logging failed imports would help identify issues.💡 Suggested improvement to log import failures
- txpool.add_txns_multi_signed( + let import_results = txpool.add_txns_multi_signed( signed_transactions .into_iter() .map(MultiSignedUserTransaction::VM2) .collect(), false, None, )?; + + let failed_count = import_results.iter().filter(|r| r.is_err()).count(); + if failed_count > 0 { + warn!( + "Benchmark batch import: {}/{} transactions failed to import", + failed_count, + import_results.len() + ); + } Ok(txn_hashes)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sync/starcoin-execute-bench/src/main.rs` around lines 1080 - 1090, The call to txpool.add_txns_multi_signed ignores its import results so failed imports are silently dropped; capture its return value in the function containing this call (the function that currently returns Ok(txn_hashes)), inspect the returned import statuses for failures, and at minimum log any failures (including transaction hash / error/status) via the same logging facility used elsewhere (see transfer_to_accounts comparison) so benchmark runs report import errors; update the call site of txpool.add_txns_multi_signed to handle the Result/ImportStatus, log failures, and propagate or decide failure behavior consistently with transfer_to_accounts.
1395-1422: Remove commented-out dead code or convert to proper documentation.These commented-out event handlers for
NewDagBlockandNewDagBlockFromPeeradd noise. If they're needed for future reference, consider documenting the intent in a comment without the full implementation, or track in a separate issue/TODO.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sync/starcoin-execute-bench/src/main.rs` around lines 1395 - 1422, Remove the commented-out dead implementations of EventHandler for ObserverService (the blocks referencing NewDagBlock and NewDagBlockFromPeer including handle_event bodies that call update_transaction_status, try_submit_next_batch, and check benchmark_state) — either delete these blocks entirely or replace them with a short TODO/doc comment explaining the intended behavior and referencing the related symbols (ObserverService, NewDagBlock, NewDagBlockFromPeer, handle_event, update_transaction_status, try_submit_next_batch, benchmark_state) so future work can reintroduce a proper implementation or track it in an issue.
1101-1154: Thread spawn approach to avoid deadlock is valid but has a caveat.The comment explains the rationale well. However, if the spawned thread panics,
join()returns anErrand only the message "Thread panicked" is propagated, losing the actual panic info. Consider usingstd::panic::catch_unwindor at least extracting the panic payload for better diagnostics.💡 Preserve panic details in error
- handle.join().map_err(|_| format_err!("Thread panicked"))? + handle.join().map_err(|e| { + let msg = e + .downcast_ref::<&str>() + .map(|s| s.to_string()) + .or_else(|| e.downcast_ref::<String>().cloned()) + .unwrap_or_else(|| "Unknown panic".to_string()); + format_err!("Thread panicked: {}", msg) + })?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sync/starcoin-execute-bench/src/main.rs` around lines 1101 - 1154, The current thread spawn swallows panic details because handle.join().map_err(|_| format_err!("Thread panicked")) only returns a generic message; modify the code to preserve the panic payload by either wrapping the spawned closure body with std::panic::catch_unwind or by inspecting the JoinHandle::join Err payload and converting it to a String (using downcast_ref::<&str>() or downcast_ref::<String>() to extract the message) before constructing the error; specifically update the join site (the handle.join() call) to match on Err(payload) and include the extracted payload text in the format_err! (or return it as part of the anyhow::Error), or wrap the async body in std::panic::catch_unwind and propagate the caught panic as an anyhow::Error so callers of the surrounding function get the real panic information.sync/starcoin-execute-bench/src/results.rs (1)
432-452: Hardcoded file paths reduce flexibility.The output paths
./transaction_results.txtand./benchmark_results.svgare hardcoded. Consider accepting a base directory or file prefix parameter to allow customization, especially when running multiple benchmarks.💡 Accept output path parameter
- pub fn dump_results(&self) -> anyhow::Result<()> { + pub fn dump_results(&self, output_dir: &std::path::Path) -> anyhow::Result<()> { let mut file = OpenOptions::new() .write(true) .create(true) .truncate(true) - .open("./transaction_results.txt") + .open(output_dir.join("transaction_results.txt")) .context("failed to open transaction_results.txt")?; for (transaction, results) in self.transaction_data { writeln!( file, "transaction id: {}, results: {:?}", *transaction, results ) .context("failed to write transaction results")?; } - self.export_combined_svg("./benchmark_results.svg") + self.export_combined_svg(output_dir.join("benchmark_results.svg").to_str().unwrap_or("benchmark_results.svg")) .map_err(|e| anyhow::format_err!("failed to export benchmark results svg: {}", e))?; Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sync/starcoin-execute-bench/src/results.rs` around lines 432 - 452, The dump_results method currently writes to hardcoded "./transaction_results.txt" and calls export_combined_svg("./benchmark_results.svg"); change dump_results signature to accept a Path or PathBuf parameter (e.g., base_path or output_dir) and use it to build the output file paths for transaction_results.txt and the SVG before opening/writing; update references in the loop (transaction_data) to write to base_path.join("transaction_results.txt") and call export_combined_svg with base_path.join("benchmark_results.svg"), and update any call sites to pass the desired output directory. Ensure error contexts/messages still include the file path and keep the existing result-writing logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commons/parallel-executor/src/executor.rs`:
- Around line 2220-2224: The match is moving CreateThenAddOp out of a borrowed
transaction in execute_transaction which takes txn: &Self::T; change matches on
txn.op to match &txn.op (or match txn.op as ref) and dereference the captured
variants when using their fields (e.g., use Create { value } as &Create { value
} or capture as &Create { value } then use *value or cloned value as needed),
and apply the same change for the other match sites in CreateThenAddTxn handling
(the similar matches around the CreateThenAddOp usage).
In `@rpc/client/tests/chain_get_block_txn_infos_in_seq_test.rs`:
- Around line 55-59: The test is flaky because wait_for_queryable_main_head_hash
may return a later miner-produced block hash instead of the generated block's
hash; change the polling to wait until the specific generated block (use
block.id()) is queryable rather than asking for the main head hash. Replace the
call to wait_for_queryable_main_head_hash with a new or existing
wait_for_block_queryable (or equivalent) helper that polls RPC until block.id()
becomes queryable, and keep all subsequent assertions using that exact block
hash so the user transactions (vm1_txn1, vm1_txn2, vm2_txn1, vm2_txn2) are
checked against the correct block.
In `@sync/starcoin-execute-bench/src/main.rs`:
- Around line 1210-1217: The logged batch index is off-by-one because
build_next_batch() uses fetch_add(1) (returning the previous value) so
state.batch_index.load() after building returns the next batch's index; fix by
using the actual submitted index instead of the current counter—either have
build_next_batch() return the index it allocated and use that value in the
sign_and_import_transactions_sync call and the info! message, or if you cannot
change build_next_batch(), compute the submitted index as
state.batch_index.load(Ordering::SeqCst).wrapping_sub(1) (or subtract 1 safely)
and use that for the log; update references around
sign_and_import_transactions_sync and the info! logging to use this corrected
index.
In `@vm2/abi/decoder/Cargo.toml`:
- Line 6: The workspace has inconsistent git revisions for Move crates: update
the move-related dependency revisions so they match across the workspace (or
document intentional divergence). Specifically, align the git rev for
move-binary-format (currently "7d37ed75c76f34abd2ea71774dcf784408625887" in the
vm2/abi/decoder Cargo.toml) with the rev used in the root Cargo.toml for other
move-* crates (or add a comment in both Cargo.toml files stating why the
different revs are required); ensure all move-* dependencies reference the same
commit hash or include a clear justification comment adjacent to each dependency
entry (e.g., for move-binary-format and other move-* crate entries).
In `@vm2/vm-runtime-types/Cargo.toml`:
- Around line 17-23: The Cargo.toml change pins move-core-types and
move-vm-types to rev "7d37ed..." which conflicts with other workspace crates
still using revision "babf994...", causing duplicate move-* crate instances;
update all move-* git dependencies across the workspace (including the root
Cargo.toml and any other Cargo.toml files that reference move- crates) to the
same single git rev (e.g., replace babf994... or 7d37ed... so they match) so
move-core-types and move-vm-types and all related move-* entries resolve to one
coherent revision in the lockfile and avoid duplicate types/binaries.
In `@vm2/vm-runtime/src/data_cache.rs`:
- Around line 175-204: The cache currently keys entries by the pointer address
of a MoveTypeLayout (in LayoutIdentifierMappingCache::has_identifier_mappings),
which can lead to stale results when addresses are reused; replace the
pointer-based key with a stable structural key derived from the layout content
(e.g., compute a structural/hash key or use a LayoutKey type built from
MoveTypeLayout fields) and store that in entries instead of usize pointers;
update LayoutIdentifierMappingCache (fields last_key, entries and
has_identifier_mappings) to use the new structural key type, compute the key
inside has_identifier_mappings before any lookup, and keep using
compute_layout_has_identifier_mappings(layout) to derive the boolean value when
missing.
In `@vm2/vm-runtime/src/parallel_executor/mod.rs`:
- Around line 457-491: The materializer currently treats blocks with only
delayed-field writes as safe for full parallel materialization; compute a
has_delayed flag from outputs (e.g., let has_delayed = outputs.iter().any(|(_,
output)| output.output.contains_delayed_fields());) and include it in the
needs_sequential condition (needs_sequential = has_agg_v1 || has_group_dup ||
has_delayed) so delayed-field-only blocks do not take the parallel-return path;
apply the same check where the alternate (later) parallel/serial decision is
made around materialize_parallel_candidate and materialize_in_place_change to
ensure any path that returns early respects contains_delayed_fields().
---
Nitpick comments:
In `@rpc/client/tests/chain_get_block_txn_infos_in_seq_test.rs`:
- Line 218: Initialize the variable `last_err` to improve readability: change
the declaration of `last_err` in the test
`chain_get_block_txn_infos_in_seq_test` from an uninitialized `let mut last_err:
Option<anyhow::Error>;` to an explicit `let mut last_err: Option<anyhow::Error>
= None;` so callers of `last_err` clearly see its initial state.
In `@sync/starcoin-execute-bench/Cargo.toml`:
- Line 18: The crate currently pins plotters = "0.3" in its Cargo.toml; move
this to the workspace-managed dependencies to match other deps. Add plotters =
"0.3" under [workspace.dependencies] in the root Cargo.toml, then remove the
explicit plotters version line from this crate's Cargo.toml and replace it with
plotters = { workspace = true } (or simply remove the entry if inheritance is
sufficient) so the workspace controls the version; update Cargo.lock by running
cargo update if needed.
In `@sync/starcoin-execute-bench/src/main.rs`:
- Around line 1080-1090: The call to txpool.add_txns_multi_signed ignores its
import results so failed imports are silently dropped; capture its return value
in the function containing this call (the function that currently returns
Ok(txn_hashes)), inspect the returned import statuses for failures, and at
minimum log any failures (including transaction hash / error/status) via the
same logging facility used elsewhere (see transfer_to_accounts comparison) so
benchmark runs report import errors; update the call site of
txpool.add_txns_multi_signed to handle the Result/ImportStatus, log failures,
and propagate or decide failure behavior consistently with transfer_to_accounts.
- Around line 1395-1422: Remove the commented-out dead implementations of
EventHandler for ObserverService (the blocks referencing NewDagBlock and
NewDagBlockFromPeer including handle_event bodies that call
update_transaction_status, try_submit_next_batch, and check benchmark_state) —
either delete these blocks entirely or replace them with a short TODO/doc
comment explaining the intended behavior and referencing the related symbols
(ObserverService, NewDagBlock, NewDagBlockFromPeer, handle_event,
update_transaction_status, try_submit_next_batch, benchmark_state) so future
work can reintroduce a proper implementation or track it in an issue.
- Around line 1101-1154: The current thread spawn swallows panic details because
handle.join().map_err(|_| format_err!("Thread panicked")) only returns a generic
message; modify the code to preserve the panic payload by either wrapping the
spawned closure body with std::panic::catch_unwind or by inspecting the
JoinHandle::join Err payload and converting it to a String (using
downcast_ref::<&str>() or downcast_ref::<String>() to extract the message)
before constructing the error; specifically update the join site (the
handle.join() call) to match on Err(payload) and include the extracted payload
text in the format_err! (or return it as part of the anyhow::Error), or wrap the
async body in std::panic::catch_unwind and propagate the caught panic as an
anyhow::Error so callers of the surrounding function get the real panic
information.
In `@sync/starcoin-execute-bench/src/results.rs`:
- Around line 432-452: The dump_results method currently writes to hardcoded
"./transaction_results.txt" and calls
export_combined_svg("./benchmark_results.svg"); change dump_results signature to
accept a Path or PathBuf parameter (e.g., base_path or output_dir) and use it to
build the output file paths for transaction_results.txt and the SVG before
opening/writing; update references in the loop (transaction_data) to write to
base_path.join("transaction_results.txt") and call export_combined_svg with
base_path.join("benchmark_results.svg"), and update any call sites to pass the
desired output directory. Ensure error contexts/messages still include the file
path and keep the existing result-writing logic intact.
In `@test-helper/src/node.rs`:
- Around line 17-21: The two launch paths duplicate logger/setup and launch
logic; refactor so run_node_by_config delegates to the single helper
run_node_with_all_service: remove duplicated logger and NodeService::launch code
from run_node_by_config, and instead call run_node_with_all_service(config) (or
the common helper you created) to obtain the NodeHandle, then perform the
existing stop_pacemaker call; ensure you preserve initialization via
starcoin_logger::init_for_test() and the stop_pacemaker await logic so
run_node_with_all_service (or the shared helper) becomes the single source of
truth for NodeService::launch and logger setup.
In `@vm2/starcoin-transactional-test-harness/Cargo.toml`:
- Around line 26-32: Multiple move-* dependencies in Cargo.toml repeat the same
git rev; centralize by moving these crate entries (move-binary-format,
move-command-line-common, move-compiler, move-core-types,
move-transactional-test-runner, move-table-extension, move-resource-viewer) into
workspace.dependencies with a single git = "https://github.com/starcoinorg/move"
and rev = "7d37ed75c76f34abd2ea71774dcf784408625887" so all workspace members
reference the same pin, then remove the duplicate per-crate git pins from the
individual vm2 package manifests so future rev bumps are done in one place.
In `@vm2/vm-runtime/src/parallel_executor/storage_wrapper.rs`:
- Around line 186-214: The cache in
LayoutIdentifierMappingCache::has_identifier_mappings currently uses the raw
pointer key (layout as *const MoveTypeLayout as usize), which is fragile; change
the cache key to a stable identifier derived from the layout's content or type
(for example a fingerprint/hash of MoveTypeLayout, a canonical type ID, or
another stable unique field) and use that instead of the pointer for last_key,
entries, and comparisons; update has_identifier_mappings, the entries HashMap
key type, and the last_key Cell to the new key type, and continue to call
compute_layout_has_identifier_mappings(layout) when a key is missing so behavior
remains the same.
🪄 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: ae79b826-f4ec-4ee5-954a-2a137a7dcfbd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
Cargo.tomlchain/service/src/chain_service.rscmd/miner_client/src/cpu_solver.rscmd/starcoin/src/account/sign_multisig_txn_cmd.rscommons/mvhashmap/Cargo.tomlcommons/parallel-executor/Cargo.tomlcommons/parallel-executor/src/executor.rscommons/utils/src/mpsc.rsconfig/src/txpool_config.rsminer/tests/miner_test.rsrpc/client/tests/chain_get_block_txn_infos_in_seq_test.rssimnet/src/lib.rssync/src/tasks/tests.rssync/starcoin-execute-bench/Cargo.tomlsync/starcoin-execute-bench/src/main.rssync/starcoin-execute-bench/src/results.rstest-helper/src/lib.rstest-helper/src/node.rstxpool/src/pool/listener/tests.rsvm2/abi/decoder/Cargo.tomlvm2/abi/resolver/Cargo.tomlvm2/abi/types/Cargo.tomlvm2/aggregator/Cargo.tomlvm2/compiler/Cargo.tomlvm2/framework/Cargo.tomlvm2/framework/cached-packages/Cargo.tomlvm2/framework/gas-algebra/Cargo.tomlvm2/framework/gas-meter/Cargo.tomlvm2/framework/gas-schedule/Cargo.tomlvm2/framework/move-stdlib/Cargo.tomlvm2/framework/native-interface/Cargo.tomlvm2/framework/table-natives/Cargo.tomlvm2/move-package-manager/Cargo.tomlvm2/package-builder/Cargo.tomlvm2/resource-viewer/Cargo.tomlvm2/sdk-builder/Cargo.tomlvm2/starcoin-transactional-test-harness/Cargo.tomlvm2/types/Cargo.tomlvm2/vm-runtime-types/Cargo.tomlvm2/vm-runtime/Cargo.tomlvm2/vm-runtime/src/data_cache.rsvm2/vm-runtime/src/parallel_executor/mod.rsvm2/vm-runtime/src/parallel_executor/storage_wrapper.rsvm2/vm-runtime/src/parallel_executor/vm_wrapper.rsvm2/vm-runtime/tests/delayed_field_exchange_baseline.rsvm2/vm-status-translator/Cargo.tomlvm2/vm-types/Cargo.toml
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rpc/client/tests/chain_get_block_txn_infos_in_seq_test.rs (1)
181-209: Include last observed RPC error in timeout output.Current timeout text is generic; preserving the latest failing call reason will make CI failures much easier to triage.
💡 Suggested refinement
fn wait_for_block_queryable( client: &RpcClient, block_hash: HashValue, min_number: u64, timeout: Duration, ) -> Result<()> { let deadline = Instant::now() + timeout; + let mut last_err: Option<anyhow::Error> = None; loop { if Instant::now() >= deadline { - return Err(format_err!( - "timeout waiting queryable block {:?}, min number {}", - block_hash, - min_number - )); + return Err(last_err.unwrap_or_else(|| { + format_err!( + "timeout waiting queryable block {:?}, min number {}", + block_hash, min_number + ) + })); } - if let Ok(chain_info) = client.chain_info() { + if let Ok(chain_info) = client.chain_info() { let head_number = chain_info.head.number.0; if head_number >= min_number { let seq_res = client.chain_get_block_txn_infos_in_seq(block_hash); let vm1_res = client.chain_get_block_txn_infos(block_hash); let vm2_res = client.chain_get_block_txn_infos2(block_hash); if seq_res.is_ok() && vm1_res.is_ok() && vm2_res.is_ok() { return Ok(()); } + last_err = Some(format_err!( + "rpc not ready: seq={:?}, vm1={:?}, vm2={:?}", + seq_res.err(), + vm1_res.err(), + vm2_res.err() + )); } + } else { + last_err = Some(format_err!("chain_info rpc failed while waiting block {:?}", block_hash)); } std::thread::sleep(Duration::from_millis(200)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rpc/client/tests/chain_get_block_txn_infos_in_seq_test.rs` around lines 181 - 209, In wait_for_block_queryable, capture the latest RPC error and include it in the timeout error: declare a mutable last_err (String) and update it whenever client.chain_info(), client.chain_get_block_txn_infos_in_seq(block_hash), client.chain_get_block_txn_infos(block_hash) or client.chain_get_block_txn_infos2(block_hash) returns Err (use their error's Display/Debug text), then when timing out return Err with the original timeout message plus the last_err value; reference the function wait_for_block_queryable and the RPC calls chain_info, chain_get_block_txn_infos_in_seq, chain_get_block_txn_infos, chain_get_block_txn_infos2 to locate where to set and include the last observed error.vm2/vm-runtime/src/parallel_executor/mod.rs (1)
553-560: Unreachable code block for sequential outputs.Items reach
sequential_outputsonly whenhas_delayed || has_agg_v1 || touches_duplicated_groupis true (line 520). If!has_delayed && !has_agg_v1, thentouches_duplicated_groupmust be true, which implies a group write op exists, makinghas_group_opstrue. Therefore, the condition!has_delayed && !has_agg_v1 && !has_group_opson line 553 is always false for items fromsequential_outputs.Consider simplifying by removing this unreachable branch or adding a debug assertion to document the invariant.
♻️ Suggested simplification
let txn_output = if let Some(output) = sequential_outputs.remove(&txn_idx) { let (mut vm_output, group_read_layouts) = output.into_inner(); let has_delayed = vm_output.contains_delayed_fields(); let has_agg_v1 = !vm_output.aggregator_v1_delta_set().is_empty(); let has_group_ops = vm_output .resource_write_set() .values() .any(is_group_write_op); - if !has_delayed && vm_output.aggregator_v1_delta_set().is_empty() && !has_group_ops { - vm_output.into_transaction_output().map_err(|err| { - VMStatus::error( - StatusCode::DELAYED_MATERIALIZATION_CODE_INVARIANT_ERROR, - Some(err.to_string()), - ) - })? - } else { + // Invariant: items in sequential_outputs always have delayed fields, + // agg_v1 deltas, or group ops (otherwise they'd be parallel candidates) + debug_assert!( + has_delayed || has_agg_v1 || has_group_ops, + "sequential output without requiring sequential processing" + ); + { if has_delayed || has_agg_v1 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm2/vm-runtime/src/parallel_executor/mod.rs` around lines 553 - 560, The branch that checks if !has_delayed && vm_output.aggregator_v1_delta_set().is_empty() && !has_group_ops for items coming from sequential_outputs is unreachable given the invariant that sequential_outputs only contains items when has_delayed || has_agg_v1 || touches_duplicated_group is true; therefore either remove this unreachable `if`/`else` branch and directly return vm_output.into_transaction_output() or replace it with a debug assertion documenting the invariant (e.g., assert!(has_delayed || !vm_output.aggregator_v1_delta_set().is_empty() || has_group_ops)) and then proceed to the existing else path; update the code paths involving has_delayed, has_group_ops, vm_output.aggregator_v1_delta_set(), and sequential_outputs accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rpc/client/tests/chain_get_block_txn_infos_in_seq_test.rs`:
- Around line 181-209: In wait_for_block_queryable, capture the latest RPC error
and include it in the timeout error: declare a mutable last_err (String) and
update it whenever client.chain_info(),
client.chain_get_block_txn_infos_in_seq(block_hash),
client.chain_get_block_txn_infos(block_hash) or
client.chain_get_block_txn_infos2(block_hash) returns Err (use their error's
Display/Debug text), then when timing out return Err with the original timeout
message plus the last_err value; reference the function wait_for_block_queryable
and the RPC calls chain_info, chain_get_block_txn_infos_in_seq,
chain_get_block_txn_infos, chain_get_block_txn_infos2 to locate where to set and
include the last observed error.
In `@vm2/vm-runtime/src/parallel_executor/mod.rs`:
- Around line 553-560: The branch that checks if !has_delayed &&
vm_output.aggregator_v1_delta_set().is_empty() && !has_group_ops for items
coming from sequential_outputs is unreachable given the invariant that
sequential_outputs only contains items when has_delayed || has_agg_v1 ||
touches_duplicated_group is true; therefore either remove this unreachable
`if`/`else` branch and directly return vm_output.into_transaction_output() or
replace it with a debug assertion documenting the invariant (e.g.,
assert!(has_delayed || !vm_output.aggregator_v1_delta_set().is_empty() ||
has_group_ops)) and then proceed to the existing else path; update the code
paths involving has_delayed, has_group_ops, vm_output.aggregator_v1_delta_set(),
and sequential_outputs accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4377f498-1a09-4071-9b29-f5b6404c2fd3
📒 Files selected for processing (4)
commons/parallel-executor/src/executor.rsrpc/client/tests/chain_get_block_txn_infos_in_seq_test.rssync/starcoin-execute-bench/src/main.rsvm2/vm-runtime/src/parallel_executor/mod.rs
✅ Files skipped from review due to trivial changes (1)
- commons/parallel-executor/src/executor.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- sync/starcoin-execute-bench/src/main.rs
jackzhhuang
force-pushed
the
spike-layout-cache-benchmark
branch
from
March 31, 2026 08:09
0165dc2 to
60eecc3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
sync/starcoin-execute-bench/src/main.rs (1)
1395-1422: Commented-out code for future DAG block support.The commented handlers for
NewDagBlockandNewDagBlockFromPeerappear to be placeholders for future DAG block support. Consider either removing if not needed soon, or adding a TODO comment explaining when this will be enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sync/starcoin-execute-bench/src/main.rs` around lines 1395 - 1422, The commented-out EventHandler implementations for ObserverService (handling NewDagBlock and NewDagBlockFromPeer) should either be removed or annotated with a clear TODO explaining when they'll be re-enabled; update the code around the commented blocks for the methods that call update_transaction_status, try_submit_next_batch, and reference benchmark_state to include a single-line TODO (or a JIRA/issue number and expected milestone) stating the condition under which DAG block support will be enabled, or delete the commented handlers entirely to avoid dead code clutter—ensure the commit message or TODO mentions EventHandler, NewDagBlock, NewDagBlockFromPeer, ObserverService, update_transaction_status, and try_submit_next_batch so reviewers can trace intent.sync/starcoin-execute-bench/src/results.rs (1)
432-452: Consider parameterizing output paths.The output paths
./transaction_results.txtand./benchmark_results.svgare hardcoded. For a benchmark tool this is acceptable, but consider making them configurable via CLI arguments if the tool needs to be run from different directories or produce multiple result sets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sync/starcoin-execute-bench/src/results.rs` around lines 432 - 452, The dump_results method currently writes to hardcoded "./transaction_results.txt" and calls export_combined_svg with "./benchmark_results.svg"; make these paths configurable by adding parameters or fields (e.g., add arguments to dump_results like dump_results(&self, tx_path: &Path, svg_path: &Path) or store result_path fields on the struct and use them inside dump_results and when calling export_combined_svg) so callers or the CLI can supply different output locations; update any call sites of dump_results and the export_combined_svg invocation to pass through the chosen paths and keep the existing error handling.vm2/vm-runtime/src/parallel_executor/storage_wrapper.rs (1)
167-216: Extract sharedLayoutIdentifierMappingCacheto avoid code duplication.The
compute_layout_has_identifier_mappingsfunction andLayoutIdentifierMappingCachestruct are independently defined here and indata_cache.rs(lines 156-205) with identical implementations. This violates DRY and creates a maintenance risk—if one implementation changes, the other may diverge silently.Consider extracting these into a shared module (e.g.,
layout_utils.rsor re-export fromdata_cache.rs) that both files can import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm2/vm-runtime/src/parallel_executor/storage_wrapper.rs` around lines 167 - 216, The same logic for compute_layout_has_identifier_mappings and the LayoutIdentifierMappingCache (including its method has_identifier_mappings) is duplicated here and in data_cache.rs; extract these into a single shared module (e.g., layout_utils.rs) and have both files import or re-export the shared symbols to avoid divergence—move the compute_layout_has_identifier_mappings function and the LayoutIdentifierMappingCache struct/impl into that module, update both callers to use the shared names, and remove the duplicate definitions from this file and data_cache.rs.vm2/vm-runtime/src/data_cache.rs (1)
1241-1279: Benchmark test is useful but only exercises the cache hit path.The test correctly demonstrates the performance benefit of caching (reported 6.66x speedup). However, since it reuses a single
layoutinstance throughout all iterations, it only measures the "last-hit" fast path and doesn't exercise:
- HashMap lookup path (multiple distinct layouts)
- Cache miss scenarios with pointer-identity edge cases
Consider adding a variant that uses multiple distinct layout instances to validate HashMap lookup performance.
💡 Suggested enhancement for broader coverage
+ #[test] + #[ignore = "benchmark-style regression probe; run manually with --ignored --nocapture"] + fn test_bench_layout_identifier_mapping_cache_multiple_layouts() { + // Build multiple distinct layouts to exercise HashMap path + let layouts: Vec<_> = (0..10) + .map(|_| build_complex_aggregator_like_layout()) + .collect(); + let iterations = 200_000usize; + + let cache = LayoutIdentifierMappingCache::default(); + let start = Instant::now(); + let mut acc = 0usize; + for i in 0..iterations { + let layout = &layouts[i % layouts.len()]; + let hit = cache.has_identifier_mappings(layout); + acc ^= usize::from(black_box(hit)); + } + let elapsed = start.elapsed(); + println!( + "[layout-check-bench-multi] iters={} elapsed={:?} ns/iter={:.2}", + iterations, + elapsed, + elapsed.as_nanos() as f64 / iterations as f64 + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm2/vm-runtime/src/data_cache.rs` around lines 1241 - 1279, The current benchmark test test_bench_layout_identifier_mapping_cache_complex_aggregator only measures repeated hits on one layout; add a second benchmark variant that constructs many distinct layouts (e.g. by calling build_complex_aggregator_like_layout in a loop to produce N different Layout instances) and then measures cache.has_identifier_mappings across that set to exercise HashMap lookups and cache-miss behavior; for correctness, for each distinct layout call compute_layout_has_identifier_mappings and compare/accumulate results against cache.has_identifier_mappings (using the same xor accumulator pattern) and assert equality (use LayoutIdentifierMappingCache, compute_layout_has_identifier_mappings, build_complex_aggregator_like_layout, and the existing accumulator/assert patterns) so the test covers both miss and hit paths without changing existing single-layout benchmark.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sync/starcoin-execute-bench/src/main.rs`:
- Around line 1395-1422: The commented-out EventHandler implementations for
ObserverService (handling NewDagBlock and NewDagBlockFromPeer) should either be
removed or annotated with a clear TODO explaining when they'll be re-enabled;
update the code around the commented blocks for the methods that call
update_transaction_status, try_submit_next_batch, and reference benchmark_state
to include a single-line TODO (or a JIRA/issue number and expected milestone)
stating the condition under which DAG block support will be enabled, or delete
the commented handlers entirely to avoid dead code clutter—ensure the commit
message or TODO mentions EventHandler, NewDagBlock, NewDagBlockFromPeer,
ObserverService, update_transaction_status, and try_submit_next_batch so
reviewers can trace intent.
In `@sync/starcoin-execute-bench/src/results.rs`:
- Around line 432-452: The dump_results method currently writes to hardcoded
"./transaction_results.txt" and calls export_combined_svg with
"./benchmark_results.svg"; make these paths configurable by adding parameters or
fields (e.g., add arguments to dump_results like dump_results(&self, tx_path:
&Path, svg_path: &Path) or store result_path fields on the struct and use them
inside dump_results and when calling export_combined_svg) so callers or the CLI
can supply different output locations; update any call sites of dump_results and
the export_combined_svg invocation to pass through the chosen paths and keep the
existing error handling.
In `@vm2/vm-runtime/src/data_cache.rs`:
- Around line 1241-1279: The current benchmark test
test_bench_layout_identifier_mapping_cache_complex_aggregator only measures
repeated hits on one layout; add a second benchmark variant that constructs many
distinct layouts (e.g. by calling build_complex_aggregator_like_layout in a loop
to produce N different Layout instances) and then measures
cache.has_identifier_mappings across that set to exercise HashMap lookups and
cache-miss behavior; for correctness, for each distinct layout call
compute_layout_has_identifier_mappings and compare/accumulate results against
cache.has_identifier_mappings (using the same xor accumulator pattern) and
assert equality (use LayoutIdentifierMappingCache,
compute_layout_has_identifier_mappings, build_complex_aggregator_like_layout,
and the existing accumulator/assert patterns) so the test covers both miss and
hit paths without changing existing single-layout benchmark.
In `@vm2/vm-runtime/src/parallel_executor/storage_wrapper.rs`:
- Around line 167-216: The same logic for compute_layout_has_identifier_mappings
and the LayoutIdentifierMappingCache (including its method
has_identifier_mappings) is duplicated here and in data_cache.rs; extract these
into a single shared module (e.g., layout_utils.rs) and have both files import
or re-export the shared symbols to avoid divergence—move the
compute_layout_has_identifier_mappings function and the
LayoutIdentifierMappingCache struct/impl into that module, update both callers
to use the shared names, and remove the duplicate definitions from this file and
data_cache.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad734702-34d7-4047-9825-df7382d41428
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
Cargo.tomlchain/service/src/chain_service.rscmd/miner_client/src/cpu_solver.rscmd/starcoin/src/account/sign_multisig_txn_cmd.rscommons/mvhashmap/Cargo.tomlcommons/parallel-executor/Cargo.tomlcommons/parallel-executor/src/executor.rscommons/utils/src/mpsc.rsconfig/src/txpool_config.rsminer/tests/miner_test.rsrpc/client/tests/chain_get_block_txn_infos_in_seq_test.rssimnet/src/lib.rssync/src/tasks/tests.rssync/starcoin-execute-bench/Cargo.tomlsync/starcoin-execute-bench/src/main.rssync/starcoin-execute-bench/src/results.rstest-helper/src/lib.rstest-helper/src/node.rstxpool/src/pool/listener/tests.rsvm2/abi/decoder/Cargo.tomlvm2/abi/resolver/Cargo.tomlvm2/abi/types/Cargo.tomlvm2/aggregator/Cargo.tomlvm2/compiler/Cargo.tomlvm2/framework/Cargo.tomlvm2/framework/cached-packages/Cargo.tomlvm2/framework/gas-algebra/Cargo.tomlvm2/framework/gas-meter/Cargo.tomlvm2/framework/gas-schedule/Cargo.tomlvm2/framework/move-stdlib/Cargo.tomlvm2/framework/native-interface/Cargo.tomlvm2/framework/table-natives/Cargo.tomlvm2/move-package-manager/Cargo.tomlvm2/package-builder/Cargo.tomlvm2/resource-viewer/Cargo.tomlvm2/sdk-builder/Cargo.tomlvm2/starcoin-transactional-test-harness/Cargo.tomlvm2/types/Cargo.tomlvm2/vm-runtime-types/Cargo.tomlvm2/vm-runtime/Cargo.tomlvm2/vm-runtime/src/data_cache.rsvm2/vm-runtime/src/parallel_executor/mod.rsvm2/vm-runtime/src/parallel_executor/storage_wrapper.rsvm2/vm-runtime/src/parallel_executor/vm_wrapper.rsvm2/vm-runtime/tests/delayed_field_exchange_baseline.rsvm2/vm-status-translator/Cargo.tomlvm2/vm-types/Cargo.toml
✅ Files skipped from review due to trivial changes (27)
- commons/parallel-executor/Cargo.toml
- vm2/abi/resolver/Cargo.toml
- vm2/vm-status-translator/Cargo.toml
- vm2/abi/decoder/Cargo.toml
- vm2/types/Cargo.toml
- vm2/aggregator/Cargo.toml
- vm2/compiler/Cargo.toml
- vm2/framework/table-natives/Cargo.toml
- vm2/framework/native-interface/Cargo.toml
- vm2/framework/move-stdlib/Cargo.toml
- vm2/framework/gas-schedule/Cargo.toml
- vm2/sdk-builder/Cargo.toml
- simnet/src/lib.rs
- vm2/package-builder/Cargo.toml
- vm2/abi/types/Cargo.toml
- chain/service/src/chain_service.rs
- vm2/resource-viewer/Cargo.toml
- sync/starcoin-execute-bench/Cargo.toml
- vm2/framework/cached-packages/Cargo.toml
- commons/mvhashmap/Cargo.toml
- vm2/framework/gas-meter/Cargo.toml
- cmd/starcoin/src/account/sign_multisig_txn_cmd.rs
- vm2/vm-runtime/Cargo.toml
- vm2/move-package-manager/Cargo.toml
- vm2/starcoin-transactional-test-harness/Cargo.toml
- vm2/framework/Cargo.toml
- vm2/vm-runtime-types/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (12)
- test-helper/src/lib.rs
- cmd/miner_client/src/cpu_solver.rs
- txpool/src/pool/listener/tests.rs
- sync/src/tasks/tests.rs
- vm2/vm-runtime/tests/delayed_field_exchange_baseline.rs
- commons/utils/src/mpsc.rs
- Cargo.toml
- vm2/framework/gas-algebra/Cargo.toml
- config/src/txpool_config.rs
- commons/parallel-executor/src/executor.rs
- rpc/client/tests/chain_get_block_txn_infos_in_seq_test.rs
- vm2/vm-runtime/src/parallel_executor/mod.rs
jackzhhuang
force-pushed
the
spike-layout-cache-benchmark
branch
from
April 8, 2026 02:44
60eecc3 to
0fb9e7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
rpc/client/tests/chain_get_block_txn_infos_in_seq_test.rs (1)
221-221: Consider initializinglast_errfor clarity.While Rust's flow analysis correctly determines that
last_erris always assigned before being read (both theOkand error branches assign it before the deadline check), explicitly initializing it makes the intent clearer and follows more conventional patterns.♻️ Suggested change
- let mut last_err: Option<anyhow::Error>; + let mut last_err: Option<anyhow::Error> = None;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rpc/client/tests/chain_get_block_txn_infos_in_seq_test.rs` at line 221, Initialize the variable last_err when declaring it in the test to make intent obvious and follow conventional patterns: change the declaration of last_err in chain_get_block_txn_infos_in_seq_test (the test using let mut last_err: Option<anyhow::Error>;) to initialize it (e.g., let mut last_err: Option<anyhow::Error> = None;) so both branches still assign before use but the variable starts with a clear value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sync/starcoin-execute-bench/src/main.rs`:
- Around line 1080-1087: The batch submission ignores per-transaction results
from txpool.add_txns_multi_signed, so update both places where
add_txns_multi_signed is called (the submissions inside
sign_and_import_transactions_sync and the other batch path) to capture its
return value, iterate the returned per-tx import_results and detect any rejected
transactions, and propagate an error (or return a failure) when any
import_result indicates rejection—mirroring the funding path's import_results
check logic (use the same rejection-detection and logging behavior as in the
funding import_results handling).
- Around line 780-819: The batch cursor (batch_index) is advanced inside
build_next_batch() before the batch is actually built/imported, which causes
failed builds or later signing/import errors to be treated as "all batches sent"
and drop transactions while total_txn_count is still incremented; fix by making
build_next_batch() not advance the cursor early (remove fetch_add from the
start) and instead return a Result that includes the batch_index or by having
try_submit_next_batch() fetch/advance batch_index only after a successful
build/import; ensure total_txn_count is only incremented after a confirmed
successful import/signing and propagate build/import errors back to the caller
so the benchmark can abort or retry rather than silently dropping batches
(update references to build_next_batch, try_submit_next_batch, batch_index, and
total_txn_count accordingly).
- Around line 519-522: The current precheck computes needed_balance =
account_count * (initial_balance + per_tx_fee) + initial_gas_fee which
incorrectly charges per_tx_fee for every receiver; instead compute the gas cost
for the funding path (e.g., number_of_batches = ceil(account_count /
batch_size)) and set total_gas_fee = number_of_batches * max_gas * gas_price,
then compute needed_balance = account_count * initial_balance + total_gas_fee +
initial_gas_fee (or otherwise account for initial_gas_fee separately if it is
not part of transferred funds). Update the check that compares
association_balance to needed_balance and use the symbols per_tx_fee,
needed_balance, account_count, initial_balance, initial_gas_fee and the
batch-size/batching logic (or existing batch function) to derive
number_of_batches so the precheck matches the actual funding transactions.
- Around line 1235-1261: The handler currently records every entry in
block.body.transactions2 and increments executed counts without verifying the
transaction was one of our submitted benchmark hashes or that it succeeded;
modify the logic in the loop that pushes TransactionExecutionResult::Executed
(and in update_transaction_status()) to first check the transaction.id() against
the set of persisted submitted hashes (the submission store you maintain) and
only for those queries fetch the on-chain receipt/transaction info to confirm
success before pushing Executed and calling state.add_executed_count; apply the
same submitted-hash filter and success-check in record_mined_event() so
mined-based TPS and completed-marking (state.mark_completed /
state.all_batches_sent / state.total_txn_count) only account for legitimately
submitted and successful benchmark transactions.
In `@sync/starcoin-execute-bench/src/results.rs`:
- Around line 549-555: The helper functions (e.g., draw_latency_chart and the
other chart-drawing helper) use DrawingArea<SVGBackend, plotters::coord::Shift>
but must include the SVGBackend lifetime; update their signatures to use
DrawingArea<SVGBackend<'_>, plotters::coord::Shift> (or an explicit lifetime
like 'a) so the SVGBackend lifetime parameter is supplied and the code compiles.
---
Nitpick comments:
In `@rpc/client/tests/chain_get_block_txn_infos_in_seq_test.rs`:
- Line 221: Initialize the variable last_err when declaring it in the test to
make intent obvious and follow conventional patterns: change the declaration of
last_err in chain_get_block_txn_infos_in_seq_test (the test using let mut
last_err: Option<anyhow::Error>;) to initialize it (e.g., let mut last_err:
Option<anyhow::Error> = None;) so both branches still assign before use but the
variable starts with a clear value.
🪄 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: 707d714d-4a57-437e-bfdf-45208453e08c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
Cargo.tomlchain/service/src/chain_service.rscmd/miner_client/src/cpu_solver.rscmd/starcoin/src/account/sign_multisig_txn_cmd.rscommons/mvhashmap/Cargo.tomlcommons/parallel-executor/Cargo.tomlcommons/parallel-executor/src/executor.rscommons/utils/src/mpsc.rsconfig/src/txpool_config.rsminer/tests/miner_test.rsrpc/client/tests/chain_get_block_txn_infos_in_seq_test.rssimnet/src/lib.rssync/src/tasks/tests.rssync/starcoin-execute-bench/Cargo.tomlsync/starcoin-execute-bench/src/main.rssync/starcoin-execute-bench/src/results.rstest-helper/src/lib.rstest-helper/src/node.rstxpool/src/pool/listener/tests.rsvm2/abi/decoder/Cargo.tomlvm2/abi/resolver/Cargo.tomlvm2/abi/types/Cargo.tomlvm2/aggregator/Cargo.tomlvm2/compiler/Cargo.tomlvm2/framework/Cargo.tomlvm2/framework/cached-packages/Cargo.tomlvm2/framework/gas-algebra/Cargo.tomlvm2/framework/gas-meter/Cargo.tomlvm2/framework/gas-schedule/Cargo.tomlvm2/framework/move-stdlib/Cargo.tomlvm2/framework/native-interface/Cargo.tomlvm2/framework/table-natives/Cargo.tomlvm2/move-package-manager/Cargo.tomlvm2/package-builder/Cargo.tomlvm2/resource-viewer/Cargo.tomlvm2/sdk-builder/Cargo.tomlvm2/starcoin-transactional-test-harness/Cargo.tomlvm2/types/Cargo.tomlvm2/vm-runtime-types/Cargo.tomlvm2/vm-runtime/Cargo.tomlvm2/vm-runtime/src/data_cache.rsvm2/vm-runtime/src/parallel_executor/mod.rsvm2/vm-runtime/src/parallel_executor/storage_wrapper.rsvm2/vm-runtime/src/parallel_executor/vm_wrapper.rsvm2/vm-runtime/tests/delayed_field_exchange_baseline.rsvm2/vm-status-translator/Cargo.tomlvm2/vm-types/Cargo.toml
✅ Files skipped from review due to trivial changes (30)
- commons/parallel-executor/Cargo.toml
- commons/mvhashmap/Cargo.toml
- vm2/aggregator/Cargo.toml
- test-helper/src/lib.rs
- vm2/vm-status-translator/Cargo.toml
- vm2/compiler/Cargo.toml
- vm2/abi/resolver/Cargo.toml
- vm2/package-builder/Cargo.toml
- vm2/framework/cached-packages/Cargo.toml
- sync/starcoin-execute-bench/Cargo.toml
- vm2/framework/table-natives/Cargo.toml
- vm2/framework/gas-schedule/Cargo.toml
- chain/service/src/chain_service.rs
- vm2/move-package-manager/Cargo.toml
- Cargo.toml
- vm2/framework/gas-meter/Cargo.toml
- vm2/framework/native-interface/Cargo.toml
- vm2/abi/types/Cargo.toml
- vm2/sdk-builder/Cargo.toml
- vm2/resource-viewer/Cargo.toml
- vm2/types/Cargo.toml
- vm2/vm-runtime-types/Cargo.toml
- vm2/framework/gas-algebra/Cargo.toml
- vm2/starcoin-transactional-test-harness/Cargo.toml
- vm2/vm-runtime/Cargo.toml
- vm2/framework/Cargo.toml
- vm2/vm-runtime/src/parallel_executor/vm_wrapper.rs
- vm2/vm-types/Cargo.toml
- vm2/framework/move-stdlib/Cargo.toml
- vm2/vm-runtime/src/parallel_executor/mod.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- commons/utils/src/mpsc.rs
- simnet/src/lib.rs
- vm2/abi/decoder/Cargo.toml
- miner/tests/miner_test.rs
- sync/src/tasks/tests.rs
- txpool/src/pool/listener/tests.rs
- test-helper/src/node.rs
- vm2/vm-runtime/tests/delayed_field_exchange_baseline.rs
- cmd/starcoin/src/account/sign_multisig_txn_cmd.rs
- config/src/txpool_config.rs
- vm2/vm-runtime/src/data_cache.rs
- vm2/vm-runtime/src/parallel_executor/storage_wrapper.rs
jackzhhuang
force-pushed
the
spike-layout-cache-benchmark
branch
from
April 9, 2026 06:25
0fb9e7a to
f5df073
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
vm2/vm-runtime/src/data_cache.rs (1)
175-204:⚠️ Potential issue | 🟠 MajorDon't memoize layout semantics by borrowed address.
Line 185 still uses
layout as *const MoveTypeLayout as usizeas the cache key. That key is tied to an allocation site, not to the layout content, so address reuse can return a stale boolean and change whether delayed identifiers are scanned/exchanged. The ignored bench also only reuses onelayout, so it won't catch this failure mode. Please use a stable structural key here instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm2/vm-runtime/src/data_cache.rs` around lines 175 - 204, The cache currently keys entries in LayoutIdentifierMappingCache by the layout's pointer (used in has_identifier_mappings), which leads to stale lookups when addresses are reused; replace the pointer-based key with a stable structural fingerprint of the layout (e.g., compute a deterministic hash/ID from the MoveTypeLayout's structure) and use that fingerprint for entries, last_key, and lookups; compute the fingerprint before checking entries (call it something like compute_layout_fingerprint or reuse an existing structural-hash helper), store it in the entries HashMap and last_key, and keep calling compute_layout_has_identifier_mappings(layout) only when the structural key is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@vm2/vm-runtime/src/data_cache.rs`:
- Around line 175-204: The cache currently keys entries in
LayoutIdentifierMappingCache by the layout's pointer (used in
has_identifier_mappings), which leads to stale lookups when addresses are
reused; replace the pointer-based key with a stable structural fingerprint of
the layout (e.g., compute a deterministic hash/ID from the MoveTypeLayout's
structure) and use that fingerprint for entries, last_key, and lookups; compute
the fingerprint before checking entries (call it something like
compute_layout_fingerprint or reuse an existing structural-hash helper), store
it in the entries HashMap and last_key, and keep calling
compute_layout_has_identifier_mappings(layout) only when the structural key is
missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8133d8b7-6e88-4765-aa61-1ac1087b0091
📒 Files selected for processing (6)
commons/parallel-executor/src/executor.rsrpc/client/tests/chain_get_block_txn_infos_in_seq_test.rssync/starcoin-execute-bench/src/main.rsvm2/vm-runtime/src/data_cache.rsvm2/vm-runtime/src/parallel_executor/mod.rsvm2/vm-runtime/src/parallel_executor/storage_wrapper.rs
✅ Files skipped from review due to trivial changes (1)
- commons/parallel-executor/src/executor.rs
jackzhhuang
force-pushed
the
spike-layout-cache-benchmark
branch
from
April 17, 2026 09:09
9b9273f to
714999e
Compare
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.
Summary
layout_has_identifier_mappingshot-path checks in VM2 runtimelast-hitfast path +HashMapfallback)Scope (Prep PR only)
This PR is intentionally a prep/performance PR.
It does not include Aptos move-logic migration changes.
A separate follow-up PR will handle Aptos logic migration and compatibility validation.
Files
vm2/vm-runtime/src/data_cache.rsvm2/vm-runtime/src/parallel_executor/storage_wrapper.rsValidation
cargo test -p starcoin-vm-runtime test_bench_layout_identifier_mapping_cache_complex_aggregator -- --ignored --nocapturespeedup=6.66xcargo test -p starcoin-vm-runtime --test delayed_field_exchange_baselineNotes
Summary by CodeRabbit