Refactor arrays to avoid clones according to the cost model. #1100

azteca1998 · 2025-02-14T17:35:15Z

Checklist

Linked to Github Issue
Unit tests added
Integration tests added.
This change requires new documentation.
- Documentation has been added/updated.

github-actions · 2025-02-14T18:22:01Z

Benchmark results Main vs HEAD.

Base

Command	Mean [s]	Min [s]	Max [s]	Relative
`base dict_insert.cairo (JIT)`	3.918 ± 0.068	3.851	4.062	1.03 ± 0.02
`base dict_insert.cairo (AOT)`	3.789 ± 0.029	3.743	3.826	1.00

Head

Command	Mean [s]	Min [s]	Max [s]	Relative
`head dict_insert.cairo (JIT)`	3.828 ± 0.022	3.792	3.873	1.02 ± 0.01
`head dict_insert.cairo (AOT)`	3.752 ± 0.038	3.716	3.834	1.00

Base

Command	Mean [s]	Min [s]	Max [s]	Relative
`base dict_snapshot.cairo (JIT)`	3.762 ± 0.025	3.718	3.796	1.02 ± 0.01
`base dict_snapshot.cairo (AOT)`	3.702 ± 0.037	3.658	3.769	1.00

Head

Command	Mean [s]	Min [s]	Max [s]	Relative
`head dict_snapshot.cairo (JIT)`	3.709 ± 0.028	3.649	3.744	1.02 ± 0.01
`head dict_snapshot.cairo (AOT)`	3.624 ± 0.042	3.563	3.717	1.00

Base

Command	Mean [s]	Min [s]	Max [s]	Relative
`base factorial_2M.cairo (JIT)`	4.125 ± 0.025	4.084	4.166	1.01 ± 0.01
`base factorial_2M.cairo (AOT)`	4.066 ± 0.041	4.002	4.128	1.00

Head

Command	Mean [s]	Min [s]	Max [s]	Relative
`head factorial_2M.cairo (JIT)`	4.052 ± 0.020	4.024	4.084	1.00 ± 0.01
`head factorial_2M.cairo (AOT)`	4.038 ± 0.024	4.008	4.083	1.00

Base

Command	Mean [s]	Min [s]	Max [s]	Relative
`base fib_2M.cairo (JIT)`	3.632 ± 0.028	3.594	3.668	1.00 ± 0.01
`base fib_2M.cairo (AOT)`	3.620 ± 0.044	3.563	3.707	1.00

Head

Command	Mean [s]	Min [s]	Max [s]	Relative
`head fib_2M.cairo (JIT)`	3.623 ± 0.036	3.584	3.692	1.01 ± 0.01
`head fib_2M.cairo (AOT)`	3.598 ± 0.037	3.543	3.672	1.00

Base

Command	Mean [s]	Min [s]	Max [s]	Relative
`base linear_search.cairo (JIT)`	3.847 ± 0.029	3.822	3.917	1.03 ± 0.01
`base linear_search.cairo (AOT)`	3.723 ± 0.041	3.663	3.782	1.00

Head

Command	Mean [s]	Min [s]	Max [s]	Relative
`head linear_search.cairo (JIT)`	3.843 ± 0.024	3.804	3.874	1.01 ± 0.01
`head linear_search.cairo (AOT)`	3.792 ± 0.040	3.714	3.855	1.00

Base

Command	Mean [s]	Min [s]	Max [s]	Relative
`base logistic_map.cairo (JIT)`	3.924 ± 0.020	3.885	3.945	1.04 ± 0.01
`base logistic_map.cairo (AOT)`	3.773 ± 0.032	3.739	3.844	1.00

Head

Command	Mean [s]	Min [s]	Max [s]	Relative
`head logistic_map.cairo (JIT)`	4.041 ± 0.045	3.996	4.148	1.04 ± 0.01
`head logistic_map.cairo (AOT)`	3.871 ± 0.035	3.831	3.933	1.00

codecov-commenter · 2025-02-14T19:03:58Z

Codecov Report

Attention: Patch coverage is 93.31984% with 33 lines in your changes missing coverage. Please review.

Project coverage is 81.08%. Comparing base (b90af8c) to head (86d7968).

Files with missing lines	Patch %	Lines
src/executor/contract.rs	77.77%	12 Missing ⚠️
src/libfuncs/array.rs	96.56%	10 Missing ⚠️
src/starknet.rs	41.17%	10 Missing ⚠️
src/values.rs	98.80%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1100      +/-   ##
==========================================
+ Coverage   81.03%   81.08%   +0.04%     
==========================================
  Files         110      110              
  Lines       30204    29535     -669     
==========================================
- Hits        24477    23948     -529     
+ Misses       5727     5587     -140

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2025-02-14T19:05:55Z

Benchmarking results

Benchmark for program `dict_insert`

Open benchmarks

Command	Mean [s]	Min [s]	Max [s]	Relative
`Cairo-vm (Rust, Cairo 1)`	20.585 ± 0.232	20.343	21.097	5.38 ± 0.09
`cairo-native (embedded AOT)`	3.824 ± 0.044	3.789	3.945	1.00
`cairo-native (embedded JIT using LLVM's ORC Engine)`	3.991 ± 0.080	3.833	4.119	1.04 ± 0.02

Benchmark for program `dict_snapshot`

Open benchmarks

Command	Mean [s]	Min [s]	Max [s]	Relative
`Cairo-vm (Rust, Cairo 1)`	6.039 ± 0.053	5.937	6.107	1.61 ± 0.02
`cairo-native (embedded AOT)`	3.744 ± 0.032	3.710	3.827	1.00
`cairo-native (embedded JIT using LLVM's ORC Engine)`	3.840 ± 0.022	3.812	3.878	1.03 ± 0.01

Benchmark for program `factorial_2M`

Open benchmarks

Command	Mean [s]	Min [s]	Max [s]	Relative
`Cairo-vm (Rust, Cairo 1)`	14.203 ± 0.108	14.077	14.453	3.40 ± 0.06
`cairo-native (embedded AOT)`	4.177 ± 0.062	4.069	4.238	1.00
`cairo-native (embedded JIT using LLVM's ORC Engine)`	4.222 ± 0.061	4.153	4.327	1.01 ± 0.02

Benchmark for program `fib_2M`

Open benchmarks

Command	Mean [s]	Min [s]	Max [s]	Relative
`Cairo-vm (Rust, Cairo 1)`	14.108 ± 0.065	13.994	14.208	3.82 ± 0.02
`cairo-native (embedded AOT)`	3.696 ± 0.012	3.679	3.717	1.00
`cairo-native (embedded JIT using LLVM's ORC Engine)`	3.761 ± 0.031	3.703	3.804	1.02 ± 0.01

Benchmark for program `linear_search`

Open benchmarks

Command	Mean [s]	Min [s]	Max [s]	Relative
`Cairo-vm (Rust, Cairo 1)`	6.058 ± 0.029	6.023	6.103	1.60 ± 0.01
`cairo-native (embedded AOT)`	3.778 ± 0.022	3.752	3.823	1.00
`cairo-native (embedded JIT using LLVM's ORC Engine)`	3.936 ± 0.030	3.894	3.988	1.04 ± 0.01

Benchmark for program `logistic_map`

Open benchmarks

Command	Mean [s]	Min [s]	Max [s]	Relative
`Cairo-vm (Rust, Cairo 1)`	5.919 ± 0.079	5.844	6.061	1.51 ± 0.04
`cairo-native (embedded AOT)`	3.915 ± 0.091	3.810	4.065	1.00
`cairo-native (embedded JIT using LLVM's ORC Engine)`	4.128 ± 0.042	4.086	4.214	1.05 ± 0.03

FrancoGiachetta · 2025-02-20T14:02:30Z

src/libfuncs/array.rs

+                    let array_ptr = block.gep(
+                        context,
+                        location,
+                        array_ptr,
+                        &[GepIndex::Const(data_prefix_size as i32)],
+                        IntegerType::new(context, 8).into(),
+                    )?;


I'm a little confused here. What's the difference between this array_ptr and the one above (line 642) whose GepIndex has a negative value of data_prefix_size?

The input array_ptr (obtained from the realloc) is the allocation pointer. The offset pointer (result of the gep) is the pointer that will be stored in the array, which points to the first element (after the reference counter and max_len prefix).

The same process applies, but in reverse at line 642. Realloc needs the pointer returned by the original allocation, so we need to undo the offset (thus the negative) to obtain the original pointer and realloc.

Ah I see, thanks!

src/libfuncs/array.rs

JulianGCalderon · 2025-02-20T20:53:44Z

src/types/array.rs

+pub fn calc_data_prefix_offset(layout: Layout) -> usize {
+    get_integer_layout(32)
+        .extend(get_integer_layout(32))
+        .unwrap()
+        .0
+        .align_to(layout.align())
+        .unwrap()
+        .pad_to_align()
+        .size()
+}
+


Could you change unwrap for expect?

Also, could you document why we need two integers at the start of the array?

Should never fail, so I don't know if using expect makes sense. Would a comment explaining that it's impossible to trigger them work?

The integers are:

Reference counter: literally the number of references to the allocation.

Max length: The number of elements present in the allocation (not necessarily the length array/span being accessed, but the whole allocation). It is used to know how many elements to drop when freeing the allocation.

Would a comment explaining that it's impossible to trigger them work?

That should be fine, but isn't an unwrap + comment the same as an expect?

The integers are:

Reference counter: literally the number of references to the allocation.

Max length: The number of elements present in the allocation (not necessarily the length array/span being accessed, but the whole allocation). It is used to know how many elements to drop when freeing the allocation.

Could you add that text to the code? Maybe to lines 17-20 in this file.

JulianGCalderon · 2025-02-21T13:43:51Z

src/executor/contract.rs

@@ -520,62 +527,48 @@ impl AotContractExecutor {
        let tag = *unsafe { enum_ptr.cast::<u8>().as_ref() } as usize;
        let tag = tag & 0x01; // Filter out bits that are not part of the enum's tag.

-        // layout of both enum variants, both are a array of felts


Is this comment no longer valid? If it is, could you add it back?

JulianGCalderon · 2025-02-21T14:05:41Z

src/types/array.rs

Could you update this file's top-level documentation with the new layout?

Also, I think we should explain in a bit more detail how the drop mechanism works (and why we need the max_len field)

JulianGCalderon

The PR looks good to me, although I think the documentation could be improved. I left some comments with ideas on how to improve it.

Also, the build_pop function (and overall the whole file) is much easier to understand now, nice work.

JulianGCalderon · 2025-02-21T18:40:54Z

src/libfuncs/array.rs

Could you add some high-level documentation on how arrays operate? Some thing that could be mentioned are:

About arrays

They only grow

Operations:

append (mutates the array)

pop front (doesn't mutate the array)

About spans:

They have the same structure as arrays

They share the same double pointer, to propagate reallocs

Mutations on the original array are not propagated (as array only grows and cannot modify previous element)

Operations:

pop front

pop back

JulianGCalderon · 2025-02-21T18:42:49Z

src/types/array.rs

Also, I think we should explain in a bit more detail how the drop mechanism works (and why we need the max_len field)

JulianGCalderon · 2025-02-21T19:04:11Z

src/libfuncs/array.rs


-                let array_allocation_ptr = block.gep(
+                // TODO: Drop elements before array_start and between array_end and max length.


Is this TODO relevant? Should it be addressed in a diferent pull request?

JulianGCalderon · 2025-02-21T20:52:35Z

src/libfuncs/array.rs

-    // Signature:
-    //   Params: RangeCheck, Snapshot<Array<felt252>>, u32, u32
-    //   Branches:
-    //     0: RangeCheck, Snapshot<Array<felt252>>
-    //     1: RangeCheck


Could you restore this comment? It's useful for understanding the behaviour of the function.

azteca1998 and others added 9 commits February 13, 2025 11:04

Partial array refactor.

2cb7501

More progress.

aa2b36e

Implement array_pop_*.

fbe1935

Some fixes.

11070f1

Implement array_slice.

b05dbae

Fix contract executor.

0ff06e4

Fix other bugs.

966bcd3

Fix AOT contract runner.

207ff9e

Fix array from_ptr drops.

ac87223

Fix starknet module.

749f5c0

azteca1998 marked this pull request as ready for review February 14, 2025 18:38

azteca1998 requested review from edg-l, igaray, jrchatruc, entropidelic, fmoletta, Oppen, pefontana and gabrielbosio as code owners February 14, 2025 18:38

azteca1998 added 3 commits February 18, 2025 13:40

Fix stuff.

14c3777

Fix bug.

e212ad8

Restore makefile.

5505c9c

edg-l added the review-ready A PR that is ready for review label Feb 20, 2025

Merge branch 'main' into refactor-array-cows

610ba01

edg-l previously approved these changes Feb 20, 2025

View reviewed changes

FrancoGiachetta reviewed Feb 20, 2025

View reviewed changes

src/libfuncs/array.rs Outdated Show resolved Hide resolved

JulianGCalderon reviewed Feb 20, 2025

View reviewed changes

Fix libfunc name mistype.

ff67e7a

azteca1998 dismissed edg-l’s stale review via ff67e7a February 21, 2025 10:33

JulianGCalderon reviewed Feb 21, 2025

View reviewed changes

gabrielbosio mentioned this pull request Feb 21, 2025

Document array libfuncs #1106

Open

Merge branch 'main' into refactor-array-cows

86d7968

JulianGCalderon approved these changes Feb 21, 2025

View reviewed changes

gabrielbosio approved these changes Feb 21, 2025

View reviewed changes

gabrielbosio added this pull request to the merge queue Feb 21, 2025

Any commits made after this event will not be merged.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor arrays to avoid clones according to the cost model. #1100

Refactor arrays to avoid clones according to the cost model. #1100

azteca1998 commented Feb 14, 2025

github-actions bot commented Feb 14, 2025 •

edited

Loading

codecov-commenter commented Feb 14, 2025 •

edited

Loading

github-actions bot commented Feb 14, 2025 •

edited

Loading

FrancoGiachetta Feb 20, 2025 •

edited

Loading

azteca1998 Feb 21, 2025

FrancoGiachetta Feb 21, 2025

JulianGCalderon Feb 20, 2025

azteca1998 Feb 21, 2025 •

edited

Loading

JulianGCalderon Feb 21, 2025

JulianGCalderon Feb 21, 2025

JulianGCalderon Feb 21, 2025

JulianGCalderon Feb 21, 2025

JulianGCalderon Feb 21, 2025

JulianGCalderon left a comment

JulianGCalderon Feb 21, 2025

JulianGCalderon Feb 21, 2025

JulianGCalderon Feb 21, 2025

JulianGCalderon Feb 21, 2025


		let array_allocation_ptr = block.gep(
		// TODO: Drop elements before array_start and between array_end and max length.

Refactor arrays to avoid clones according to the cost model. #1100

Refactor arrays to avoid clones according to the cost model. #1100

Conversation

azteca1998 commented Feb 14, 2025

Checklist

github-actions bot commented Feb 14, 2025 • edited Loading

Benchmark results Main vs HEAD.

Base

Head

Base

Head

Base

Head

Base

Head

Base

Head

Base

Head

codecov-commenter commented Feb 14, 2025 • edited Loading

Codecov Report

github-actions bot commented Feb 14, 2025 • edited Loading

Benchmarking results

Benchmark for program dict_insert

Benchmark for program dict_snapshot

Benchmark for program factorial_2M

Benchmark for program fib_2M

Benchmark for program linear_search

Benchmark for program logistic_map

FrancoGiachetta Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azteca1998 Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JulianGCalderon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 14, 2025 •

edited

Loading

codecov-commenter commented Feb 14, 2025 •

edited

Loading

github-actions bot commented Feb 14, 2025 •

edited

Loading

Benchmark for program `dict_insert`

Benchmark for program `dict_snapshot`

Benchmark for program `factorial_2M`

Benchmark for program `fib_2M`

Benchmark for program `linear_search`

Benchmark for program `logistic_map`

FrancoGiachetta Feb 20, 2025 •

edited

Loading

azteca1998 Feb 21, 2025 •

edited

Loading