-
Notifications
You must be signed in to change notification settings - Fork 48
Refactor syscalls #1484
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
base: main
Are you sure you want to change the base?
Refactor syscalls #1484
Conversation
|
✅ Code is now correctly formatted. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1484 +/- ##
==========================================
+ Coverage 81.47% 83.32% +1.84%
==========================================
Files 105 105
Lines 25759 25128 -631
==========================================
- Hits 20987 20937 -50
+ Misses 4772 4191 -581 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmark results Main vs HEAD.Base
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.579 ± 0.071 | 11.488 | 11.667 | 6.09 ± 0.06 |
cairo-native (embedded AOT) |
1.902 ± 0.016 | 1.881 | 1.925 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
2.015 ± 0.027 | 1.976 | 2.050 | 1.06 ± 0.02 |
Benchmark for program dict_snapshot
Open benchmarks
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
566.2 ± 10.7 | 552.6 | 584.1 | 1.00 |
cairo-native (embedded AOT) |
1602.8 ± 25.0 | 1565.5 | 1639.7 | 2.83 ± 0.07 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
1723.5 ± 40.9 | 1692.5 | 1803.9 | 3.04 ± 0.09 |
Benchmark for program factorial_2M
Open benchmarks
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
5.304 ± 0.010 | 5.284 | 5.321 | 2.61 ± 0.03 |
cairo-native (embedded AOT) |
2.035 ± 0.019 | 2.011 | 2.067 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
2.094 ± 0.020 | 2.063 | 2.121 | 1.03 ± 0.01 |
Benchmark for program fib_2M
Open benchmarks
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
5.163 ± 0.035 | 5.129 | 5.237 | 3.32 ± 0.06 |
cairo-native (embedded AOT) |
1.557 ± 0.025 | 1.519 | 1.609 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
1.618 ± 0.023 | 1.581 | 1.653 | 1.04 ± 0.02 |
Benchmark for program heavy_circuit
Open benchmarks
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
10.032 ± 0.164 | 9.817 | 10.264 | 1.00 |
cairo-native (embedded AOT) |
13.078 ± 0.135 | 12.902 | 13.311 | 1.30 ± 0.03 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
13.339 ± 0.074 | 13.238 | 13.470 | 1.33 ± 0.02 |
Benchmark for program linear_search
Open benchmarks
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
614.2 ± 14.3 | 596.8 | 646.9 | 1.00 |
cairo-native (embedded AOT) |
1623.8 ± 19.9 | 1595.7 | 1651.9 | 2.64 ± 0.07 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
1773.2 ± 18.1 | 1746.5 | 1796.1 | 2.89 ± 0.07 |
Benchmark for program logistic_map
Open benchmarks
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
510.9 ± 6.3 | 499.4 | 518.3 | 1.00 |
cairo-native (embedded AOT) |
1748.3 ± 29.9 | 1698.0 | 1810.1 | 3.42 ± 0.07 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
1934.5 ± 12.2 | 1912.4 | 1956.9 | 3.79 ± 0.05 |
|
|
||
| let payload_ok = { | ||
| let value = entry.load( | ||
| let value = block.load( |
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 might be wrong, but in many of the syscalls we were moving the pointer with a .gep() operation and I think that is not being done anymore. Or is it being done in another place and I am missing it?
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.
An extract_value operation is similar to gep but simpler. With gep, you get to determine how much you want to offset the pointer. With extract_value you simply give the container and offset pointer based on an index. In this case, you can tell they represent the same operation because, for those syscalls using gep, they are using the result_tag_layout which is the same as indexing the ptr to the 2nd element using extract_value
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.
The blog post https://llvm.org/docs/Frontend/PerformanceTips.html#id6 suggests not to use extract_value on aggregate types, so it might be better to remove the extract value and use a gep instead.
Co-authored-by: DiegoC <[email protected]>
| /// This function receives the necessary arguments needed by the syscall as | ||
| /// well as the return types, and returns the result of its call. |
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.
How about adding the corresponding arguments the docs are referencing? For instance, if I'm not mistaken:
/// This function receives the necessary arguments `args`
This would also apply to the argument/arguments referenced in return types
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.
Don't really know if this is what you meant, but I added more docs here: 0823ea5
Co-authored-by: Gabriel Bosio <[email protected]>
| /// This function receives the necessary arguments needed by the syscall as | ||
| /// well as the return types, and returns the result of its call. |
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.
We should mention what is the expected signature of the syscall. I think not all syscalls have the same signature. Are all possible signatures supported?
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.
We should also mention what are the expected arguments:
- Is the first argument the syscall to call? If so, its better to keep it in a different argument.
- Should the callee include the return pointer? If not, we should document it.
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.
Sometimes this function is used on syscall that don't have a type for the ok variant. We should document what to do in that case.
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.
Done in: 0823ea5
| let result_ptr = helper.init_block().alloca1( | ||
| context, | ||
| location, | ||
| llvm::r#type::r#struct( | ||
| context, | ||
| &[ | ||
| result_tag_ty, | ||
| llvm::r#type::array( | ||
| IntegerType::new(context, 8).into(), | ||
| (result_layout.size() - 1).try_into()?, | ||
| ), | ||
| ], | ||
| false, | ||
| ), | ||
| result_layout.align(), | ||
| )?; |
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 wonder why this is different from typical enum type, defined in:
cairo_native/src/types/enum.rs
Line 160 in b31ad2b
| pub fn build<'ctx>( |
| [1, 0], | ||
| [ | ||
| &[remaining_gas, entry.arg(1)?, payload_err], | ||
| &[remaining_gas, entry.arg(1)?, payload_ok], |
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.
This branch has 3 values, but the signature only contains 2 values. This is not a problem because cond_br ignores the extra argument, but I would add a comment saying that payload_ok is garbage and should not used.
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.
Done in: 0823ea5
| [1, 0], | ||
| [ | ||
| &[remaining_gas, entry.arg(1)?, payload_err], | ||
| &[remaining_gas, entry.arg(1)?, payload_ok], |
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.
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.
Done in: 0823ea5
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.
Why isn't call_syscall used here?
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.
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.
Done: 0823ea5
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.
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.
Done in: 0823ea5
|
|
||
| let payload_ok = { | ||
| let value = entry.load( | ||
| let value = block.load( |
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.
The blog post https://llvm.org/docs/Frontend/PerformanceTips.html#id6 suggests not to use extract_value on aggregate types, so it might be better to remove the extract value and use a gep instead.
Refactor syscalls
Closes #NA
This PR refactors syscalls' call implementation.
Key changes:
New helper function
call_syscallwhich receives the expected arguments and return types and takes care of calling the syscall and creating the resulting payloads. This way, the amount of code gets drastically reduced.Change the type of
payload_argin syscallsend_message_to_l1. This is because it aSpan<felt252>, which represents:But it was being represented as:
Even though this was fine, since the each represent the same pointer, the former is the correct type to use.
Introduces Breaking Changes?
No.
starknet-blocks.ymlworkflow to use these PRs.These PRs should be merged after this one right away, in that order.
Checklist