-
Notifications
You must be signed in to change notification settings - Fork 47
Use SierraGenerator for testing (part 1) #1125
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?
Conversation
Benchmark results Main vs HEAD.Base
Head
Base
Head
Base
Head
Base
Head
Base
Head
Base
Head
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1125 +/- ##
==========================================
+ Coverage 80.34% 80.46% +0.11%
==========================================
Files 110 110
Lines 29498 29506 +8
==========================================
+ Hits 23701 23742 +41
+ Misses 5797 5764 -33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmarking resultsBenchmark for program
|
Command | Mean [s] | Min [s] | Max [s] | Relative |
---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
15.281 ± 0.082 | 15.211 | 15.469 | 3.16 ± 0.03 |
cairo-native (embedded AOT) |
4.840 ± 0.043 | 4.761 | 4.905 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
4.928 ± 0.038 | 4.890 | 5.017 | 1.02 ± 0.01 |
Benchmark for program dict_snapshot
Open benchmarks
Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
673.2 ± 7.0 | 663.6 | 686.8 | 1.00 |
cairo-native (embedded AOT) |
4646.8 ± 37.6 | 4593.4 | 4723.2 | 6.90 ± 0.09 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
4776.5 ± 43.2 | 4706.2 | 4827.0 | 7.10 ± 0.10 |
Benchmark for program factorial_2M
Open benchmarks
Command | Mean [s] | Min [s] | Max [s] | Relative |
---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
9.295 ± 0.038 | 9.211 | 9.352 | 1.83 ± 0.02 |
cairo-native (embedded AOT) |
5.104 ± 0.024 | 5.073 | 5.137 | 1.01 ± 0.01 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
5.076 ± 0.046 | 4.983 | 5.151 | 1.00 |
Benchmark for program fib_2M
Open benchmarks
Command | Mean [s] | Min [s] | Max [s] | Relative |
---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
8.785 ± 0.032 | 8.740 | 8.838 | 1.95 ± 0.01 |
cairo-native (embedded AOT) |
4.506 ± 0.022 | 4.469 | 4.543 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
4.580 ± 0.041 | 4.503 | 4.633 | 1.02 ± 0.01 |
Benchmark for program linear_search
Open benchmarks
Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
692.7 ± 16.7 | 671.1 | 723.5 | 1.00 |
cairo-native (embedded AOT) |
4620.0 ± 26.5 | 4577.6 | 4679.5 | 6.67 ± 0.17 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
4763.8 ± 30.3 | 4739.5 | 4830.3 | 6.88 ± 0.17 |
Benchmark for program logistic_map
Open benchmarks
Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
421.3 ± 2.7 | 418.4 | 427.4 | 1.00 |
cairo-native (embedded AOT) |
4719.3 ± 31.2 | 4662.8 | 4762.0 | 11.20 ± 0.10 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
4889.0 ± 47.0 | 4843.6 | 4973.7 | 11.60 ± 0.13 |
match ty { | ||
CoreTypeConcrete::Box(info) | ||
| CoreTypeConcrete::NonZero(info) | ||
| CoreTypeConcrete::Nullable(info) | ||
| CoreTypeConcrete::Snapshot(info) => { | ||
return self.to_ptr(arena, registry, &info.ty, find_dict_drop_override); | ||
} | ||
_ => {} | ||
} | ||
|
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 don't fully understand this: if we try to call to_ptr
with a Box<uint32>
, then this function would return a pointer to an uint32, and not a double pointer to the uint32 (which is the correct type), right?
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.
At that point, the value being transformed to a pointer is a Value
from the jit, it has no knowledge it is a pointer. I think @azteca1998 or @edg-l can give a better answer.
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.
At a glance, the match should work for NonZero
and Snapshot
, but should fail for Box
and Nullable
.
Both NonZero
and Snapshot
have the same internal representation as the original type, but Box
and Nullable
are heap pointers, so we need to allocate and initialize them.
Additionally, Nullable
should handle the Value::Null
variant.
Your approach is not wrong, but it's missing those parts.
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.
mmm, both box and nullable tests are passing without any failures. Perhaps this is because the values are not really pointers since they are passed as an arguments through the test. Does this have an impact outside of tests? If not maybe I should leave it only for tests.
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.
Right now It will only be used for tests, but its better to implement it properly in case we need it later, right? If not we could be leaving bugs that show up sooner or later.
CoreTypeConcrete::Uint128MulGuarantee(_) => { | ||
native_panic!("todo: implement uint128mulguarantee from_ptr") | ||
} | ||
CoreTypeConcrete::Uint128MulGuarantee(_) => Self::Null, |
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.
As Null
is only for nullables, i'd add a new variant Unit
that could be used in these situations.
…ve into tests-to-sierra-1
This PR changes some tests to use Sierra code, rather than Cairo code.
This has the following advantages:
Related to #1152
Checklist