Skip to content

feat: add host configurable memory cell type to MemoryConfig#1903

Merged
jonathanpwang merged 17 commits into
feat/new-executionfrom
INT-4397
Jul 26, 2025
Merged

feat: add host configurable memory cell type to MemoryConfig#1903
jonathanpwang merged 17 commits into
feat/new-executionfrom
INT-4397

Conversation

@jonathanpwang

@jonathanpwang jonathanpwang commented Jul 25, 2025

Copy link
Copy Markdown
Contributor

Add trait AddressSpaceHostLayout with concrete implementation as enum MemoryCellType.

Add AddressSpaceHostConfig struct and now MemoryConfig stores a vec of these, one per address space.
Now all handling of conversions from raw bytes to F is done through this config.

Comparison: https://github.com/axiom-crypto/openvm-reth-benchmark/actions/runs/16532679208
Memory finalize time seems to have increased significantly percentage-wise, but it is still on the order of milliseconds so I think negligible?

@codspeed-hq

codspeed-hq Bot commented Jul 25, 2025

Copy link
Copy Markdown

CodSpeed WallTime Performance Report

Merging #1903 will degrade performances by 23.01%

Comparing INT-4397 (e82c995) with feat/new-execution (0f1213d)

⚠️ 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

⚡ 2 improvements
❌ 2 regressions
✅ 20 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
benchmark_execute[fibonacci_iterative] 52.8 ms 40.9 ms +28.98%
benchmark_execute[fibonacci_recursive] 81.4 ms 66.7 ms +22.03%
benchmark_execute[keccak256] 37 ms 48.1 ms -23.01%
benchmark_execute[sha256] 37.2 ms 46.5 ms -20.03%

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@jonathanpwang

Copy link
Copy Markdown
Contributor Author

@Golovanov399 should be ready for review

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Golovanov399 Golovanov399 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.

Thank you

Comment thread CHANGELOG.md
## [Unreleased]

### Added
- (Config) Added `addr_spaces` vector of `AddressSpaceHostConfig` to `MemoryConfig`.

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.

And removed addr_space_sizes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

addr_space_sizes was only added in feat/new-execution so 1.3.0 doesn't know about it

Comment thread crates/vm/src/arch/config.rs Outdated
Self::Null => unreachable!(),
Self::U8 => F::from_canonical_u8(*value.get_unchecked(0)),
Self::U16 => F::from_canonical_u16(core::ptr::read(value.as_ptr() as *const u16)),
Self::U32 => F::from_canonical_u32(core::ptr::read(value.as_ptr() as *const u32)),

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.

is there any advantage of U32 <-> F being not the same as Native <-> F?

@jonathanpwang jonathanpwang Jul 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just put it there in case for some reason we want to use canonical form instead of Montgomery form or maybe to support PrimeField64, but right now we don't need it

Comment thread crates/vm/src/arch/testing/memory/mod.rs
Comment thread crates/vm/src/system/memory/online.rs
Co-authored-by: Golovanov399 <Golovanov12345@gmail.com>
@github-actions

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 (+16 [+1.5%]) 1,100 322,698 17,672,384 - - -
fibonacci (+10 [+0.5%]) 2,204 3,000,556 51,508,517 - - -
regex (-100 [-1.3%]) 7,444 8,330,866 165,431,684 - - -
ecrecover (+2 [+0.2%]) 1,125 274,568 8,596,011 - - -
pairing (+35 [+1.0%]) 3,681 3,725,930 93,105,233 - - -

Commit: e82c995

Benchmark Workflow

@jonathanpwang jonathanpwang merged commit b788e45 into feat/new-execution Jul 26, 2025
34 checks passed
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