-
Notifications
You must be signed in to change notification settings - Fork 44
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
Make dicts clone on write. #964
base: main
Are you sure you want to change the base?
Conversation
Benchmarking resultsBenchmark for program
|
Command | Mean [s] | Min [s] | Max [s] | Relative |
---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
13.712 ± 0.079 | 13.597 | 13.845 | 10.16 ± 0.06 |
cairo-native (embedded AOT) |
4.298 ± 0.016 | 4.273 | 4.320 | 3.18 ± 0.01 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
4.113 ± 0.026 | 4.079 | 4.159 | 3.05 ± 0.02 |
cairo-native (standalone AOT with -march=native) |
1.350 ± 0.003 | 1.347 | 1.357 | 1.00 |
Benchmark for program fib_2M
Open benchmarks
Command | Mean [s] | Min [s] | Max [s] | Relative |
---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
14.090 ± 0.117 | 13.941 | 14.319 | 180.06 ± 2.31 |
cairo-native (embedded AOT) |
3.850 ± 0.038 | 3.788 | 3.903 | 49.21 ± 0.69 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
3.643 ± 0.043 | 3.576 | 3.735 | 46.55 ± 0.72 |
cairo-native (standalone AOT with -march=native) |
0.078 ± 0.001 | 0.078 | 0.082 | 1.00 |
Benchmark for program linear_search
Open benchmarks
Command | Mean [s] | Min [s] | Max [s] | Relative |
---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
5.855 ± 0.041 | 5.757 | 5.900 | 2943.51 ± 66.33 |
cairo-native (embedded AOT) |
3.960 ± 0.046 | 3.923 | 4.065 | 1990.83 ± 48.42 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
3.863 ± 0.024 | 3.836 | 3.903 | 1942.00 ± 43.25 |
cairo-native (standalone AOT with -march=native) |
0.002 ± 0.000 | 0.002 | 0.002 | 1.00 |
Benchmark for program logistic_map
Open benchmarks
Command | Mean [s] | Min [s] | Max [s] | Relative |
---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
5.737 ± 0.061 | 5.660 | 5.815 | 23.62 ± 0.26 |
cairo-native (embedded AOT) |
4.104 ± 0.019 | 4.064 | 4.127 | 16.90 ± 0.09 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
3.927 ± 0.027 | 3.893 | 3.985 | 16.16 ± 0.12 |
cairo-native (standalone AOT with -march=native) |
0.243 ± 0.001 | 0.242 | 0.244 | 1.00 |
Benchmark results Main vs HEAD.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #964 +/- ##
==========================================
+ Coverage 80.76% 80.82% +0.06%
==========================================
Files 107 108 +1
Lines 29256 29749 +493
==========================================
+ Hits 23628 24046 +418
- Misses 5628 5703 +75 ☔ View full report in Codecov by Sentry. |
find_dict_overrides: impl Copy | ||
+ Fn( | ||
&ConcreteTypeId, | ||
) -> ( | ||
Option<extern "C" fn(*mut c_void, *mut c_void)>, | ||
Option<extern "C" fn(*mut c_void)>, | ||
), |
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.
Could you add a comment explaining why is the argument necessary?
Also, maybe we could define a type alias and use it everywhere else, as it's quite verbose. Do you think it will make it more readable?
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 can't make type aliases for impl types in stable Rust yet (it's unstable).
I can add a comment, but I'm not sure where I should add it. I'll add it where we actually use it (in Value::to_ptr
).
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.
Aah, it's a pity.
Yes, you could add it to Value::to_ptr, and maybe a short mention to it in the trait definition, pointing developers to the Value::to_ptr documentation
Depends on #963.
Checklist