factor: Faster modular arithmetic with the Montgomery transform

[nicoonoclaste]

This can probably be optimised further, but as of commit 4851619 this is already ~2.43 times faster as previously, taking ~3.55s for all integers from 2 to 10⁶.

• Add tests to factor::{factor,miller_rabin}.
• Rework the Arithmetic trait, implement the Montgomery transform on 64b integers (requires 128b integers for some intermediate values)
• Add consistency checks with debug_assert!

A further optimisation would be to make the Montgomery implementation generic, and add a 32b variant. Moreover, a 32b variant could use a shorter basis in the Miller-Rabin primality test (either 3 witnesses, or a single witness depending on n).

This seems however out-of-scope for this PR. Moreover, such an optimisation would be currently premature, and its impact hard to measure: after this change, factor only spends ~10% of its time in the Miller-Rabin primality test, and another ~10% in Pollard's ρ algorithm, versus ~35% in table::factor and ~25% printing the factorisations (!)

[nicoonoclaste]

Marked WIP as 2 of the tests I introduced are now failing, after switching to the Montgomery transform. I suspect there is an overflow-related issue somewhere.

[Arcterus]

Let me know when you finish this. It looks good to me other than the test failures.

[nicoonoclaste]

@Arcterus I fixed one of the test failures, will take a break before having a go at the next one.

[nicoonoclaste]

This should now be working, and ~45% faster than master.

This also unlocks a lot of further algorithmic work, as we now have fast, non-overflowing modular arithmetic.

[nicoonoclaste]

Update: about 59% faster than master.

[sylvestre]

We have integrations tests here:
https://github.com/uutils/coreutils/blob/master/tests/by-util/test_factor.rs
Could you make sure that your changes are also covered here? thanks :)

[nicoonoclaste]

We have integrations tests here:
https://github.com/uutils/coreutils/blob/master/tests/by-util/test_factor.rs
Could you make sure that your changes are also covered here? thanks :)

Yes, the integration tests do exercise the new code too (essentially, anything that calls into miller_rabin::test or rho::factor does)

[nicoonoclaste]

@sylvestre Done :)

[Arcterus]

This fails test_random_big for me. It’s apparently overflowing somewhere.

[nicoonoclaste]

This fails test_random_big for me. It’s apparently overflowing somewhere.

Odd; can you provide a log output (and preferably the backtrace too) ?

[Arcterus]

Here’s the backtrace. There is no output on stdout, and this is all that is on stderr (when run with RUST_BACKTRACE=1. Sorry that the formatting is a little messed up.

thread \'main\' panicked at \'attempt to add with overflow\', src/uu/factor/src/numeric.rs:126:17
stack backtrace:
   $
: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace0.3.46/src/bac$trace/libunwind.rs:86
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db$ec823/backtrace-0.3.46/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_commo$/backtrace.rs:78
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/lib$td/sys_common/backtrace.rs:59
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1069
   5: std::io::Write::write_fm$
             at src/libstd/io/mod.rs:1504
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backt$ace.rs:62
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:49
   8: std::panicking::d$fault_hook::{{closure}}
             at src/libstd/panicking.rs:198
   9: std::panicking::default_hook
             at src/libs$d/panicking.rs:218
  10: <alloc::boxed::Box<F> as core::ops::function::Fn<A>>::call
             at /rustc/49cae55760da0a43428eb$73abcb659bb70cf2e4/src/liballoc/boxed.rs:1022
  11: uucore::mods::panic::mute_sigpipe_panic::{{closure}}
             at /home/arcterus/.cargo/git/checkouts/uucore-cbba7adad0ea8524/47ad6ab/src/lib/mods/panic.rs:15
  12: std::panicking::rust_panic_with_hook
            at src/libstd/panicking.rs:515
  13: rust_begin_unwind
             at src/libstd/panicking.rs:419
  14: core::panicking::panic_fmt
             at src/libcore/panicking.rs:111
  15: core::panicking::panic
             at src/libcore/panicking.rs:54
  16: <uu_factor::numeric::Montgomery as uu_factor::numeric::Arithmetic>::add
             at src/uu/factor/src/numeric.rs:126          
  17: uu_factor::rho::find_divisor::{{closure}}::{{closure}}
             at src/uu/factor/src/rho.rs:18
  18: uu_factor::rho::find_divisor
             at src/uu/factor/src/rho.rs:26
  19: uu_factor::rho::_factor
             at src/uu/factor/src/rho.rs:70        
  20: uu_factor::rho::factor
             at src/uu/factor/src/rho.rs:77
  21: uu_factor::factor
             at src/uu/factor/src/factor.rs:105
  22: uu_factor::print_factors
             at src/uu/factor/src/factor.rs:112
  23: uu_factor::print_factors_str::{{closure}}
             at src/uu/factor/src/factor.rs:118
  24: core::result::Result<T,E>::and_then
             at /rustc/49cae55760da0a43428eba73abcb659bb70cf2e4/src/libcore/result.rs:729
  25: uu_factor::print_factors_str
             at src
/uu/factor/src/factor.rs:117
  26: uu_factor::uumain
             at src/uu/factor/src/factor.rs:132
  27: coreutils::main
         at src/bin/coreutils.rs:80
  28: std::rt::lang_start::{{closure}}
             at /rustc/49cae55760da0a43428eba73abcb659bb70cf2e4/src/libstd/rt.rs:67
  29: std::rt::lang_start_internal::{{closure}}
             at src/libstd/rt.rs:52
  30: std::pan
icking::try::do_call
             at src/libstd/panicking.rs:331
  31: std::panicking::try
             at src/libstd/panicking.rs:274
  32: std::panic::catch_unwind
             at src/libstd/panic.rs:394
  33: std::rt::lang_start_internal
             at src/libstd/rt.rs:51
  34: std::rt::lang_start
             at /rustc/49cae55760da0a43428eba73abcb659bb70cf2e4/src/libstd/rt.rs:
67           
  35: main
  36: __libc_start_main
  37: _start 
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose
 backtrace.

[nicoonoclaste]

@Arcterus Thanks, I think I know what the issue might be then. Will have a deeper look after dinner.

[Arcterus]

Sounds good 👍

[nicoonoclaste]

@Arcterus Fixed there's an unrelated CI failure (failed while running rustup) though

[nicoonoclaste]

🎉