-
Notifications
You must be signed in to change notification settings - Fork 84
feat: update segmentation weights for hardware memory #2341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
shuklaayush
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good but would prefer if we first do a reth benchmark run to confirm that 1.2bn cells ~ 15gb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the segmentation memory model from cell-based counting to actual hardware memory-based calculations. The changes introduce weight multipliers for different cell types and convert the final calculation to bytes, providing more accurate memory tracking for segmentation decisions.
Key changes:
- Replaced
max_cells(1.2B cells) withmax_memory(15GB) as the segmentation threshold - Introduced
main_cell_weightandinteraction_cell_weightmultipliers to account for proof system overhead - Updated memory calculation formula to:
(main_cells * main_weight + interaction_cells * interaction_weight) * base_field_size
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/vm/src/arch/execution_mode/metered/segment_ctx.rs | Core refactoring: renamed constants and fields, added weight multipliers and base field size, updated memory calculation formula with weighted cell counts |
| crates/vm/src/arch/execution_mode/metered/ctx.rs | Renamed builder method with_max_cells to with_max_memory, added builder methods for new weight configuration fields |
| crates/vm/src/arch/execution_mode/metered_cost.rs | Updated import from DEFAULT_MAX_CELLS to DEFAULT_MAX_MEMORY and corresponding usage in cost calculation |
| crates/cli/src/commands/prove.rs | Updated CLI argument from segment_max_cells to segment_max_memory, improved help text to clarify memory measurement in bytes |
| benchmarks/prove/src/util.rs | Updated CLI argument and setter method names to use memory terminology instead of cells |
| benchmarks/prove/src/bin/async_regex.rs | Updated CLI argument and setter method names to use memory terminology instead of cells |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I ran it and it did seem to give approximately similar behavior. |
segment_ctx.rs: - DEFAULT_MAX_CELLS → DEFAULT_MAX_MEMORY = 15gb - max_cells → max_memory in SegmentationLimits - set_max_cells → set_max_memory ctx.rs: - with_max_cells → with_max_memory metered_cost.rs: - Updated import to use DEFAULT_MAX_MEMORY cli/src/commands/prove.rs: - Updated import to DEFAULT_MAX_MEMORY - segment_max_cells → segment_max_memory - with_max_cells → with_max_memory benchmarks/prove/src/bin/async_regex.rs: - segment_max_cells → segment_max_memory - set_max_cells → set_max_memory benchmarks/prove/src/util.rs: - segment_max_cells → segment_max_memory - set_max_cells → set_max_memory --------- Co-authored-by: Copilot <[email protected]>
segment_ctx.rs:
ctx.rs:
metered_cost.rs:
cli/src/commands/prove.rs:
benchmarks/prove/src/bin/async_regex.rs:
benchmarks/prove/src/util.rs: