factor: Add 32b variant for modular arithmetic by nicoonoclaste · Pull Request #1554 · uutils/coreutils

nicoonoclaste · 2020-06-22T00:18:13Z

Twice as fast as master

Implement and test 32b variant.
Refactor to avoid code duplication between the 32 and 64b versions.
Use specialised basis for the Miller-Rabin primality test.

nicoonoclaste · 2020-06-22T00:19:47Z

I'm not sure what's the way forward there: ideally, I can make Montgomery generic over a numeric type, which means needing something like num-traits which IIRC isn't the most pleasant thing to work with.

Arcterus · 2020-06-24T08:28:37Z

@nbraud I consolidated Montgomery and Montgomery32 into one. The code needs to be cleaned up a bit, but the commit is ef50150. If you’d prefer, I can just push it to your fork.

nicoonoclaste · 2020-06-24T12:08:30Z

I successfully generalised the function for computing modular inverses (which might also be useful if we want to make the trial division table smaller, etc.), but I'm running into a couple of issues trying to refactor Montgomery to make it similarly generic:

I don't only need an integer type, but also a type of intermediate values with double the width (i.e. u32 -> u64, u64 -> u128, etc.) ; that can be solved with an associated type, but the conversions get pretty messy.
Not being able to use integer literals (i.e. write 2 * x + 1 in source or somesuch) really hurts readability.
It would make sense to replace Arithmetic::{from,to}_u64 with a {from,to}_int that takes the datatype's native integer type (i.e. u32 for Montgomery32, u64 for Montgomery64, etc.), so we would need to make the Arithmetic trait itself generic.
There are some optimisations which only apply to certain sizes; for instance, in Montgomery64 we can safely assume that 2⁶⁴ > n > 2³² (i.e. n is a u64, but couldn't be a u32 otherwise we would use Montgomery32) and that we only want to reduce 64b integers x mod n, so n² > x and we can use Barrett reduction ( I expect that to be a pretty large win, as the 128b reduction mod n in Montgomery::from_u64 consumes ~20% of the runtime in master )

nicoonoclaste · 2020-06-24T15:41:37Z

I thought I had a trait-based solution for this, but it doesn't work out: where clauses on associated types are unstable and do not support “constraints cycles” (like type DoubleWidth: From<Self> where Self: From<Self::DoubleWidth>)

I guess the way forward is to accept a little bit of code duplication, as it seems like a macro-based solution would not be too good here (even worse maintainability)

nicoonoclaste · 2020-06-24T17:37:07Z

@nbraud I consolidated Montgomery and Montgomery32 into one. The code needs to be cleaned up a bit, but the commit is ef50150. If you’d prefer, I can just push it to your fork.

Oops, sorry @Arcterus, the Github UI was hiding this message until I refreshed. I will do have a look, thanks :)

Arcterus · 2020-06-24T18:07:36Z

Oh? cp -r failed, but only on one build environment? I guess this is something to investigate later.

Arcterus · 2020-06-24T18:12:39Z

I think we need to add RUST_BACKTRACE=1 to the CI and dump stderr on failure, as we have no idea why things fail with sporadic failures like this.

I think we can apply size-specific optimizations by playing around with the traits a bit more.

Arcterus

I don’t really have any other changes. I guess just double-check that my casts/conversions are correct if you haven’t already.

nicoonoclaste · 2020-06-24T20:36:44Z

@Arcterus Thanks for the feedback, I'll look later this week; maybe tomorrow if I feel up for it

nicoonoclaste · 2020-06-25T17:42:29Z

I think we can apply size-specific optimizations by playing around with the traits a bit more.

Yes, I think I can do the optimization I had in mind in a pretty straightforward way

nicoonoclaste · 2020-07-01T13:21:13Z

~~I can't seem to load the log for the failing CI instance :(~~
Nevermind, looks like it's green now; I guess someone re-ran it?

Arcterus

These comments are rather nit-picky. After you address these, I think the code is good to go.

Arcterus · 2020-07-01T17:47:35Z

I feel like as_double() will confuse people coming from more C-like languages. It sounds like you are converting the value into a f64.

The name could be changed to as_dint() or something like that (although I don’t really like that name either). Another alternative is to find a different name for DoubleInt, although I can’t think of a good name at the moment.

Eh; I had the same thought, but I really couldn't find a better name (except maybe spelling out “double-width” in full, but that doesn't seem more helpful).

@Arcterus Were you able to think of a more-helpful name? I wasn't :(

From my review of the code, it looks like DoubleInt is being used as a dual-precision integer, generally to avoid overflow issues. Correct?

If so, how about DualPrecisionInt (or maybe VariablePrecisionInt)?
That might lead to as_high_precision() and from_high_precision() trait functions and annotations like T::HighPrecision::one().

More verbose, but possibly more informative to readers.

@Arcterus @rivy I just pushed 96224f6, which switches to hopefully-clearer names :)

PS: Also documented the type in 33a1c44

nicoonoclaste · 2020-07-02T21:59:08Z

The build failure is due to one task, Style (macos-latest, macos), failing to install the Rust toolchain.
Why are we even running what's presumably cargo fmt and clippy on 3 different OSes?

~32% faster

~3% faster

Montgomery<_> only works for odd n, so attempting to construct an instance for an even number results in a panic! The most obvious solution is to special-case even numbers.

Detected by the testsuite improvement just prior.

nicoonoclaste · 2020-07-04T22:30:14Z

@Arcterus Sorry, this got a bit out of hand, but I ended up doing some serious improvements to the testsuite to be confident that there were no bugs left, ended up finding a couple more issues in miller_rabin (which are now fixed)

nicoonoclaste · 2020-07-04T22:31:34Z

Also, this is currently twice as fast as master :)

sylvestre · 2020-07-08T12:11:21Z

Bravo for the coverage
I am afraid I don't know enough about this kind of math to do a proper review

nicoonoclaste · 2020-07-09T13:00:25Z

Bravo for the coverage
I am afraid I don't know enough about this kind of math to do a proper review

Thanks regardless 💜

sylvestre · 2020-07-15T20:48:18Z

@nbraud could you please fix the conflicts? sorry

thanks

nicoonoclaste · 2020-07-15T22:11:47Z

@nbraud could you please fix the conflicts? sorry

No need to apologise, I already had a local commit to resolve the merge conflict, I just needed to know which PR it would go to :)

rivy · 2020-07-20T12:17:18Z

Tests are passing.
@nbraud , if you're happy with the PR status, I'll take a quick overview look later today and merge.

rivy

I think that a bit of name refactoring for DoubleInt, ..., etc. would help improve the code clarity.

rivy · 2020-07-20T23:18:48Z

From my review of the code, it looks like DoubleInt is being used as a dual-precision integer, generally to avoid overflow issues. Correct?

If so, how about DualPrecisionInt (or maybe VariablePrecisionInt)?
That might lead to as_high_precision() and from_high_precision() trait functions and annotations like T::HighPrecision::one().

More verbose, but possibly more informative to readers.

- `DoubleInt::Double` renamed to `DoubleWidth` - `{as,from}_double()` renamed to `{as,from}_double_width()`. This should hopefully clarify that this is not a “double precision” floating-point type, but an integer type with a larger range (used for storing intermediate results, typ. from a multiplication)

It was unused, the debug assertions only need `to_u128`.

rivy · 2020-07-22T20:11:13Z

@nbraud , I'm happy with this ...
@Arcterus , any remaining concerns on your end?

If no, I'll merge on Friday and then start getting the the next couple of @nbraud's PRs.

nicoonoclaste · 2020-07-23T08:29:13Z

@rivy Thanks!

rivy · 2020-07-24T16:12:48Z

Merged.
If you're happy with the other two PRs and will rebase them, I'll merge them later today.

factor: Refactor and improve performance (plus a few bug fixes)

nicoonoclaste marked this pull request as ready for review June 24, 2020 17:35

Arcterus reviewed Jun 24, 2020

View reviewed changes

Comment thread src/uu/factor/src/numeric.rs Outdated

Comment thread src/uu/factor/src/numeric.rs Outdated

nicoonoclaste mentioned this pull request Jun 25, 2020

factor::miller_rabin: Fix bug #1556 #1557

Merged

2 tasks

Arcterus reviewed Jul 1, 2020

View reviewed changes

nicoonoclaste and others added 15 commits July 5, 2020 00:01

factor::numeric: Generalise modular inverse computation

43ee92c

factor::numeric: Add a 32b Montgomery variant [WiP]

e68bb19

~32% faster

factor::miller_rabin: Use a specialized basis for 32b integers

a440807

~3% faster

factor: combine Montgomery and Montgomery32

4d28f48

factor::numeric: Generalise tests for Arithmetic trait

774feb0

factor::numeric: Use debug_assert! for runtime assertions.

3f79be0

factor::numeric: Document when to remove OverflowingAdd trait

2824441

factor::numeric: Split Int and DoubleInt traits

caa79a1

factor::numeric::Arithmetic: Rename associated type I to ModInt

19a8231

factor::numeric: Refactor away the use of {To,From}Primitives

53954ba

factor::numeric: Generate implementations of Int with a macro

f95f977

factor::numeric: Generate implementations of DoubleInt with a macro

b25c77c

factor::numeric::OverflowingAdd: Generate impls with a macro

d2b43f4

factor::miller_rabin::is_prime: Fix bug

3082903

Montgomery<_> only works for odd n, so attempting to construct an instance for an even number results in a panic! The most obvious solution is to special-case even numbers.

factor::miller_rabin: Add test for the largest 64b composite numbers

0a1200b

nicoonoclaste added 4 commits July 5, 2020 00:04

factor::miller_rabin: Squash another bug! >:3

cbcc760

Detected by the testsuite improvement just prior.

factor::numeric::tests: Use a macro to instantiate every test

7a1b86c

factor::miller_rabin: Use a macro to instantiate every test

6256750

Merge branches 'factor/faster/{centralise_logic, montgomery32}'

6e228d3

nicoonoclaste mentioned this pull request Jul 4, 2020

factor: Refactor the factoring logic #1558

Merged

3 tasks

sylvestre reviewed Jul 8, 2020

View reviewed changes

Comment thread src/uu/factor/Cargo.toml Outdated

fixup! factor::numeric: Generalise modular inverse computation

c6276b7

nicoonoclaste mentioned this pull request Jul 17, 2020

factor::miller_rabin: Minor improvements #1562

Merged

2 tasks

rivy self-requested a review July 20, 2020 12:16

rivy suggested changes Jul 20, 2020

View reviewed changes

nicoonoclaste added 3 commits July 21, 2020 19:39

factor::numeric::Int: Remove from_u128 method

141d813

It was unused, the debug assertions only need `to_u128`.

factor::numeric::DoubleInt: Document the DoubleWidth associated type

33a1c44

nicoonoclaste requested review from Arcterus and rivy July 21, 2020 17:51

rivy approved these changes Jul 22, 2020

View reviewed changes

rivy merged this pull request into uutils:master Jul 24, 2020

rivy added a commit that referenced this pull request Jul 24, 2020

Merge pull request #1554 from nbraud/factor/faster/montgomery32

8cda0f5

factor: Refactor and improve performance (plus a few bug fixes)

nicoonoclaste deleted the factor/faster/montgomery32 branch July 24, 2020 21:02

nicoonoclaste mentioned this pull request Jul 24, 2020

Speed up factor::numeric::gcd #1563

Merged

3 tasks

Uh oh!

Conversation

nicoonoclaste commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoonoclaste commented Jun 22, 2020

Uh oh!

Arcterus commented Jun 24, 2020

Uh oh!

nicoonoclaste commented Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoonoclaste commented Jun 24, 2020

Uh oh!

nicoonoclaste commented Jun 24, 2020

Uh oh!

Arcterus commented Jun 24, 2020

Uh oh!

Arcterus commented Jun 24, 2020

Uh oh!

Arcterus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nicoonoclaste commented Jun 24, 2020

Uh oh!

nicoonoclaste commented Jun 25, 2020

Uh oh!

nicoonoclaste commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Arcterus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Arcterus Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

nicoonoclaste Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

nicoonoclaste Jul 4, 2020

Choose a reason for hiding this comment

Uh oh!

rivy Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoonoclaste Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

nicoonoclaste Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicoonoclaste commented Jul 2, 2020

Uh oh!

nicoonoclaste commented Jul 4, 2020

Uh oh!

nicoonoclaste commented Jul 4, 2020

Uh oh!

sylvestre commented Jul 8, 2020

Uh oh!

Uh oh!

nicoonoclaste commented Jul 9, 2020

Uh oh!

sylvestre commented Jul 15, 2020

Uh oh!

nicoonoclaste commented Jul 15, 2020

Uh oh!

rivy commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rivy left a comment

Choose a reason for hiding this comment

Uh oh!

rivy Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

nicoonoclaste commented Jun 22, 2020 •

edited

Loading

nicoonoclaste commented Jun 24, 2020 •

edited

Loading

nicoonoclaste commented Jul 1, 2020 •

edited

Loading

rivy Jul 20, 2020 •

edited

Loading

rivy commented Jul 20, 2020 •

edited

Loading

rivy Jul 20, 2020 •

edited

Loading