Merge #98

98: Avoid two clones in 'reduce'. r=cuviper a=lemmih

Hi,

`reduce` needlessly clones the numerator and denominator. The "proper" way to fix this would be to add a new trait bound but that is not backwards compatible. This PR proposes a temporary fix: Take the values and leave behind zeroes. This avoids cloning and `T::zero()` usually doesn't involve heap allocations (at least they don't for `BigInt`). All in all, this PR improves performance by about 10% (1150ns -> 1050ns) when creating values from small `BigInt`s (tested on an M1).
Creating a `Ratio<BigInt>` is still about 10x slower than `Ratio<isize>` but that is entirely due to problems in `num-bigint`.

Note: I copied the `rng` module from `num-bigint`.

Note: Adding a `NumAssignRef` bound would get rid of `g.clone()`.

Co-authored-by: David Himmelstrup <[email protected]>
Co-authored-by: Josh Stone <[email protected]>

ci/benchmarks/Cargo.toml

@@ -0,0 +1,21 @@
+[package]
+name = "benchmarks"
+version = "0.1.0"
+edition = "2021"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]
+
+[dependencies.num-rational]
+path = "../.."
+default-features = false
+features = ["num-bigint"]
+
+[dependencies.num-bigint]
+version = "0.4.0"
+default-features = false
+
+[dependencies.rand]
+version = "0.8"
+default-features = false

ci/benchmarks/src/main.rs

@@ -0,0 +1,32 @@
+#![feature(test)]
+
+extern crate test;
+
+use num_bigint::BigInt;
+use num_rational::{BigRational, Ratio};
+use test::Bencher;
+
+mod rng;
+use rng::get_rng;
+
+#[bench]
+fn alloc_ratio_bigint_bench(b: &mut Bencher) {
+    use rand::RngCore;
+    let mut rng = get_rng();
+    b.iter(|| {
+        let a = BigInt::from(rng.next_u64());
+        let b = BigInt::from(rng.next_u64());
+        BigRational::new(a, b)
+    });
+}
+
+#[bench]
+fn alloc_ratio_u64_bench(b: &mut Bencher) {
+    use rand::RngCore;
+    let mut rng = get_rng();
+    b.iter(|| {
+        let a = rng.next_u64();
+        let b = rng.next_u64();
+        Ratio::new(a, b)
+    });
+}

ci/benchmarks/src/rng.rs

@@ -0,0 +1,38 @@
+use rand::RngCore;
+
+pub(crate) fn get_rng() -> impl RngCore {
+    XorShiftStar {
+        a: 0x0123_4567_89AB_CDEF,
+    }
+}
+
+/// Simple `Rng` for benchmarking without additional dependencies
+struct XorShiftStar {
+    a: u64,
+}
+
+impl RngCore for XorShiftStar {
+    fn next_u32(&mut self) -> u32 {
+        self.next_u64() as u32
+    }
+
+    fn next_u64(&mut self) -> u64 {
+        // https://en.wikipedia.org/wiki/Xorshift#xorshift*
+        self.a ^= self.a >> 12;
+        self.a ^= self.a << 25;
+        self.a ^= self.a >> 27;
+        self.a.wrapping_mul(0x2545_F491_4F6C_DD1D)
+    }
+
+    fn fill_bytes(&mut self, dest: &mut [u8]) {
+        for chunk in dest.chunks_mut(8) {
+            let bytes = self.next_u64().to_le_bytes();
+            let slice = &bytes[..chunk.len()];
+            chunk.copy_from_slice(slice)
+        }
+    }
+
+    fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), rand::Error> {
+        Ok(self.fill_bytes(dest))
+    }
+}

ci/test_full.sh

@@ -63,3 +63,8 @@ done
 # test all supported features without std
 cargo build --no-default-features --features="${NO_STD_FEATURES[*]}"
 cargo test --no-default-features --features="${NO_STD_FEATURES[*]}"
+
+# make sure benchmarks can be built and sanity-tested
+if rustc --version | grep -q nightly; then
+    cargo test --manifest-path ci/benchmarks/Cargo.toml
+fi

src/lib.rs

@@ -142,18 +142,24 @@ impl<T: Clone + Integer> Ratio<T> {
         let g: T = self.numer.gcd(&self.denom);
 
         // FIXME(#5992): assignment operator overloads
-        // self.numer /= g;
         // T: Clone + Integer != T: Clone + NumAssign
-        self.numer = self.numer.clone() / g.clone();
-        // FIXME(#5992): assignment operator overloads
+
+        #[inline]
+        fn replace_with<T: Zero>(x: &mut T, f: impl FnOnce(T) -> T) {
+            let y = core::mem::replace(x, T::zero());
+            *x = f(y);
+        }
+
+        // self.numer /= g;
+        replace_with(&mut self.numer, |x| x / g.clone());
+
         // self.denom /= g;
-        // T: Clone + Integer != T: Clone + NumAssign
-        self.denom = self.denom.clone() / g;
+        replace_with(&mut self.denom, |x| x / g);
 
         // keep denom positive!
         if self.denom < T::zero() {
-            self.numer = T::zero() - self.numer.clone();
-            self.denom = T::zero() - self.denom.clone();
+            replace_with(&mut self.numer, |x| T::zero() - x);
+            replace_with(&mut self.denom, |x| T::zero() - x);
         }
     }