Optimize the parallelizing the output of the transaction execution - #4829
Open
jackzhhuang wants to merge 3 commits into
Open
Optimize the parallelizing the output of the transaction execution#4829jackzhhuang wants to merge 3 commits into
jackzhhuang wants to merge 3 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:
📝 WalkthroughWalkthroughRefactors parallel materialization to support mixed-mode execution: classifies transactions into parallel-safe and sequential sets, runs safe candidates in parallel with per-candidate group caches, then materializes sequential ones in original order while preserving shared state and validating invariants; also updates a chain test and CI/dependency files. Changes
Sequence Diagram(s)sequenceDiagram
participant Driver as Executor (materialize_parallel_outputs)
participant Parallel as Parallel Worker (Rayon)
participant Seq as Sequential Materializer
participant SVC as StateViewCache
participant GC as GroupCache
rect rgba(200,220,255,0.5)
Driver->>Driver: classify txns -> parallel_candidates + sequential_outputs
end
rect rgba(200,255,200,0.5)
Driver->>Parallel: spawn materialize_parallel_candidate per candidate
Parallel->>GC: use per-candidate group cache
Parallel->>SVC: read-state via shared StateViewCache (read-only)
Parallel-->>Driver: return (txn_idx, TransactionOutput)
end
rect rgba(255,230,200,0.5)
Driver->>Seq: iterate ordered txns, apply sequential materialization for sequential_outputs
Seq->>GC: update shared group_cache (patches)
Seq->>SVC: update/read shared StateViewCache as needed
Seq-->>Driver: write final outputs in order
end
Driver->>Driver: invariant check -> error if unconsumed outputs remain
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 887e65a9c7
ℹ️ 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".
![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
🧹 Nitpick comments (1)
vm2/vm-runtime/src/parallel_executor/mod.rs (1)
1221-1250: The new tests still miss thegroup_dupand delayed-field branches.
build_sparse_conflict_case()always creates a freshgroup_keyon Line 1221, and the test/bench calls here keep usingVersionedDelayedFields::empty()(for example Line 1372 and Line 1462). So the new mixed path is only validated through theagg_v1split, while the duplicated-group and delayed-field materialization paths can regress unnoticed.Also applies to: 1363-1479
🤖 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 1221 - 1250, build_sparse_conflict_case currently always generates a fresh group_key and uses VersionedDelayedFields::empty(), so tests never exercise the group_dup and delayed-field materialization branches; modify build_sparse_conflict_case to (1) insert at least one duplicate group_key for a different txn index to trigger the group_dup logic (reuse the existing StateKey::raw("group-{...}") creation but intentionally reuse the same key for a second transaction) and (2) construct and attach non-empty VersionedDelayedFields for one or more transactions (instead of VersionedDelayedFields::empty()) so the delayed-field materialization path runs; update any test callers that rely on this helper so they validate the dup-group and delayed-field behavior as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@vm2/vm-runtime/src/parallel_executor/mod.rs`:
- Around line 1221-1250: build_sparse_conflict_case currently always generates a
fresh group_key and uses VersionedDelayedFields::empty(), so tests never
exercise the group_dup and delayed-field materialization branches; modify
build_sparse_conflict_case to (1) insert at least one duplicate group_key for a
different txn index to trigger the group_dup logic (reuse the existing
StateKey::raw("group-{...}") creation but intentionally reuse the same key for a
second transaction) and (2) construct and attach non-empty
VersionedDelayedFields for one or more transactions (instead of
VersionedDelayedFields::empty()) so the delayed-field materialization path runs;
update any test callers that rely on this helper so they validate the dup-group
and delayed-field behavior as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a20e19d-2889-4d9e-86d4-439d1b1e672a
📒 Files selected for processing (1)
vm2/vm-runtime/src/parallel_executor/mod.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
vm2/vm-runtime/src/parallel_executor/mod.rs (2)
530-541: Minor: simplify redundant condition.Line 530 already computes
has_agg_v1, but line 535 re-checksvm_output.aggregator_v1_delta_set().is_empty(). Consider using the existing variable for consistency.🔧 Suggested simplification
if !has_delayed && vm_output.aggregator_v1_delta_set().is_empty() && !has_group_ops { + if !has_delayed && !has_agg_v1 && !has_group_ops {🤖 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 530 - 541, The condition redundantly recomputes vm_output.aggregator_v1_delta_set().is_empty() instead of using the already-computed has_agg_v1; update the if-condition to use has_agg_v1 (i.e., check !has_delayed && !has_agg_v1 && !has_group_ops) so the logic is consistent and avoid re-accessing vm_output, keeping the subsequent call to vm_output.into_transaction_output() and the VMStatus::error(StatusCode::DELAYED_MATERIALIZATION_CODE_INVARIANT_ERROR, ...) handling unchanged.
1366-1388: Good comparison test; consider adding a no-conflict case.This test validates the mixed path against the legacy sequential path for the conflict scenario. Consider adding a complementary test where
conflict_aandconflict_bdon't triggerneeds_sequential(e.g., no agg_v1 deltas) to exercise the pure parallel path at lines 464–480.🤖 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 1366 - 1388, Add a complementary test that exercises the pure-parallel path (no sequential fallback) by creating inputs where conflict_a and conflict_b do not trigger needs_sequential (no agg_v1 deltas) and asserting materialize_parallel_outputs equals materialize_parallel_outputs_legacy_all_seq; implement a new test function (e.g., mixed_materialization_matches_legacy_no_conflict_outputs) that uses a builder producing a no-conflict scenario (either add a build_no_conflict_case helper or call build_sparse_conflict_case with parameters that produce no agg_v1 deltas), call materialize_parallel_outputs and materialize_parallel_outputs_legacy_all_seq with VersionedDelayedFields::empty() and Arc::new(DelayedFieldCache::default()) and the returned state_views, unwrap the results, and assert_eq!(mixed, legacy).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@vm2/vm-runtime/src/parallel_executor/mod.rs`:
- Around line 530-541: The condition redundantly recomputes
vm_output.aggregator_v1_delta_set().is_empty() instead of using the
already-computed has_agg_v1; update the if-condition to use has_agg_v1 (i.e.,
check !has_delayed && !has_agg_v1 && !has_group_ops) so the logic is consistent
and avoid re-accessing vm_output, keeping the subsequent call to
vm_output.into_transaction_output() and the
VMStatus::error(StatusCode::DELAYED_MATERIALIZATION_CODE_INVARIANT_ERROR, ...)
handling unchanged.
- Around line 1366-1388: Add a complementary test that exercises the
pure-parallel path (no sequential fallback) by creating inputs where conflict_a
and conflict_b do not trigger needs_sequential (no agg_v1 deltas) and asserting
materialize_parallel_outputs equals materialize_parallel_outputs_legacy_all_seq;
implement a new test function (e.g.,
mixed_materialization_matches_legacy_no_conflict_outputs) that uses a builder
producing a no-conflict scenario (either add a build_no_conflict_case helper or
call build_sparse_conflict_case with parameters that produce no agg_v1 deltas),
call materialize_parallel_outputs and
materialize_parallel_outputs_legacy_all_seq with VersionedDelayedFields::empty()
and Arc::new(DelayedFieldCache::default()) and the returned state_views, unwrap
the results, and assert_eq!(mixed, legacy).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e93f98f-39c2-471a-ad06-02ff27c73dcf
📒 Files selected for processing (1)
vm2/vm-runtime/src/parallel_executor/mod.rs
jackzhhuang
force-pushed
the
parallize-vm-execute-if-needed
branch
from
March 25, 2026 04:06
43da76b to
cf3eac7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
vm2/vm-runtime/src/parallel_executor/mod.rs (1)
1458-1554: Consider additional edge case coverage.The benchmark test provides good validation with sparse conflicts. Consider adding unit tests for edge cases:
- All transactions require sequential processing (conflicts at indices 0 and 1)
- No transactions require sequential processing (no conflicts)
- Single transaction requiring sequential (only one conflict index)
This would increase confidence in boundary conditions.
🤖 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 1458 - 1554, The benchmark covers a sparse-conflict scenario but lacks boundary tests; add small unit tests calling build_sparse_conflict_case and comparing materialize_parallel_outputs_legacy_all_seq and materialize_parallel_outputs outputs for three edge cases: (1) all transactions sequential (use conflict indices like 0 and 1 so every txn conflicts), (2) no sequential conflicts (empty or distinct non-conflicting indices), and (3) single-transaction sequential case (only one conflict index). Create three new #[test] functions (non-ignored) that construct inputs via build_sparse_conflict_case with tiny TXN_COUNT, invoke both materialize_parallel_outputs_legacy_all_seq and materialize_parallel_outputs with the same VersionedDelayedFields::empty() and DelayedFieldCache, unwrap the results and assert_eq! on outputs to validate behavioral parity for edge boundaries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@vm2/vm-runtime/src/parallel_executor/mod.rs`:
- Around line 1458-1554: The benchmark covers a sparse-conflict scenario but
lacks boundary tests; add small unit tests calling build_sparse_conflict_case
and comparing materialize_parallel_outputs_legacy_all_seq and
materialize_parallel_outputs outputs for three edge cases: (1) all transactions
sequential (use conflict indices like 0 and 1 so every txn conflicts), (2) no
sequential conflicts (empty or distinct non-conflicting indices), and (3)
single-transaction sequential case (only one conflict index). Create three new
#[test] functions (non-ignored) that construct inputs via
build_sparse_conflict_case with tiny TXN_COUNT, invoke both
materialize_parallel_outputs_legacy_all_seq and materialize_parallel_outputs
with the same VersionedDelayedFields::empty() and DelayedFieldCache, unwrap the
results and assert_eq! on outputs to validate behavioral parity for edge
boundaries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee61d76a-0940-40c1-9354-2f528c21f58f
📒 Files selected for processing (1)
vm2/vm-runtime/src/parallel_executor/mod.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/dependabot_auto_merge.yml:
- Line 18: Replace the mutable GitHub Action reference
"actions/github-script@v7" with an immutable pinned commit SHA: find the step
that uses actions/github-script (the line containing uses:
actions/github-script@v7), look up the repository's commit SHA for the desired
v7 release tag (via the actions/github-script repo on GitHub) and update the
uses value to uses: actions/github-script@<full-commit-sha>; ensure you use the
full 40-character commit SHA so the workflow is pinned to an immutable revision.
🪄 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: 379d51ef-5bb3-48dc-8f31-06a6a2696b03
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/dependabot.yml.github/workflows/dependabot_auto_merge.ymlCargo.tomlsimnet/Cargo.toml
✅ Files skipped from review due to trivial changes (3)
- simnet/Cargo.toml
- .github/dependabot.yml
- Cargo.toml
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Enable auto-merge for Dependabot PRs | ||
| uses: actions/github-script@v7 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find workflow action refs that are not pinned to a full 40-char SHA.
fd -HI -e yml -e yaml '' .github/workflows \
-x rg -nP '^\s*uses:\s*[^@\s]+@(?!(?:[0-9a-fA-F]{40})\b)[^ \t#]+' {}
# Optional: list github-script tags and backing SHAs to choose a pin.
gh api repos/actions/github-script/tags --jq '.[0:20][] | "\(.name)\t\(.commit.sha)"'Repository: starcoinorg/starcoin
Length of output: 2345
🏁 Script executed:
cat -n .github/workflows/dependabot_auto_merge.ymlRepository: starcoinorg/starcoin
Length of output: 3280
Pin actions/github-script to an immutable commit SHA.
Line 18 uses a mutable tag (@v7). Since this workflow has write permissions, an unpinned action tag presents a supply-chain risk if the tag is reassigned.
Suggested change
- name: Enable auto-merge for Dependabot PRs
- uses: actions/github-script@v7
+ uses: actions/github-script@<40-char-commit-sha>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/dependabot_auto_merge.yml at line 18, Replace the mutable
GitHub Action reference "actions/github-script@v7" with an immutable pinned
commit SHA: find the step that uses actions/github-script (the line containing
uses: actions/github-script@v7), look up the repository's commit SHA for the
desired v7 release tag (via the actions/github-script repo on GitHub) and update
the uses value to uses: actions/github-script@<full-commit-sha>; ensure you use
the full 40-character commit SHA so the workflow is pinned to an immutable
revision.
jackzhhuang
force-pushed
the
parallize-vm-execute-if-needed
branch
from
March 31, 2026 09:26
cc3a814 to
786d717
Compare
jackzhhuang
force-pushed
the
parallize-vm-execute-if-needed
branch
from
April 8, 2026 02:22
786d717 to
c07d62a
Compare
sanlee42
approved these changes
Apr 8, 2026
Contributor
|
@jackhuang run ci with k8s tag |
simonjiao
added
ci-runner: k8s
ci-runner: jack
and removed
ci-runner: jack
ci-runner: k8s
labels
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
3 participants
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
Refactor
Bug Fixes
Tests
Chores / CI