From c68c83c6ddc8b629d178b47ba6d7a6f987f54817 Mon Sep 17 00:00:00 2001 From: nicoo Date: Thu, 29 Apr 2021 13:50:31 +0200 Subject: [PATCH 01/15] factor::table: Take mutable refs This will be easier to adapt to working with multiple numbers to process at once. --- src/uu/factor/src/factor.rs | 2 +- src/uu/factor/src/table.rs | 12 ++++-------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/uu/factor/src/factor.rs b/src/uu/factor/src/factor.rs index ebe06a1c5..f53abd772 100644 --- a/src/uu/factor/src/factor.rs +++ b/src/uu/factor/src/factor.rs @@ -161,7 +161,7 @@ pub fn factor(mut n: u64) -> Factors { return factors; } - let (factors, n) = table::factor(n, factors); + table::factor(&mut n, &mut factors); #[allow(clippy::let_and_return)] let r = if n < (1 << 32) { diff --git a/src/uu/factor/src/table.rs b/src/uu/factor/src/table.rs index 94ad6df4c..cbd4af5e4 100644 --- a/src/uu/factor/src/table.rs +++ b/src/uu/factor/src/table.rs @@ -8,15 +8,13 @@ // spell-checker: ignore (ToDO) INVS -use std::num::Wrapping; - use crate::Factors; include!(concat!(env!("OUT_DIR"), "/prime_table.rs")); -pub(crate) fn factor(mut num: u64, mut factors: Factors) -> (Factors, u64) { +pub(crate) fn factor(num: &mut u64, factors: &mut Factors) { for &(prime, inv, ceil) in P_INVS_U64 { - if num == 1 { + if *num == 1 { break; } @@ -27,11 +25,11 @@ pub(crate) fn factor(mut num: u64, mut factors: Factors) -> (Factors, u64) { // for a nice explanation. let mut k = 0; loop { - let Wrapping(x) = Wrapping(num) * Wrapping(inv); + let x = num.wrapping_mul(inv); // While prime divides num if x <= ceil { - num = x; + *num = x; k += 1; #[cfg(feature = "coz")] coz::progress!("factor found"); @@ -43,6 +41,4 @@ pub(crate) fn factor(mut num: u64, mut factors: Factors) -> (Factors, u64) { } } } - - (factors, num) } From cd047425aaefbf9ea4bb059651dffddbcbace251 Mon Sep 17 00:00:00 2001 From: nicoo Date: Thu, 29 Apr 2021 14:15:40 +0200 Subject: [PATCH 02/15] factor::table: Add chunked implementation and microbenchmarks The factor_chunk implementation is a strawman, but getting it in place allows us to set up the microbenchmarking etc. --- src/uu/factor/Cargo.toml | 5 ++++ src/uu/factor/benches/table.rs | 44 ++++++++++++++++++++++++++++++++++ src/uu/factor/src/cli.rs | 4 ++-- src/uu/factor/src/table.rs | 9 ++++++- 4 files changed, 59 insertions(+), 3 deletions(-) create mode 100644 src/uu/factor/benches/table.rs diff --git a/src/uu/factor/Cargo.toml b/src/uu/factor/Cargo.toml index c4e7e8469..cb77c5d19 100644 --- a/src/uu/factor/Cargo.toml +++ b/src/uu/factor/Cargo.toml @@ -23,6 +23,7 @@ uucore = { version=">=0.0.8", package="uucore", path="../../uucore" } uucore_procs = { version=">=0.0.5", package="uucore_procs", path="../../uucore_procs" } [dev-dependencies] +array-init = "2.0.0" criterion = "0.3" paste = "0.1.18" quickcheck = "0.9.2" @@ -32,6 +33,10 @@ rand_chacha = "0.2.2" name = "gcd" harness = false +[[bench]] +name = "table" +harness = false + [[bin]] name = "factor" path = "src/main.rs" diff --git a/src/uu/factor/benches/table.rs b/src/uu/factor/benches/table.rs new file mode 100644 index 000000000..8fae7cef6 --- /dev/null +++ b/src/uu/factor/benches/table.rs @@ -0,0 +1,44 @@ +use array_init::array_init; +use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion}; +use uu_factor::{table::*, Factors}; + +fn table(c: &mut Criterion) { + let inputs = { + // Deterministic RNG; use an explicitely-named RNG to guarantee stability + use rand::{RngCore, SeedableRng}; + use rand_chacha::ChaCha8Rng; + const SEED: u64 = 0xdead_bebe_ea75_cafe; + let mut rng = ChaCha8Rng::seed_from_u64(SEED); + + std::iter::repeat_with(move || array_init(|_| rng.next_u64())) + }; + + let mut group = c.benchmark_group("table"); + for a in inputs.take(10) { + let a_str = format!("{:?}", a); + group.bench_with_input( + BenchmarkId::from_parameter("chunked_".to_owned() + &a_str), + &a, + |b, &a| { + b.iter(|| factor_chunk(&mut a.clone(), &mut array_init(|_| Factors::one()))); + }, + ); + group.bench_with_input( + BenchmarkId::from_parameter("seq_".to_owned() + &a_str), + &a, + |b, &a| { + b.iter(|| { + let mut n_s = a.clone(); + let mut f_s: [_; CHUNK_SIZE] = array_init(|_| Factors::one()); + for (n, f) in n_s.iter_mut().zip(f_s.iter_mut()) { + factor(n, f) + } + }) + }, + ); + } + group.finish() +} + +criterion_group!(benches, table); +criterion_main!(benches); diff --git a/src/uu/factor/src/cli.rs b/src/uu/factor/src/cli.rs index fb7b3f192..ee4c8a4c4 100644 --- a/src/uu/factor/src/cli.rs +++ b/src/uu/factor/src/cli.rs @@ -13,13 +13,13 @@ use std::error::Error; use std::io::{self, stdin, stdout, BufRead, Write}; mod factor; -pub(crate) use factor::*; +pub use factor::*; use uucore::InvalidEncodingHandling; mod miller_rabin; pub mod numeric; mod rho; -mod table; +pub mod table; static SYNTAX: &str = "[OPTION] [NUMBER]..."; static SUMMARY: &str = "Print the prime factors of the given number(s). diff --git a/src/uu/factor/src/table.rs b/src/uu/factor/src/table.rs index cbd4af5e4..72628054c 100644 --- a/src/uu/factor/src/table.rs +++ b/src/uu/factor/src/table.rs @@ -12,7 +12,7 @@ use crate::Factors; include!(concat!(env!("OUT_DIR"), "/prime_table.rs")); -pub(crate) fn factor(num: &mut u64, factors: &mut Factors) { +pub fn factor(num: &mut u64, factors: &mut Factors) { for &(prime, inv, ceil) in P_INVS_U64 { if *num == 1 { break; @@ -42,3 +42,10 @@ pub(crate) fn factor(num: &mut u64, factors: &mut Factors) { } } } + +pub const CHUNK_SIZE: usize = 4; +pub fn factor_chunk(n_s: &mut [u64; CHUNK_SIZE], f_s: &mut [Factors; CHUNK_SIZE]) { + for (n, s) in n_s.iter_mut().zip(f_s.iter_mut()) { + factor(n, s); + } +} From 1fd5f9da25d9ce7be5206e5b1eabc0364064cdfb Mon Sep 17 00:00:00 2001 From: nicoo Date: Thu, 29 Apr 2021 14:29:59 +0200 Subject: [PATCH 03/15] factor::table::factor_chunk: Turn loop inside-out This keeps the traversal of `P_INVS_U64` (a large table) to a single pass in-order, rather than `CHUNK_SIZE` passes. --- src/uu/factor/src/table.rs | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/src/uu/factor/src/table.rs b/src/uu/factor/src/table.rs index 72628054c..45464ac27 100644 --- a/src/uu/factor/src/table.rs +++ b/src/uu/factor/src/table.rs @@ -45,7 +45,30 @@ pub fn factor(num: &mut u64, factors: &mut Factors) { pub const CHUNK_SIZE: usize = 4; pub fn factor_chunk(n_s: &mut [u64; CHUNK_SIZE], f_s: &mut [Factors; CHUNK_SIZE]) { - for (n, s) in n_s.iter_mut().zip(f_s.iter_mut()) { - factor(n, s); + for &(prime, inv, ceil) in P_INVS_U64 { + if n_s[0] == 1 && n_s[1] == 1 && n_s[2] == 1 && n_s[3] == 1 { + break; + } + + for (num, factors) in n_s.iter_mut().zip(f_s.iter_mut()) { + if *num == 1 { + continue; + } + let mut k = 0; + loop { + let x = num.wrapping_mul(inv); + + // While prime divides num + if x <= ceil { + *num = x; + k += 1; + } else { + if k > 0 { + factors.add(prime, k); + } + break; + } + } + } } } From 7c287542c7cd436520b3b07f124c6dcdf69e9f36 Mon Sep 17 00:00:00 2001 From: nicoo Date: Thu, 29 Apr 2021 15:45:04 +0200 Subject: [PATCH 04/15] factor::table: Fixup microbenchmark Previous version would perform an amount of work proportional to `CHUNK_SIZE`, so this wasn't a valid way to benchmark at multiple values of that constant. The `TryInto` implementation for `&mut [T]` to `&mut [T; N]` relies on `const` generics, and is available in (stable) Rust v1.51 and later. --- src/uu/factor/benches/table.rs | 20 +++++++++++++++++--- src/uu/factor/src/table.rs | 2 +- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/uu/factor/benches/table.rs b/src/uu/factor/benches/table.rs index 8fae7cef6..ad8036d67 100644 --- a/src/uu/factor/benches/table.rs +++ b/src/uu/factor/benches/table.rs @@ -1,8 +1,16 @@ +use std::convert::TryInto; use array_init::array_init; use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion}; use uu_factor::{table::*, Factors}; fn table(c: &mut Criterion) { + const INPUT_SIZE: usize = 128; + assert!( + INPUT_SIZE % CHUNK_SIZE == 0, + "INPUT_SIZE ({}) is not divisible by CHUNK_SIZE ({})", + INPUT_SIZE, + CHUNK_SIZE + ); let inputs = { // Deterministic RNG; use an explicitely-named RNG to guarantee stability use rand::{RngCore, SeedableRng}; @@ -10,7 +18,7 @@ fn table(c: &mut Criterion) { const SEED: u64 = 0xdead_bebe_ea75_cafe; let mut rng = ChaCha8Rng::seed_from_u64(SEED); - std::iter::repeat_with(move || array_init(|_| rng.next_u64())) + std::iter::repeat_with(move || array_init::<_, _, INPUT_SIZE>(|_| rng.next_u64())) }; let mut group = c.benchmark_group("table"); @@ -20,7 +28,13 @@ fn table(c: &mut Criterion) { BenchmarkId::from_parameter("chunked_".to_owned() + &a_str), &a, |b, &a| { - b.iter(|| factor_chunk(&mut a.clone(), &mut array_init(|_| Factors::one()))); + b.iter(|| { + let mut n_s = a.clone(); + let mut f_s: [_; INPUT_SIZE] = array_init(|_| Factors::one()); + for (n_s, f_s) in n_s.chunks_mut(CHUNK_SIZE).zip(f_s.chunks_mut(CHUNK_SIZE)) { + factor_chunk(n_s.try_into().unwrap(), f_s.try_into().unwrap()) + } + }) }, ); group.bench_with_input( @@ -29,7 +43,7 @@ fn table(c: &mut Criterion) { |b, &a| { b.iter(|| { let mut n_s = a.clone(); - let mut f_s: [_; CHUNK_SIZE] = array_init(|_| Factors::one()); + let mut f_s: [_; INPUT_SIZE] = array_init(|_| Factors::one()); for (n, f) in n_s.iter_mut().zip(f_s.iter_mut()) { factor(n, f) } diff --git a/src/uu/factor/src/table.rs b/src/uu/factor/src/table.rs index 45464ac27..db2698e4b 100644 --- a/src/uu/factor/src/table.rs +++ b/src/uu/factor/src/table.rs @@ -43,7 +43,7 @@ pub fn factor(num: &mut u64, factors: &mut Factors) { } } -pub const CHUNK_SIZE: usize = 4; +pub const CHUNK_SIZE: usize = 8; pub fn factor_chunk(n_s: &mut [u64; CHUNK_SIZE], f_s: &mut [Factors; CHUNK_SIZE]) { for &(prime, inv, ceil) in P_INVS_U64 { if n_s[0] == 1 && n_s[1] == 1 && n_s[2] == 1 && n_s[3] == 1 { From 12efaa6add6d5ceab0c9c73459088faa806ec82e Mon Sep 17 00:00:00 2001 From: nicoo Date: Mon, 3 May 2021 12:26:05 +0200 Subject: [PATCH 05/15] factor: Add BENCHMARKING.md --- src/uu/factor/BENCHMARKING.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 src/uu/factor/BENCHMARKING.md diff --git a/src/uu/factor/BENCHMARKING.md b/src/uu/factor/BENCHMARKING.md new file mode 100644 index 000000000..e93bed95e --- /dev/null +++ b/src/uu/factor/BENCHMARKING.md @@ -0,0 +1,12 @@ +# Benchmarking `factor` + +## Microbenchmarking deterministic functions + +We currently use [`criterion`] to benchmark deterministic functions, +such as `gcd` and `table::factor`. + +Those benchmarks can be simply executed with `cargo bench` as usual, +but may require a recent version of Rust, *i.e.* the project's minimum +supported version of Rust does not apply to the benchmarks. + +[`criterion`]: https://bheisler.github.io/criterion.rs/book/index.html From ae15bf16a83328c08fd77f31da40af192eeedb5e Mon Sep 17 00:00:00 2001 From: nicoo Date: Mon, 3 May 2021 14:42:26 +0200 Subject: [PATCH 06/15] factor::benches::table: Report throughput (in numbers/s) --- src/uu/factor/benches/table.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/uu/factor/benches/table.rs b/src/uu/factor/benches/table.rs index ad8036d67..44ea1c863 100644 --- a/src/uu/factor/benches/table.rs +++ b/src/uu/factor/benches/table.rs @@ -1,6 +1,6 @@ -use std::convert::TryInto; use array_init::array_init; -use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion}; +use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion, Throughput}; +use std::convert::TryInto; use uu_factor::{table::*, Factors}; fn table(c: &mut Criterion) { @@ -22,6 +22,7 @@ fn table(c: &mut Criterion) { }; let mut group = c.benchmark_group("table"); + group.throughput(Throughput::Elements(INPUT_SIZE as _)); for a in inputs.take(10) { let a_str = format!("{:?}", a); group.bench_with_input( From e9f8194266125f72d8bf99ab83e8562a21c9d048 Mon Sep 17 00:00:00 2001 From: nicoo Date: Mon, 3 May 2021 14:45:00 +0200 Subject: [PATCH 07/15] =?UTF-8?q?factor::benchmarking(doc):=20Add=20guidan?= =?UTF-8?q?ce=20on=20running=20=C2=B5benches?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/uu/factor/BENCHMARKING.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/uu/factor/BENCHMARKING.md b/src/uu/factor/BENCHMARKING.md index e93bed95e..e87c965c2 100644 --- a/src/uu/factor/BENCHMARKING.md +++ b/src/uu/factor/BENCHMARKING.md @@ -9,4 +9,26 @@ Those benchmarks can be simply executed with `cargo bench` as usual, but may require a recent version of Rust, *i.e.* the project's minimum supported version of Rust does not apply to the benchmarks. + +However, µbenchmarks are by nature unstable: not only are they specific to +the hardware, operating system version, etc., but they are noisy and affected +by other tasks on the system (browser, compile jobs, etc.), which can cause +`criterion` to report spurious performance improvements and regressions. + +This can be mitigated by getting as close to [idealised conditions][lemire] +as possible: +- minimize the amount of computation and I/O running concurrently to the + benchmark, *i.e.* close your browser and IM clients, don't compile at the + same time, etc. ; +- ensure the CPU's [frequency stays constant] during the benchmark ; +- [isolate a **physical** core], set it to `nohz_full`, and pin the benchmark + to it, so it won't be preempted in the middle of a measurement ; +- disable ASLR by running `setarch -R cargo bench`, so we can compare results + across multiple executions. + **TODO**: check this propagates to the benchmark process + + [`criterion`]: https://bheisler.github.io/criterion.rs/book/index.html +[lemire]: https://lemire.me/blog/2018/01/16/microbenchmarking-calls-for-idealized-conditions/ +[isolate a **physical** core]: https://pyperf.readthedocs.io/en/latest/system.html#isolate-cpus-on-linux +[frequency stays constant]: XXXTODO From 1d75f09743a8fbe436da4d84701200f844cfae9f Mon Sep 17 00:00:00 2001 From: nicoo Date: Mon, 3 May 2021 14:45:24 +0200 Subject: [PATCH 08/15] =?UTF-8?q?factor::benchmarking(doc):=20Add=20guidan?= =?UTF-8?q?ce=20on=20writing=20=C2=B5benches?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/uu/factor/BENCHMARKING.md | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/uu/factor/BENCHMARKING.md b/src/uu/factor/BENCHMARKING.md index e87c965c2..c629252b8 100644 --- a/src/uu/factor/BENCHMARKING.md +++ b/src/uu/factor/BENCHMARKING.md @@ -32,3 +32,34 @@ as possible: [lemire]: https://lemire.me/blog/2018/01/16/microbenchmarking-calls-for-idealized-conditions/ [isolate a **physical** core]: https://pyperf.readthedocs.io/en/latest/system.html#isolate-cpus-on-linux [frequency stays constant]: XXXTODO + + +### Guidance for designing µbenchmarks + +*Note:* this guidance is specific to `factor` and takes its application domain +into account; do not expect it to generalise to other projects. It is based +on Daniel Lemire's [*Microbenchmarking calls for idealized conditions*][lemire], +which I recommend reading if you want to add benchmarks to `factor`. + +1. Select a small, self-contained, deterministic component + `gcd` and `table::factor` are good example of such: + - no I/O or access to external data structures ; + - no call into other components ; + - behaviour is deterministic: no RNG, no concurrency, ... ; + - the test's body is *fast* (~100ns for `gcd`, ~10µs for `factor::table`), + so each sample takes a very short time, minimizing variability and + maximizing the numbers of samples we can take in a given time. + +2. Benchmarks are immutable (once merged in `uutils`) + Modifying a benchmark means previously-collected values cannot meaningfully + be compared, silently giving nonsensical results. If you must modify an + existing benchmark, rename it. + +3. Test common cases + We are interested in overall performance, rather than specific edge-cases; + use **reproducibly-randomised inputs**, sampling from either all possible + input values or some subset of interest. + +4. Use [`criterion`], `criterion::black_box`, ... + `criterion` isn't perfect, but it is also much better than ad-hoc + solutions in each benchmark. From ddfcd2eb14d8046c6246c753b00e2c0466e43c17 Mon Sep 17 00:00:00 2001 From: nicoo Date: Mon, 3 May 2021 15:04:06 +0200 Subject: [PATCH 09/15] factor::benchmarking: Add wishlist / planned work --- src/uu/factor/BENCHMARKING.md | 49 +++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/uu/factor/BENCHMARKING.md b/src/uu/factor/BENCHMARKING.md index c629252b8..3ad038c15 100644 --- a/src/uu/factor/BENCHMARKING.md +++ b/src/uu/factor/BENCHMARKING.md @@ -63,3 +63,52 @@ which I recommend reading if you want to add benchmarks to `factor`. 4. Use [`criterion`], `criterion::black_box`, ... `criterion` isn't perfect, but it is also much better than ad-hoc solutions in each benchmark. + + +## Wishlist + +### Configurable statistical estimators + +`criterion` always uses the arithmetic average as estimator; in µbenchmarks, +where the code under test is fully deterministic and the measurements are +subject to additive, positive noise, [the minimum is more appropriate][lemire]. + + +### CI & reproducible performance testing + +Measuring performance on real hardware is important, as it relates directly +to what users of `factor` experience; however, such measurements are subject +to the constraints of the real-world, and aren't perfectly reproducible. +Moreover, the mitigations for it (described above) aren't achievable in +virtualized, multi-tenant environments such as CI. + +Instead, we could run the µbenchmarks in a simulated CPU with [`cachegrind`], +measure execution “time” in that model (in CI), and use it to detect and report +performance improvements and regressions. + +[`iai`] is an implementation of this idea for Rust. + +[`cachegrind`]: https://www.valgrind.org/docs/manual/cg-manual.html +[`iai`]: https://bheisler.github.io/criterion.rs/book/iai/iai.html + + +### Comparing randomised implementations across multiple inputs + +`factor` is a challenging target for system benchmarks as it combines two +characteristics: + +1. integer factoring algorithms are randomised, with large variance in + execution time ; + +2. various inputs also have large differences in factoring time, that + corresponds to no natural, linear ordering of the inputs. + + +If (1) was untrue (i.e. if execution time wasn't random), we could faithfully +compare 2 implementations (2 successive versions, or `uutils` and GNU) using +a scatter plot, where each axis corresponds to the perf. of one implementation. + +Similarly, without (2) we could plot numbers on the X axis and their factoring +time on the Y axis, using multiple lines for various quantiles. The large +differences in factoring times for successive numbers, mean that such a plot +would be unreadable. From 7c649bc74ecd425bdf15f5376979eeac973821c0 Mon Sep 17 00:00:00 2001 From: nicoo Date: Mon, 3 May 2021 16:14:38 +0200 Subject: [PATCH 10/15] factor::benches: Add check against ASLR --- src/uu/factor/BENCHMARKING.md | 1 - src/uu/factor/benches/table.rs | 27 +++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/uu/factor/BENCHMARKING.md b/src/uu/factor/BENCHMARKING.md index 3ad038c15..cf3bb35d0 100644 --- a/src/uu/factor/BENCHMARKING.md +++ b/src/uu/factor/BENCHMARKING.md @@ -25,7 +25,6 @@ as possible: to it, so it won't be preempted in the middle of a measurement ; - disable ASLR by running `setarch -R cargo bench`, so we can compare results across multiple executions. - **TODO**: check this propagates to the benchmark process [`criterion`]: https://bheisler.github.io/criterion.rs/book/index.html diff --git a/src/uu/factor/benches/table.rs b/src/uu/factor/benches/table.rs index 44ea1c863..232e59053 100644 --- a/src/uu/factor/benches/table.rs +++ b/src/uu/factor/benches/table.rs @@ -4,6 +4,9 @@ use std::convert::TryInto; use uu_factor::{table::*, Factors}; fn table(c: &mut Criterion) { + #[cfg(target_os = "linux")] + check_personality(); + const INPUT_SIZE: usize = 128; assert!( INPUT_SIZE % CHUNK_SIZE == 0, @@ -55,5 +58,29 @@ fn table(c: &mut Criterion) { group.finish() } +#[cfg(target_os = "linux")] +fn check_personality() { + use std::fs; + const ADDR_NO_RANDOMIZE: u64 = 0x0040000; + const PERSONALITY_PATH: &'static str = "/proc/self/personality"; + + let p_string = fs::read_to_string(PERSONALITY_PATH) + .expect(&format!("Couldn't read '{}'", PERSONALITY_PATH)) + .strip_suffix("\n") + .unwrap() + .to_owned(); + + let personality = u64::from_str_radix(&p_string, 16).expect(&format!( + "Expected a hex value for personality, got '{:?}'", + p_string + )); + if personality & ADDR_NO_RANDOMIZE == 0 { + eprintln!( + "WARNING: Benchmarking with ASLR enabled (personality is {:x}), results might not be reproducible.", + personality + ); + } +} + criterion_group!(benches, table); criterion_main!(benches); From 1cd001f529c80484f52175370972015abef425b9 Mon Sep 17 00:00:00 2001 From: nicoo Date: Sat, 8 May 2021 17:56:18 +0200 Subject: [PATCH 11/15] factor::benches::table: Match BenchmarkId w/ criterion's conventions See https://bheisler.github.io/criterion.rs/book/user_guide/comparing_functions.html --- src/uu/factor/benches/table.rs | 44 ++++++++++++++-------------------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/src/uu/factor/benches/table.rs b/src/uu/factor/benches/table.rs index 232e59053..0b31b2b4c 100644 --- a/src/uu/factor/benches/table.rs +++ b/src/uu/factor/benches/table.rs @@ -28,32 +28,24 @@ fn table(c: &mut Criterion) { group.throughput(Throughput::Elements(INPUT_SIZE as _)); for a in inputs.take(10) { let a_str = format!("{:?}", a); - group.bench_with_input( - BenchmarkId::from_parameter("chunked_".to_owned() + &a_str), - &a, - |b, &a| { - b.iter(|| { - let mut n_s = a.clone(); - let mut f_s: [_; INPUT_SIZE] = array_init(|_| Factors::one()); - for (n_s, f_s) in n_s.chunks_mut(CHUNK_SIZE).zip(f_s.chunks_mut(CHUNK_SIZE)) { - factor_chunk(n_s.try_into().unwrap(), f_s.try_into().unwrap()) - } - }) - }, - ); - group.bench_with_input( - BenchmarkId::from_parameter("seq_".to_owned() + &a_str), - &a, - |b, &a| { - b.iter(|| { - let mut n_s = a.clone(); - let mut f_s: [_; INPUT_SIZE] = array_init(|_| Factors::one()); - for (n, f) in n_s.iter_mut().zip(f_s.iter_mut()) { - factor(n, f) - } - }) - }, - ); + group.bench_with_input(BenchmarkId::new("factor_chunk", &a_str), &a, |b, &a| { + b.iter(|| { + let mut n_s = a.clone(); + let mut f_s: [_; INPUT_SIZE] = array_init(|_| Factors::one()); + for (n_s, f_s) in n_s.chunks_mut(CHUNK_SIZE).zip(f_s.chunks_mut(CHUNK_SIZE)) { + factor_chunk(n_s.try_into().unwrap(), f_s.try_into().unwrap()) + } + }) + }); + group.bench_with_input(BenchmarkId::new("factor", &a_str), &a, |b, &a| { + b.iter(|| { + let mut n_s = a.clone(); + let mut f_s: [_; INPUT_SIZE] = array_init(|_| Factors::one()); + for (n, f) in n_s.iter_mut().zip(f_s.iter_mut()) { + factor(n, f) + } + }) + }); } group.finish() } From 00322b986bfe9311985f64f3401112db12600813 Mon Sep 17 00:00:00 2001 From: nicoo Date: Mon, 17 May 2021 19:22:56 +0200 Subject: [PATCH 12/15] factor: Move benchmarks out-of-crate --- Cargo.toml | 3 +++ src/uu/factor/BENCHMARKING.md | 13 ++++++---- src/uu/factor/Cargo.toml | 18 +++---------- tests/benches/factor/Cargo.toml | 26 +++++++++++++++++++ .../benches}/factor/benches/gcd.rs | 0 .../benches}/factor/benches/table.rs | 0 6 files changed, 41 insertions(+), 19 deletions(-) create mode 100644 tests/benches/factor/Cargo.toml rename {src/uu => tests/benches}/factor/benches/gcd.rs (100%) rename {src/uu => tests/benches}/factor/benches/table.rs (100%) diff --git a/Cargo.toml b/Cargo.toml index 7c1a771fd..745393260 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -324,6 +324,9 @@ wc = { optional=true, version="0.0.6", package="uu_wc", path="src/uu/wc" } who = { optional=true, version="0.0.6", package="uu_who", path="src/uu/who" } whoami = { optional=true, version="0.0.6", package="uu_whoami", path="src/uu/whoami" } yes = { optional=true, version="0.0.6", package="uu_yes", path="src/uu/yes" } + +factor_benches = { optional = true, version = "0.0.0", package = "uu_factor_benches", path = "tests/benches/factor" } + # # * pinned transitive dependencies # Not needed for now. Keep as examples: diff --git a/src/uu/factor/BENCHMARKING.md b/src/uu/factor/BENCHMARKING.md index cf3bb35d0..e174d62b7 100644 --- a/src/uu/factor/BENCHMARKING.md +++ b/src/uu/factor/BENCHMARKING.md @@ -1,15 +1,18 @@ # Benchmarking `factor` +The benchmarks for `factor` are located under `tests/benches/factor` +and can be invoked with `cargo bench` in that directory. + +They are located outside the `uu_factor` crate, as they do not comply +with the project's minimum supported Rust version, *i.e.* may require +a newer version of `rustc`. + + ## Microbenchmarking deterministic functions We currently use [`criterion`] to benchmark deterministic functions, such as `gcd` and `table::factor`. -Those benchmarks can be simply executed with `cargo bench` as usual, -but may require a recent version of Rust, *i.e.* the project's minimum -supported version of Rust does not apply to the benchmarks. - - However, µbenchmarks are by nature unstable: not only are they specific to the hardware, operating system version, etc., but they are noisy and affected by other tasks on the system (browser, compile jobs, etc.), which can cause diff --git a/src/uu/factor/Cargo.toml b/src/uu/factor/Cargo.toml index cb77c5d19..eb34519f1 100644 --- a/src/uu/factor/Cargo.toml +++ b/src/uu/factor/Cargo.toml @@ -17,25 +17,15 @@ num-traits = "0.2.13" # used in src/numerics.rs, which is included by build.rs [dependencies] coz = { version = "0.1.3", optional = true } num-traits = "0.2.13" # Needs at least version 0.2.13 for "OverflowingAdd" -rand = { version="0.7", features=["small_rng"] } -smallvec = { version="0.6.14, < 1.0" } -uucore = { version=">=0.0.8", package="uucore", path="../../uucore" } -uucore_procs = { version=">=0.0.5", package="uucore_procs", path="../../uucore_procs" } +rand = { version = "0.7", features = ["small_rng"] } +smallvec = { version = "0.6.14, < 1.0" } +uucore = { version = ">=0.0.8", package = "uucore", path = "../../uucore" } +uucore_procs = { version = ">=0.0.5", package = "uucore_procs", path = "../../uucore_procs" } [dev-dependencies] -array-init = "2.0.0" -criterion = "0.3" paste = "0.1.18" quickcheck = "0.9.2" -rand_chacha = "0.2.2" -[[bench]] -name = "gcd" -harness = false - -[[bench]] -name = "table" -harness = false [[bin]] name = "factor" diff --git a/tests/benches/factor/Cargo.toml b/tests/benches/factor/Cargo.toml new file mode 100644 index 000000000..b3b718477 --- /dev/null +++ b/tests/benches/factor/Cargo.toml @@ -0,0 +1,26 @@ +[package] +name = "uu_factor_benches" +version = "0.0.0" +authors = ["nicoo "] +license = "MIT" +description = "Benchmarks for the uu_factor integer factorization tool" +homepage = "https://github.com/uutils/coreutils" +edition = "2018" + +[dependencies] +uu_factor = { path = "../../../src/uu/factor" } + +[dev-dependencies] +array-init = "2.0.0" +criterion = "0.3" +rand = "0.7" +rand_chacha = "0.2.2" + + +[[bench]] +name = "gcd" +harness = false + +[[bench]] +name = "table" +harness = false diff --git a/src/uu/factor/benches/gcd.rs b/tests/benches/factor/benches/gcd.rs similarity index 100% rename from src/uu/factor/benches/gcd.rs rename to tests/benches/factor/benches/gcd.rs diff --git a/src/uu/factor/benches/table.rs b/tests/benches/factor/benches/table.rs similarity index 100% rename from src/uu/factor/benches/table.rs rename to tests/benches/factor/benches/table.rs From 9afed1f25f244305a1e24fc17aae061d0bd58c75 Mon Sep 17 00:00:00 2001 From: nicoo Date: Mon, 17 May 2021 19:41:32 +0200 Subject: [PATCH 13/15] Update Cargo.lock Adding array-init v2.0.0 Updating cast v0.2.5 -> v0.2.6 Adding pest v2.1.3 Updating rustc_version v0.2.3 -> v0.3.3 Adding semver v0.11.0 Adding semver-parser v0.10.2 Updating serde v1.0.125 -> v1.0.126 Updating serde_derive v1.0.125 -> v1.0.126 Adding ucd-trie v0.1.3 Adding uu_factor_benches v0.0.0 (#tests/benches/factor) --- Cargo.lock | 77 ++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 63 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 77957de80..34e918b45 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -37,6 +37,12 @@ dependencies = [ "winapi 0.3.9", ] +[[package]] +name = "array-init" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6945cc5422176fc5e602e590c2878d2c2acd9a4fe20a4baa7c28022521698ec6" + [[package]] name = "arrayvec" version = "0.4.12" @@ -136,9 +142,9 @@ checksum = "14c189c53d098945499cdfa7ecc63567cf3886b3332b312a5b4585d8d3a6a610" [[package]] name = "cast" -version = "0.2.5" +version = "0.2.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cc38c385bfd7e444464011bb24820f40dd1c76bcdfa1b78611cb7c2e5cafab75" +checksum = "57cdfa5d50aad6cb4d44dcab6101a7f79925bd59d82ca42f38a9856a28865374" dependencies = [ "rustc_version", ] @@ -258,6 +264,7 @@ dependencies = [ "uu_expand", "uu_expr", "uu_factor", + "uu_factor_benches", "uu_false", "uu_fmt", "uu_fold", @@ -1027,6 +1034,15 @@ dependencies = [ "proc-macro-hack", ] +[[package]] +name = "pest" +version = "2.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "10f4872ae94d7b90ae48754df22fd42ad52ce740b8f370b03da4835417403e53" +dependencies = [ + "ucd-trie", +] + [[package]] name = "pkg-config" version = "0.3.19" @@ -1336,11 +1352,11 @@ checksum = "3e52c148ef37f8c375d49d5a73aa70713125b7f19095948a923f80afdeb22ec2" [[package]] name = "rustc_version" -version = "0.2.3" +version = "0.3.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "138e3e0acb6c9fb258b19b67cb8abd63c00679d2851805ea151465464fe9030a" +checksum = "f0dfe2087c51c460008730de8b57e6a320782fbfb312e1f4d520e6c6fae155ee" dependencies = [ - "semver", + "semver 0.11.0", ] [[package]] @@ -1370,7 +1386,16 @@ version = "0.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1d7eb9ef2c18661902cc47e535f9bc51b78acd254da71d375c2f6720d9a40403" dependencies = [ - "semver-parser", + "semver-parser 0.7.0", +] + +[[package]] +name = "semver" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f301af10236f6df4160f7c3f04eec6dbc70ace82d23326abad5edee88801c6b6" +dependencies = [ + "semver-parser 0.10.2", ] [[package]] @@ -1380,10 +1405,19 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "388a1df253eca08550bef6c72392cfe7c30914bf41df5269b68cbd6ff8f570a3" [[package]] -name = "serde" -version = "1.0.125" +name = "semver-parser" +version = "0.10.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "558dc50e1a5a5fa7112ca2ce4effcb321b0300c0d4ccf0776a9f60cd89031171" +checksum = "00b0bef5b7f9e0df16536d3961cfb6e84331c065b4066afb39768d0e319411f7" +dependencies = [ + "pest", +] + +[[package]] +name = "serde" +version = "1.0.126" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ec7505abeacaec74ae4778d9d9328fe5a5d04253220a85c4ee022239fc996d03" [[package]] name = "serde_cbor" @@ -1397,9 +1431,9 @@ dependencies = [ [[package]] name = "serde_derive" -version = "1.0.125" +version = "1.0.126" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b093b7a2bb58203b5da3056c05b4ec1fed827dcfdb37347a8841695263b3d06d" +checksum = "963a7dbc9895aeac7ac90e74f34a5d5261828f79df35cbed41e10189d3804d43" dependencies = [ "proc-macro2", "quote 1.0.9", @@ -1627,6 +1661,12 @@ version = "1.13.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "879f6906492a7cd215bfa4cf595b600146ccfac0c79bcbd1f3000162af5e8b06" +[[package]] +name = "ucd-trie" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "56dee185309b50d1f11bfedef0fe6d036842e3fb77413abef29f8f8d1c5d4c1c" + [[package]] name = "unicode-segmentation" version = "1.7.1" @@ -1922,17 +1962,26 @@ name = "uu_factor" version = "0.0.6" dependencies = [ "coz", - "criterion", "num-traits", "paste", "quickcheck", "rand 0.7.3", - "rand_chacha", "smallvec", "uucore", "uucore_procs", ] +[[package]] +name = "uu_factor_benches" +version = "0.0.0" +dependencies = [ + "array-init", + "criterion", + "rand 0.7.3", + "rand_chacha", + "uu_factor", +] + [[package]] name = "uu_false" version = "0.0.6" @@ -2407,7 +2456,7 @@ dependencies = [ "itertools 0.10.0", "rand 0.7.3", "rayon", - "semver", + "semver 0.9.0", "tempdir", "unicode-width", "uucore", From 998b3c11d3af933afdd714f1678e2080740cbb07 Mon Sep 17 00:00:00 2001 From: nicoo Date: Thu, 20 May 2021 17:00:49 +0200 Subject: [PATCH 14/15] factor: Make random Factors instance generatable for tests --- src/uu/factor/src/factor.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/uu/factor/src/factor.rs b/src/uu/factor/src/factor.rs index f53abd772..b279de7fc 100644 --- a/src/uu/factor/src/factor.rs +++ b/src/uu/factor/src/factor.rs @@ -239,9 +239,13 @@ mod tests { } #[cfg(test)] -impl quickcheck::Arbitrary for Factors { - fn arbitrary(gen: &mut G) -> Self { - use rand::Rng; +use rand::{ + distributions::{Distribution, Standard}, + Rng, +}; +#[cfg(test)] +impl Distribution for Standard { + fn sample(&self, rng: &mut R) -> Factors { let mut f = Factors::one(); let mut g = 1u64; let mut n = u64::MAX; @@ -252,7 +256,7 @@ impl quickcheck::Arbitrary for Factors { // See Generating Random Factored Numbers, Easily, J. Cryptology (2003) 'attempt: loop { while n > 1 { - n = gen.gen_range(1, n); + n = rng.gen_range(1, n); if miller_rabin::is_prime(n) { if let Some(h) = g.checked_mul(n) { f.push(n); @@ -269,6 +273,13 @@ impl quickcheck::Arbitrary for Factors { } } +#[cfg(test)] +impl quickcheck::Arbitrary for Factors { + fn arbitrary(g: &mut G) -> Self { + g.gen() + } +} + #[cfg(test)] impl std::ops::BitXor for Factors { type Output = Self; From a0a103b15e52b20f63d36aee93083c81b007326d Mon Sep 17 00:00:00 2001 From: nicoo Date: Thu, 20 May 2021 17:01:33 +0200 Subject: [PATCH 15/15] factor::table::chunked: Add test (equivalent to the single-number version) --- src/uu/factor/src/table.rs | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/uu/factor/src/table.rs b/src/uu/factor/src/table.rs index db2698e4b..518d4f241 100644 --- a/src/uu/factor/src/table.rs +++ b/src/uu/factor/src/table.rs @@ -72,3 +72,30 @@ pub fn factor_chunk(n_s: &mut [u64; CHUNK_SIZE], f_s: &mut [Factors; CHUNK_SIZE] } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::Factors; + use quickcheck::quickcheck; + use rand::{rngs::SmallRng, Rng, SeedableRng}; + + quickcheck! { + fn chunk_vs_iter(seed: u64) -> () { + let mut rng = SmallRng::seed_from_u64(seed); + let mut n_c: [u64; CHUNK_SIZE] = rng.gen(); + let mut f_c: [Factors; CHUNK_SIZE] = rng.gen(); + + let mut n_i = n_c.clone(); + let mut f_i = f_c.clone(); + for (n, f) in n_i.iter_mut().zip(f_i.iter_mut()) { + factor(n, f); + } + + factor_chunk(&mut n_c, &mut f_c); + + assert_eq!(n_i, n_c); + assert_eq!(f_i, f_c); + } + } +}