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

use our own is_prime impl #1238

Merged
merged 1 commit into from
Jun 15, 2023
Merged

Conversation

tdelabro
Copy link
Contributor

custom is_prime

Description

num-prime cannot be used in a no_std env.
This PR replace the calls to num_prime::is_prime with our custom impl

It incidentally restore no_std support for cairo-vm

@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #1238 (2b77816) into main (4221723) will increase coverage by 0.00%.
The diff coverage is 99.55%.

@@           Coverage Diff            @@
##             main    #1238    +/-   ##
========================================
  Coverage   97.61%   97.62%            
========================================
  Files          88       89     +1     
  Lines       36112    37001   +889     
========================================
+ Hits        35251    36122   +871     
- Misses        861      879    +18     
Impacted Files Coverage Δ
vm/src/math_utils/is_prime.rs 99.55% <99.55%> (ø)
vm/src/math_utils/mod.rs 99.39% <100.00%> (ø)
vm/src/serde/deserialize_program.rs 96.75% <100.00%> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

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

Thanks for your help! 😄

@tdelabro
Copy link
Contributor Author

By the way, do you have a way to check if this new way of checking for prime numbers does not hurt performances?

@MegaRedHand
Copy link
Contributor

Yes! We have benchmarks that run as part of the CI. The numbers usually appear as a comment on the PR, but in this case, it fails because of permissions. You can see the raw output in the logs.

@MegaRedHand MegaRedHand added this pull request to the merge queue Jun 15, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 15, 2023
@MegaRedHand MegaRedHand added this pull request to the merge queue Jun 15, 2023
Merged via the queue into lambdaclass:main with commit 80dd475 Jun 15, 2023
30 of 31 checks passed
@tdelabro tdelabro deleted the restore-no_std branch June 15, 2023 23:03
@tdelabro
Copy link
Contributor Author

@MegaRedHand
base field_arithmetic_get_square_benchmark | 153.4 ± 5.7 | 148.9 | 168.5 | 1.00 |\n|`

± 5.7 is it bad?

@MegaRedHand
Copy link
Contributor

MegaRedHand commented Jun 15, 2023

It ran ~14% slower than main (you can see that here), but no-std is a priority. We already mitigated it by adding the original implementation as an std alternative in #1232. Also opened an issue to look for a better alternative: #1243.

kariy pushed a commit to dojoengine/cairo-rs that referenced this pull request Jun 23, 2023
kariy pushed a commit to dojoengine/cairo-rs that referenced this pull request Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants