Skip to content

Commit

Permalink
Fix benchmarks
Browse files Browse the repository at this point in the history
  • Loading branch information
pitdicker committed May 3, 2018
1 parent 87620f5 commit a70b664
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 23 deletions.
29 changes: 16 additions & 13 deletions benches/misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,58 +7,61 @@ const RAND_BENCH_N: u64 = 1000;

use test::{black_box, Bencher};

use rand::{SeedableRng, SmallRng, Rng, thread_rng};
use rand::{SeedableRng, SmallRng, StdRng, Rng, thread_rng};
use rand::seq::*;

#[bench]
fn misc_gen_bool_const(b: &mut Bencher) {
let mut rng = SmallRng::from_rng(&mut thread_rng()).unwrap();
let mut rng = StdRng::from_rng(&mut thread_rng()).unwrap();
b.iter(|| {
// Can be evaluated at compile time.
let mut accum = true;
for _ in 0..::RAND_BENCH_N {
accum ^= rng.gen_bool(0.18);
}
accum
black_box(accum);

This comment has been minimized.

Copy link
@vks

vks May 3, 2018

Isn't this black_box redundant?

This comment has been minimized.

Copy link
@pitdicker

pitdicker May 3, 2018

Author Owner

Depends on how smart LLVM is. But as I understand it you always have to either return something (println! etc.) or use black_box, otherwise LLVM should be able to figure out the loop does nothing. I suppose using thread_rng() does at least part of the trick here, as tls should be invisible to it (?).

But better to be safe I think, we hit our head plenty of times with parts of benchmarks being optimized out.

This comment has been minimized.

Copy link
@vks

vks May 3, 2018

I thought the only point of returning something in Bencher::iter was to avoid it being optimized out.

This comment has been minimized.

Copy link
@pitdicker

pitdicker May 3, 2018

Author Owner

That would be nice! Are you sure that works, and why?

This comment has been minimized.

Copy link
@vks

vks May 3, 2018

Not sure why exactly, but it is claimed by the docs:

The benchmarking runner offers two ways to avoid this. Either, the closure that the iter method receives can return an arbitrary value which forces the optimizer to consider the result used and ensures it cannot remove the computation entirely.

This comment has been minimized.

Copy link
@pitdicker

pitdicker May 4, 2018

Author Owner

Do you want to change the benchmarks in rand to use this pattern?

})
}

#[bench]
fn misc_gen_bool_var(b: &mut Bencher) {
let mut rng = SmallRng::from_rng(&mut thread_rng()).unwrap();
let mut rng = StdRng::from_rng(&mut thread_rng()).unwrap();
b.iter(|| {
let mut accum = true;
let mut p = 0.18;
black_box(&mut p); // Avoid constant folding.
for _ in 0..::RAND_BENCH_N {
black_box(rng.gen_bool(p));
accum ^= rng.gen_bool(p);
p += 0.0001;
}
black_box(accum);
})
}

#[bench]
fn misc_bernoulli_const(b: &mut Bencher) {
let mut rng = SmallRng::from_rng(&mut thread_rng()).unwrap();
let mut rng = StdRng::from_rng(&mut thread_rng()).unwrap();
let d = rand::distributions::Bernoulli::new(0.18);
b.iter(|| {
// Can be evaluated at compile time.
let mut accum = true;
for _ in 0..::RAND_BENCH_N {
accum ^= rng.sample(d);
}
accum
black_box(accum);
})
}

#[bench]
fn misc_bernoulli_var(b: &mut Bencher) {
let mut rng = SmallRng::from_rng(&mut thread_rng()).unwrap();
let mut rng = StdRng::from_rng(&mut thread_rng()).unwrap();
b.iter(|| {
let mut accum = true;
let mut p = 0.18;
black_box(&mut p); // Avoid constant folding.
let d = rand::distributions::Bernoulli::new(p);
for _ in 0..::RAND_BENCH_N {
black_box(rng.sample(d));
let d = rand::distributions::Bernoulli::new(p);
accum ^= rng.sample(d);
p += 0.0001;
}
black_box(accum);
})
}

Expand Down
2 changes: 1 addition & 1 deletion src/distributions/bernoulli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ impl Bernoulli {
/// 2<sup>-64</sup> in `[0, 1]` can be represented as a `f64`.)
#[inline]
pub fn new(p: f64) -> Bernoulli {
assert!(p >= 0.0 & p <= 1.0, "Bernoulli::new not called with 0 <= p <= 0");
assert!(p >= 0.0 && p <= 1.0, "Bernoulli::new not called with 0 <= p <= 0");
let p_int = if p < 1.0 {
(p * (::core::u64::MAX as f64)) as u64
} else {
Expand Down
22 changes: 13 additions & 9 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -465,8 +465,6 @@ pub trait Rng: RngCore {

/// Return a bool with a probability `p` of being true.
///
/// This is a wrapper around [`distributions::Bernoulli`].
///
/// # Example
///
/// ```rust
Expand All @@ -476,15 +474,21 @@ pub trait Rng: RngCore {
/// println!("{}", rng.gen_bool(1.0 / 3.0));
/// ```
///
/// # Panics
///
/// If `p` < 0 or `p` > 1.
/// # Accuracy note
///
/// [`distributions::Bernoulli`]: distributions/bernoulli/struct.Bernoulli.html
/// `gen_bool` uses 32 bits of the RNG, so if you use it to generate close
/// to or more than `2^32` results, a tiny bias may become noticable.
/// A notable consequence of the method used here is that the worst case is
/// `rng.gen_bool(0.0)`: it has a chance of 1 in `2^32` of being true, while
/// it should always be false. But using `gen_bool` to consume *many* values
/// from an RNG just to consistently generate `false` does not match with
/// the intent of this method.
fn gen_bool(&mut self, p: f64) -> bool {
let d = distributions::Bernoulli::new(p);
self.sample(d)
}
assert!(p >= 0.0 && p <= 1.0);
// If `p` is constant, this will be evaluated at compile-time.
let p_int = (p * f64::from(core::u32::MAX)) as u32;
self.gen::<u32>() <= p_int
}

/// Return a random element from `values`.
///
Expand Down

0 comments on commit a70b664

Please sign in to comment.