-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
factor::miller_rabin: Fix bug #1556 #1557
Conversation
nbraud
commented
Jun 23, 2020
- Added testcases for the bug.
- Squashed it! >:3
@tdulcet, could you confirm that |
Yes, I get the same output now for 2..10⁸:
Thanks for fixing this so quickly! |
@tdulcet Thank you for noticing (and reporting) the issue 💜 |
🎉 |
FYI, this commit drops performance by roughly 50%. |
Oh, joy >_>' |
@Arcterus Looks like the performance regression is fixed incidentally by #1554 : this fix causes more numbers (up to the entire basis) to be converted to Montgomery representation, which costs one double-width modular reduction, and that's really expensive on 128b numbers. #1554 is not a complete fix, as this still causes a performance regression for instance above 2³². |
Ah, well that’s rather convenient. We should probably set up benchmarking project-wide as I imagine other utilities could benefit from improved performance as well. |
@nbraud No problem. Here is the Bash script I have been using to test and benchmark this and the GNU factor implementation, if it is helpful. I adapted part of it from the GNU factor tests. Some of the tests will not work until this supports numbers greater than 264. |