factor::miller_rabin: Fix bug #1556

[nicoonoclaste]

• Added testcases for the bug.
• Squashed it! >:3

[nicoonoclaste]

@tdulcet, could you confirm that factor produces the same output as the GNU implementation on 10⁷..10⁸ with this fix? (I already confirmed they do the same on 2..10⁷)

[tdulcet]

Yes, I get the same output now for 2..10⁸:

$ time { seq 2 $(( 10 ** 8 )) | factor | sha256sum; }
b0f21acd0f9d4cfa81f3a828ffaa8f2b0a99eac30527142666705a54a461556b  -

real    1m18.101s
user    1m15.087s
sys     0m36.162s
$ time { seq 2 $(( 10 ** 8 )) | uu-factor | sha256sum; }
b0f21acd0f9d4cfa81f3a828ffaa8f2b0a99eac30527142666705a54a461556b  -

real    7m45.660s
user    7m50.180s
sys     0m11.295s

Thanks for fixing this so quickly!

[nicoonoclaste]

@tdulcet Thank you for noticing (and reporting) the issue 💜

[Arcterus]

🎉

[Arcterus]

FYI, this commit drops performance by roughly 50%.

[nicoonoclaste]

Oh, joy >_>'
makes a note to have a hard look in there, later.

[nicoonoclaste]

@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³².
I have ideas how to improve that (implementing Barrett reduction for Montgomery<u64>) but I would need to first set up a much better benchmarking harness, so we can check this is indeed an improvement.

[Arcterus]

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.

[tdulcet]

@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 2⁶⁴.