Skip to content

refactor: Move Streams into VmStateMut#1707

Merged
nyunyunyunyu merged 1 commit into
feat/new-executionfrom
refactor/streams
Jun 4, 2025
Merged

refactor: Move Streams into VmStateMut#1707
nyunyunyunyu merged 1 commit into
feat/new-executionfrom
refactor/streams

Conversation

@nyunyunyunyu

Copy link
Copy Markdown
Contributor

Previously some chips hold Arc<Mutex<Streams>> to read from Streams. Move Streams into VmStateMut so it has all data about VM state.

@codspeed-hq

codspeed-hq Bot commented Jun 3, 2025

Copy link
Copy Markdown

CodSpeed Walltime Performance Report

Merging #1707 will improve performances by 13.62%

Comparing refactor/streams (257ddb3) with feat/new-execution (82f7ff9)

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

Summary

⚡ 8 improvements
✅ 2 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
benchmark_execute[factorial_iterative_u256] 558.6 ms 506 ms +10.4%
benchmark_execute[fibonacci_iterative] 560.8 ms 503.2 ms +11.45%
benchmark_execute[fibonacci_recursive] 538.8 ms 482.1 ms +11.77%
benchmark_execute[keccak256] 566.4 ms 498.5 ms +13.62%
benchmark_execute[keccak256_iter] 560.9 ms 500.4 ms +12.08%
benchmark_execute[quicksort] 562.1 ms 508.9 ms +10.47%
benchmark_execute[sha256] 559.2 ms 500.5 ms +11.74%
benchmark_execute[sha256_iter] 538.6 ms 489.1 ms +10.11%

@codspeed-hq

codspeed-hq Bot commented Jun 3, 2025

Copy link
Copy Markdown

CodSpeed Instrumentation Performance Report

Merging #1707 will not alter performance

Comparing refactor/streams (257ddb3) with feat/new-execution (82f7ff9)

Summary

✅ 10 untouched benchmarks

@github-actions

github-actions Bot commented Jun 3, 2025

Copy link
Copy Markdown
group app.proof_time_ms app.cycles app.cells_used leaf.proof_time_ms leaf.cycles leaf.cells_used
verify_fibair (-55 [-4.7%]) 1,124 (+11347 [+3.5%]) 334,023 (+336128 [+1.9%]) 17,675,984 - - -
fibonacci (-459 [-17.7%]) 2,127 1,500,277 50,589,503 - - -
regex (-882 [-11.5%]) 6,797 4,165,432 166,519,456 - - -
ecrecover (+40 [+2.9%]) 1,436 289,547 (+43030 [+0.3%]) 14,513,216 - - -
pairing (-140 [-3.1%]) 4,359 1,820,436 (+346586 [+0.4%]) 96,178,993 - - -

Commit: 257ddb3

Benchmark Workflow

@jonathanpwang

Copy link
Copy Markdown
Contributor

To check, the logic to move into VmStateMut directly instead of Ctx is because it is shared regardless of Ctx, so might as well make it global?

@nyunyunyunyu

Copy link
Copy Markdown
Contributor Author

To check, the logic to move into VmStateMut directly instead of Ctx is because it is shared regardless of Ctx, so might as well make it global?

Yes, it's not specific to any execution mode

Comment thread crates/vm/src/arch/vm.rs

@jonathanpwang jonathanpwang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except I don't understand why the metrics thing changed

@nyunyunyunyu nyunyunyunyu merged commit 69aa7a0 into feat/new-execution Jun 4, 2025
28 of 29 checks passed
@nyunyunyunyu nyunyunyunyu deleted the refactor/streams branch June 4, 2025 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants