-
Notifications
You must be signed in to change notification settings - Fork 732
performance(sierra-to-casm): Using builder with capacity for instructions and relocations. #9490
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
This stack of pull requests is managed by Graphite. Learn more about stacking. |
eytan-starkware
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.
@eytan-starkware reviewed 1 file and all commit messages, and made 4 comments.
Reviewable status: 1 of 33 files reviewed, 4 unresolved discussions (waiting on @orizi and @TomerStarkware).
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 70 at r1 (raw file):
let [start_ptr] = builder.try_get_single_cells()?; let full_struct_size = builder.program_info.type_sizes[ty]; let mut casm_builder = CasmBuilder::with_capacity(0, 0);
casm_build_extend will lead to a larger capacity. 3?
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 122 at r1 (raw file):
let [expr_arr, elem] = builder.try_get_refs()?; let [arr_start, arr_end] = expr_arr.try_unpack()?; let mut casm_builder = CasmBuilder::with_capacity(elem.cells.len(), 0);
Isnt deref and ++ two seperate instructions?
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 182 at r1 (raw file):
let element_size = builder.program_info.type_sizes[elem_ty]; let mut casm_builder = CasmBuilder::with_capacity(3, 2);
I htink jumps are also part of instructions in the builder, so 5,2?
crates/cairo-lang-sierra-to-casm/src/invocations/bitwise.rs line 24 at r1 (raw file):
let [bitwise, x, y] = builder.try_get_single_cells()?; let mut casm_builder = CasmBuilder::with_capacity(2, 0);
5, 0? Do asserts create a jump?
orizi
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.
@orizi made 4 comments.
Reviewable status: 1 of 33 files reviewed, 4 unresolved discussions (waiting on @eytan-starkware and @TomerStarkware).
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 70 at r1 (raw file):
Previously, eytan-starkware wrote…
casm_build_extend will lead to a larger capacity. 3?
no instructions are generated.
instruction count == number of asserts and jumps (tempvar with assignment is an assert as well)
relocation count == number of jumps.
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 122 at r1 (raw file):
Previously, eytan-starkware wrote…
Isnt deref and ++ two seperate instructions?
ditto.
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 182 at r1 (raw file):
Previously, eytan-starkware wrote…
I htink jumps are also part of instructions in the builder, so 5,2?
ditto.
crates/cairo-lang-sierra-to-casm/src/invocations/bitwise.rs line 24 at r1 (raw file):
Previously, eytan-starkware wrote…
5, 0? Do asserts create a jump?
only the asserts.
b894b20 to
99a862b
Compare
b056dc0 to
cd3be03
Compare
eytan-starkware
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.
@eytan-starkware reviewed 2 files and all commit messages, made 5 comments, and resolved 4 discussions.
Reviewable status: 2 of 33 files reviewed, 5 unresolved discussions (waiting on @orizi and @TomerStarkware).
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 218 at r2 (raw file):
let index = expr_index.try_unpack_single()?; let element_size = builder.program_info.type_sizes[elem_ty]; let mut casm_builder = CasmBuilder::with_capacity(9, 2);
10, 2 (I suspect it didnt count maybe_tempvar)
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 287 at r2 (raw file):
let element_size = builder.program_info.type_sizes[elem_ty]; let mut casm_builder = CasmBuilder::with_capacity(11, 2);
ditto?
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 354 at r2 (raw file):
let [arr_start, arr_end] = builder.try_get_refs::<1>()?[0].try_unpack()?; let element_size = builder.program_info.type_sizes[elem_ty]; let mut casm_builder = CasmBuilder::with_capacity(0, 0);
1, 0 (else case)
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 388 at r2 (raw file):
let [arr_start, arr_end] = expr_arr.try_unpack()?; let popped_size = builder.program_info.type_sizes[&libfunc.popped_ty]; let mut casm_builder = CasmBuilder::with_capacity(8, 1);
6 + 4 or 6 + 3?
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 438 at r2 (raw file):
let [arr_start, arr_end] = expr_arr.try_unpack()?; let popped_size = builder.program_info.type_sizes[&libfunc.popped_ty]; let mut casm_builder = CasmBuilder::with_capacity(8, 1);
ditto
eytan-starkware
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.
I think it would just be easier to always give a capacity of 16 or 32, as it is not a lot of space, and allocation won't take longer.
@eytan-starkware made 1 comment.
Reviewable status: 2 of 33 files reviewed, 5 unresolved discussions (waiting on @orizi and @TomerStarkware).
cd3be03 to
1d053b2
Compare
99a862b to
6a874aa
Compare
orizi
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.
even if marginal - probably better.
@orizi made 6 comments.
Reviewable status: 1 of 33 files reviewed, 5 unresolved discussions (waiting on @eytan-starkware and @TomerStarkware).
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 218 at r2 (raw file):
Previously, eytan-starkware wrote…
10, 2 (I suspect it didnt count maybe_tempvar)
Done.
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 287 at r2 (raw file):
Previously, eytan-starkware wrote…
ditto?
nope - recounted - 11.
(tempvar is_in_range; - not an instruction, just an allocation)
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 354 at r2 (raw file):
Previously, eytan-starkware wrote…
1, 0 (else case)
i think that case would allocate an array, so cheaper all and all.
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 388 at r2 (raw file):
Previously, eytan-starkware wrote…
6 + 4 or 6 + 3?
3+5
crates/cairo-lang-sierra-to-casm/src/invocations/array.rs line 438 at r2 (raw file):
Previously, eytan-starkware wrote…
ditto
3 + 5
eytan-starkware
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.
@eytan-starkware reviewed 10 files, made 5 comments, and resolved 5 discussions.
Reviewable status: 11 of 33 files reviewed, 5 unresolved discussions (waiting on @orizi and @TomerStarkware).
crates/cairo-lang-sierra-to-casm/src/invocations/felt252_dict.rs line 731 at r3 (raw file):
) -> Result<CompiledInvocation, InvocationError> { let [dict_ptr, key] = builder.try_get_single_cells()?; let mut casm_builder = CasmBuilder::with_capacity(8, 2);
1, 0?
crates/cairo-lang-sierra-to-casm/src/invocations/felt252_dict.rs line 755 at r3 (raw file):
) -> Result<CompiledInvocation, InvocationError> { let [dict_entry, new_value] = builder.try_get_single_cells()?; let mut casm_builder = CasmBuilder::with_capacity(8, 2);
1, 0?
crates/cairo-lang-sierra-to-casm/src/invocations/felt252.rs line 45 at r3 (raw file):
) -> Result<CompiledInvocation, InvocationError> { let [a, b] = builder.try_get_single_cells()?; let mut casm_builder = CasmBuilder::with_capacity(0, 0);
1, 0
crates/cairo-lang-sierra-to-casm/src/invocations/blake.rs line 38 at r3 (raw file):
hint AllocConstantSize { size: state_size } into { dst: output }; }; casm_builder.blake2s_compress(state, byte_count, message, finalize);
blake adds an instruction
crates/cairo-lang-sierra-to-casm/src/invocations/debug.rs line 23 at r3 (raw file):
) -> Result<CompiledInvocation, InvocationError> { let [arr_start, arr_end] = builder.try_get_refs::<1>()?[0].try_unpack()?; let mut casm_builder = CasmBuilder::with_capacity(1, 0);
0, 0 ?
1d053b2 to
72f2b03
Compare
orizi
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.
@orizi made 5 comments.
Reviewable status: 11 of 33 files reviewed, 5 unresolved discussions (waiting on @eytan-starkware and @TomerStarkware).
crates/cairo-lang-sierra-to-casm/src/invocations/blake.rs line 38 at r3 (raw file):
Previously, eytan-starkware wrote…
blake adds an instruction
Done.
crates/cairo-lang-sierra-to-casm/src/invocations/debug.rs line 23 at r3 (raw file):
Previously, eytan-starkware wrote…
0, 0 ?
ap += 0 is an instruction.
crates/cairo-lang-sierra-to-casm/src/invocations/felt252.rs line 45 at r3 (raw file):
Previously, eytan-starkware wrote…
1, 0
Done.
crates/cairo-lang-sierra-to-casm/src/invocations/felt252_dict.rs line 731 at r3 (raw file):
Previously, eytan-starkware wrote…
1, 0?
Done
crates/cairo-lang-sierra-to-casm/src/invocations/felt252_dict.rs line 755 at r3 (raw file):
Previously, eytan-starkware wrote…
1, 0?
Done.
eytan-starkware
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.
@eytan-starkware reviewed 24 files and all commit messages, made 8 comments, and resolved 5 discussions.
Reviewable status: 31 of 33 files reviewed, 8 unresolved discussions (waiting on @orizi and @TomerStarkware).
crates/cairo-lang-sierra-to-casm/src/invocations/starknet/storage.rs line 16 at r4 (raw file):
) -> Result<CompiledInvocation, InvocationError> { let [base, offset] = builder.try_get_single_cells()?; let mut casm_builder = CasmBuilder::with_capacity(8, 2);
0, 0?
crates/cairo-lang-sierra-to-casm/src/invocations/int/unsigned.rs line 77 at r4 (raw file):
) -> Result<CompiledInvocation, InvocationError> { let [range_check, value] = builder.try_get_single_cells()?; let mut casm_builder = CasmBuilder::with_capacity(16, 4);
no jumps?
crates/cairo-lang-sierra-to-casm/src/invocations/starknet/mod.rs line 113 at r4 (raw file):
let [gas_builtin] = builder.refs[0].expression.try_unpack()?; let [system] = builder.refs[1].expression.try_unpack()?; let mut casm_builder = CasmBuilder::with_capacity(32, 8);
Depends on input count
crates/cairo-lang-sierra-to-casm/src/invocations/gas.rs line 121 at r4 (raw file):
)); } let mut casm_builder = CasmBuilder::with_capacity(8, 2);
Seems to be wrong due to loop in add_get_total_requested_count_code
crates/cairo-lang-sierra-to-casm/src/invocations/gas.rs line 148 at r4 (raw file):
) -> Result<CompiledInvocation, InvocationError> { let [gas_counter] = builder.try_get_single_cells()?; let mut casm_builder = CasmBuilder::with_capacity(16, 4);
Also
crates/cairo-lang-sierra-to-casm/src/invocations/int/unsigned128.rs line 93 at r4 (raw file):
) -> Result<CompiledInvocation, InvocationError> { let [a, b] = builder.try_get_single_cells()?; let mut casm_builder = CasmBuilder::with_capacity(8, 2);
No jumps?
crates/cairo-lang-sierra-to-casm/src/invocations/int/unsigned128.rs line 122 at r4 (raw file):
let [a, b, res_high, res_low] = guarantee.try_unpack()?; let mut casm_builder = CasmBuilder::with_capacity(32, 8);
Also? Can a hint cause a jump?
crates/cairo-lang-sierra-to-casm/src/invocations/int/unsigned512.rs line 32 at r4 (raw file):
let mut casm_builder = CasmBuilder::with_capacity(64, 16); add_input_variables! {casm_builder,
How is this only 64 and u256 is like 120
…ions and relocations. SIERRA_UPDATE_PATCH_CHANGE_TAG=No interface changes.
72f2b03 to
3c0537f
Compare
orizi
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.
@orizi made 8 comments.
Reviewable status: 13 of 33 files reviewed, 8 unresolved discussions (waiting on @eytan-starkware and @TomerStarkware).
crates/cairo-lang-sierra-to-casm/src/invocations/gas.rs line 121 at r4 (raw file):
Previously, eytan-starkware wrote…
Seems to be wrong due to loop in add_get_total_requested_count_code
decided to go with 16 that would cover almost all cases.
crates/cairo-lang-sierra-to-casm/src/invocations/gas.rs line 148 at r4 (raw file):
Previously, eytan-starkware wrote…
Also
decided to go with 16 that would cover almost all cases.
crates/cairo-lang-sierra-to-casm/src/invocations/int/unsigned.rs line 77 at r4 (raw file):
Previously, eytan-starkware wrote…
no jumps?
Done.
crates/cairo-lang-sierra-to-casm/src/invocations/int/unsigned128.rs line 93 at r4 (raw file):
Previously, eytan-starkware wrote…
No jumps?
Done.
crates/cairo-lang-sierra-to-casm/src/invocations/int/unsigned128.rs line 122 at r4 (raw file):
Previously, eytan-starkware wrote…
Also? Can a hint cause a jump?
Done.
crates/cairo-lang-sierra-to-casm/src/invocations/int/unsigned512.rs line 32 at r4 (raw file):
Previously, eytan-starkware wrote…
How is this only 64 and u256 is like 120
Done.
crates/cairo-lang-sierra-to-casm/src/invocations/starknet/mod.rs line 113 at r4 (raw file):
Previously, eytan-starkware wrote…
Depends on input count
Done.
crates/cairo-lang-sierra-to-casm/src/invocations/starknet/storage.rs line 16 at r4 (raw file):
Previously, eytan-starkware wrote…
0, 0?
Done.
eytan-starkware
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.
@eytan-starkware reviewed 20 files and all commit messages, made 1 comment, and resolved 8 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @TomerStarkware).

Summary
Added a
with_capacityconstructor toCasmBuilderto pre-allocate memory for instructions and relocations, and updated all invocation implementations to use this constructor with appropriate capacity estimates. This optimization reduces memory allocations during the Sierra to CASM compilation process.Type of change
Please check one:
Why is this change needed?
The
CasmBuilderwas previously usingDefault::default()for all vector allocations, which resulted in many small allocations and reallocations as instructions were added. By pre-allocating vectors with appropriate capacities based on the expected number of instructions and relocations for each invocation type, we can reduce memory allocations and improve compilation performance.What was the behavior or documentation before?
Previously, all invocations created a
CasmBuilderusingDefault::default(), which initialized empty vectors for instructions and relocations. These vectors would need to reallocate memory as elements were added, causing performance overhead.What is the behavior or documentation after?
Now,
CasmBuilderhas a newwith_capacityconstructor that pre-allocates memory for instructions and relocations. All invocation implementations have been updated to use this constructor with appropriate capacity estimates based on their expected usage patterns. TheDefaultimplementation now useswith_capacity(1, 0)as a minimal allocation.Additional context
This change is purely a performance optimization that doesn't affect functionality. It reduces memory allocations during the Sierra to CASM compilation process, which should improve compilation speed, especially for larger programs.