Implement Libfunc Counter #1267
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅ Code is now correctly formatted. |
FrancoGiachetta
force-pushed
the
libfunc_counter
branch
from
d6d1365 to
09ec4d1
Compare
Benchmark results Main vs HEAD.Base
Head
Base
Head
Base
Head
Base
Head
Base
Head
Base
Head
|
Benchmarking resultsBenchmark for program
|
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
11.329 ± 0.037 | 11.281 | 11.411 | 2.29 ± 0.02 |
cairo-native (embedded AOT) |
4.937 ± 0.030 | 4.888 | 4.974 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
5.030 ± 0.049 | 4.974 | 5.146 | 1.02 ± 0.01 |
Benchmark for program dict_snapshot
Open benchmarks
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
822.6 ± 7.7 | 813.7 | 840.3 | 1.00 |
cairo-native (embedded AOT) |
4975.2 ± 33.1 | 4944.0 | 5036.0 | 6.05 ± 0.07 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
5079.8 ± 38.0 | 4985.5 | 5137.2 | 6.18 ± 0.07 |
Benchmark for program factorial_2M
Open benchmarks
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
5.310 ± 0.061 | 5.259 | 5.425 | 1.00 |
cairo-native (embedded AOT) |
5.377 ± 0.033 | 5.320 | 5.437 | 1.01 ± 0.01 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
5.434 ± 0.029 | 5.398 | 5.480 | 1.02 ± 0.01 |
Benchmark for program fib_2M
Open benchmarks
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
5.165 ± 0.090 | 5.093 | 5.399 | 1.05 ± 0.02 |
cairo-native (embedded AOT) |
4.923 ± 0.040 | 4.878 | 4.991 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
4.952 ± 0.028 | 4.902 | 5.007 | 1.01 ± 0.01 |
Benchmark for program linear_search
Open benchmarks
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
852.3 ± 9.5 | 838.4 | 865.5 | 1.00 |
cairo-native (embedded AOT) |
5057.0 ± 39.4 | 4999.9 | 5132.3 | 5.93 ± 0.08 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
5179.2 ± 52.4 | 5115.6 | 5280.7 | 6.08 ± 0.09 |
Benchmark for program logistic_map
Open benchmarks
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
550.6 ± 4.8 | 544.8 | 560.8 | 1.00 |
cairo-native (embedded AOT) |
5076.0 ± 31.1 | 5028.7 | 5119.5 | 9.22 ± 0.10 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
5242.0 ± 52.6 | 5138.1 | 5307.6 | 9.52 ± 0.13 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1267 +/- ##
==========================================
- Coverage 80.12% 80.01% -0.11%
==========================================
Files 112 112
Lines 30068 30145 +77
==========================================
+ Hits 24093 24122 +29
- Misses 5975 6023 +48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
FrancoGiachetta
requested review from
edg-l,
igaray,
azteca1998,
jrchatruc,
entropidelic,
fmoletta,
Oppen,
pefontana and
gabrielbosio
as code owners
FrancoGiachetta
force-pushed
the
libfunc_counter
branch
from
0b0d8a6 to
81511e1
Compare
gabrielbosio
reviewed
Jul 2, 2025
gabrielbosio
reviewed
Jul 2, 2025
gabrielbosio
reviewed
Jul 2, 2025
gabrielbosio
reviewed
Jul 2, 2025
FrancoGiachetta
force-pushed
the
libfunc_counter
branch
from
3e1a484 to
06aef99
Compare
gabrielbosio
reviewed
Jul 3, 2025
There was a problem hiding this comment.
Some thoughts related to this file:
- Why is the libfunc counter API separated in two modules instead of using only one? For example, the compiler calls
count_libfuncfrom the runtime module butbuild_array_counterandstore_array_counterare called from the root module. - If it is necessary to have two separated modules for the API then it would be great if the documentation includes what does each module do and (if it is possible or makes sense) the reason behind this separation in modules.
- In line 4 I would not call the three methods listed below as
important. If they are the only methods used by external files to count libfuncs then these 3 methods are the Libfunc Counter API.
There was a problem hiding this comment.
- To have a separation from what belongs to MLIR, and what belongs to Rust (runtime).
- Yes, I'll add docs if you agree with (1.).
- I can change it to
mainif you prefer.
FrancoGiachetta
force-pushed
the
libfunc_counter
branch
from
f8f1460 to
71ad878
Compare
FrancoGiachetta
force-pushed
the
libfunc_counter
branch
from
af29bd2 to
d5a557d
Compare
FrancoGiachetta
force-pushed
the
libfunc_counter
branch
from
a7fb2cf to
470bc78
Compare
FrancoGiachetta
force-pushed
the
libfunc_counter
branch
from
470bc78 to
3ef6949
Compare
JulianGCalderon
requested changes
Jul 18, 2025
| @@ -57,6 +59,11 @@ struct Args { | |||
| /// The output path for the libfunc profilling results | |||
| profiler_output: Option<PathBuf>, | |||
|
|
|||
| #[cfg(feature = "with-libfunc-counter")] | |||
| #[arg(long)] | |||
| /// The output path for the execution trace | |||
There was a problem hiding this comment.
Is this comment valid?
Comment on lines
+156
to
+163
| #[cfg(feature = "with-libfunc-counter")] | ||
| let libfunc_indexes = program | ||
| .libfunc_declarations | ||
| .iter() | ||
| .enumerate() | ||
| .map(|(idx, libf)| (libf.id.clone(), idx)) | ||
| .collect::<HashMap<ConcreteLibfuncId, usize>>(); | ||
|
|
There was a problem hiding this comment.
Is this mapping necessary? The libfunc ID already seems to be autoincremental, so maybe we can use the libfunc ID directly.
Comment on lines
+176
to
+203
| pub fn count_libfunc( | ||
| &mut self, | ||
| context: &Context, | ||
| module: &Module, | ||
| block: &Block<'_>, | ||
| location: Location, | ||
| libfunc_idx: usize, | ||
| ) -> Result<()> { | ||
| let u32_ty = IntegerType::new(context, 32).into(); | ||
| let k1 = block.const_int(context, location, 1, 32)?; | ||
|
|
||
| let array_counter_ptr = self.get_array_counter(context, module, block, location)?; | ||
|
|
||
| let value_counter_ptr = block.gep( | ||
| context, | ||
| location, | ||
| array_counter_ptr, | ||
| &[GepIndex::Const(libfunc_idx as i32)], | ||
| u32_ty, | ||
| )?; | ||
|
|
||
| let value_counter = block.load(context, location, value_counter_ptr, u32_ty)?; | ||
| let value_incremented = block.addi(value_counter, k1, location)?; | ||
|
|
||
| block.store(context, location, value_counter_ptr, value_incremented)?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
I'm not convinced that this is the right approach.
Right know, to obtain the pointer to the array, we must call an extern function get_array_counter. This is more complex than required. I think this feature can be achieved without relying on invoking Rust functions from the compiled code, and without relying on a global hashmap.
- Have each contract declare an MLIR global variable called
libfunc_counts(or so). It should contain a pointer to a dinamically allocated array. - Before executing each statement, obtain the array pointer, find the index of the associated libfunc, and increment it by 1. We should be able to obtain the array pointer by accessing the MLIR global previously defined.
- At the start of the execution, inside of
invoke_dynamic, orrun, alloc the array and set global's value to the arrays pointer. At the end of the execution, don't forget to replace the global's value with the previous pointer. - To expose the libfunc counts to the user, we could return it from the
invoke_dynamic/runfunctions.
This way, the functionality would be completely hidden from the user (as opposed to the current approach, that requires changes in cairo-native-run).
There was a problem hiding this comment.
I agree It would be better for the array to be built and restored inside either invoke_dynamic or run. It should be possible with the current implementation. However I don't so much complexity in calling the a rust function. Despite this, it could be done if what we want is to avoid rust as much as possible. I actually did it this way because I thought it was simpler.
There was a problem hiding this comment.
You are right, there are two different orthogonal improvements that can be made:
- Have
invoke_dynamic/runhandle the logic for setting up and reading the libfunc counters. - Use a MLIR global rather than calling a Rust function.
We could do the first improvements without changing how we obtain the array pointer. However, using an MLIR global should be more performant and require less code than calling a Rust function.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements the Libfunc Counter. Used to count every call of each libfunc in an execution. All libfuncs that have been compiled will be taken into account (even if they never get called, in which case the counter will be 0). An example of execution a program with this feature is as follows:
{ "struct_construct<Tuple<felt252>>": 2, "function_call<user@core::panic_with_const_felt252::<375233589013918064796019>>": 0, "function_call<user@core::panic_with_const_felt252::<2138404743268917596156934978104436>>": 0, "struct_construct<Tuple<Unit>>": 1, "store_temp<Tuple<core::panics::Panic, Array<felt252>>>": 0, "store_temp<RangeCheck>": 4000027, "const_as_immediate<Const<felt252, 375233589013918064796019>>": 0, "branch_align": 4000028, "withdraw_gas": 2000012, "felt252_add": 2000010, "disable_ap_tracking": 2000013, "const_as_immediate<Const<felt252, 1>>": 2000012, "enum_match<core::panics::PanicResult::<(core::felt252,)>>": 2, "enum_init<core::panics::PanicResult::<((),)>, 1>": 0, "const_as_immediate<Const<felt252, 10>>": 1, "store_temp<core::panics::PanicResult::<(core::felt252,)>>": 2, "const_as_immediate<Const<felt252, 0>>": 2, "function_call<user@core::panic_with_const_felt252::<123770447868417490207208308>>": 0, "store_temp<GasBuiltin>": 2000015, "const_as_immediate<Const<felt252, 2138404743268917596156934978104436>>": 0, "enum_init<core::panics::PanicResult::<(core::felt252,)>, 0>": 2, "felt252_is_zero": 2000014, "redeposit_gas": 2000013, "struct_construct<Unit>": 1, "struct_deconstruct<Tuple<felt252>>": 2, "dup<felt252>": 4000022, "function_call<user@fib_2M::fib_2M::fib>": 2000012, "enum_init<core::panics::PanicResult::<((),)>, 0>": 1, "store_temp<core::panics::PanicResult::<((),)>>": 1, "store_temp<felt252>": 6000039, "const_as_immediate<Const<felt252, 123770447868417490207208308>>": 0, "struct_construct<Tuple<core::panics::Panic, Array<felt252>>>": 0, "struct_construct<core::panics::Panic>": 0, "drop<felt252>": 4, "const_as_immediate<Const<felt252, 55>>": 1, "array_new<felt252>": 0, "enum_init<core::panics::PanicResult::<(core::felt252,)>, 1>": 0, "array_append<felt252>": 0, "function_call<user@core::panic_with_felt252>": 0, "felt252_sub": 2000012, "const_as_immediate<Const<felt252, 2000000>>": 1, "drop<NonZero<felt252>>": 2000010, "const_as_immediate<Const<felt252, 3428715265070152839999402611900589559274061538403385725837342803058381925627>>": 1 }Checklist