Skip to content
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

Fix Box<T> and Nullable<T> as entry point arguments. #733

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

azteca1998
Copy link
Collaborator

Something I missed from the previous fix.

Checklist

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

@azteca1998 azteca1998 marked this pull request as ready for review July 17, 2024 17:34
Copy link

github-actions bot commented Jul 17, 2024

Benchmarking results

Benchmark for program factorial_2M

Open benchmarks
Command Mean [s] Min [s] Max [s] Relative
Cairo-vm (Rust, Cairo 1) 11.605 ± 0.238 11.306 12.035 24.14 ± 0.50
cairo-native (embedded AOT) 1.512 ± 0.011 1.495 1.528 3.15 ± 0.02
cairo-native (embedded JIT using LLVM's ORC Engine) 1.684 ± 0.069 1.627 1.819 3.50 ± 0.14
cairo-native (standalone AOT) 0.653 ± 0.003 0.651 0.662 1.36 ± 0.01
cairo-native (standalone AOT with -march=native) 0.481 ± 0.001 0.480 0.482 1.00

Benchmark for program fib_2M

Open benchmarks
Command Mean [s] Min [s] Max [s] Relative
Cairo-vm (Rust, Cairo 1) 10.717 ± 0.050 10.650 10.785 1319.55 ± 16.99
cairo-native (embedded AOT) 1.109 ± 0.014 1.091 1.134 136.53 ± 2.33
cairo-native (embedded JIT using LLVM's ORC Engine) 1.127 ± 0.015 1.106 1.151 138.76 ± 2.48
cairo-native (standalone AOT) 0.008 ± 0.000 0.008 0.009 1.04 ± 0.02
cairo-native (standalone AOT with -march=native) 0.008 ± 0.000 0.008 0.009 1.00

Benchmark for program logistic_map

Open benchmarks
Command Mean [s] Min [s] Max [s] Relative
Cairo-vm (Rust, Cairo 1) 1.907 ± 0.018 1.887 1.942 28.82 ± 0.31
cairo-native (embedded AOT) 1.268 ± 0.010 1.253 1.292 19.17 ± 0.18
cairo-native (embedded JIT using LLVM's ORC Engine) 1.422 ± 0.011 1.401 1.437 21.49 ± 0.21
cairo-native (standalone AOT) 0.107 ± 0.000 0.107 0.107 1.62 ± 0.01
cairo-native (standalone AOT with -march=native) 0.066 ± 0.000 0.066 0.068 1.00

@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.81%. Comparing base (ec85d3d) to head (9e8b2f8).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #733      +/-   ##
==========================================
+ Coverage   91.78%   91.81%   +0.02%     
==========================================
  Files         121      121              
  Lines       32496    32535      +39     
==========================================
+ Hits        29827    29872      +45     
+ Misses       2669     2663       -6     

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

@edg-l edg-l added the review-ready A PR that is ready for review label Jul 22, 2024
(value, CoreTypeConcrete::Box(info)) => {
let ptr = value
.to_jit(self.arena, self.registry, self.type_id)
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these unwraps safe?
Should be implement an error handling here? Maybe we can make it in another PR if it is necesarry

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, yes. It could technically fail, but it'll most likely be because of an invalid Sierra program or internal Cairo error (the libraries we're using).
We can turn it into an error later on along with all the others once we fix our current bugs.

} else {
let ptr = value
.to_jit(self.arena, self.registry, self.type_id)
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pefontana pefontana added this pull request to the merge queue Aug 1, 2024
Merged via the queue into main with commit 9cb9b37 Aug 1, 2024
9 checks passed
@pefontana pefontana deleted the test-box-nullable-abiargument branch August 1, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-ready A PR that is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants