Remove Unsafe Blocks

[bheisler]

There are a lot of old unsafe blocks in Criterion.rs' internals. Many of them were written to work around compiler bugs or missing features that have since been fixed (see #188 (comment)). These should be cleaned up.

There are a few others that might be harder to remove without impacting analysis performance in the bootstrapping code. I've tried several times to remove these without much success so far.

[gnzlbg]

I managed to remove many unsafe blocks in #208 . Most of the remaining ones are due to the usage of thread_scoped, here we should migrate criterion either to crossbeam::scope, or maybe even just rayon. cc @cuviper thoughts?

The remaining unsafe blocks are only in the stats crate and hint about the crate's problematic design. For example, some transmutes are just used to workaround some types not having new methods (to subvert privacy boundaries), others are also used to subvert the type system in weird ways: for example, Sample<A: Float> but Distribution<A> does not require A: Float, yet distributions with As that are not floats are transmuted into Samples.

[cuviper]

or maybe even just rayon. cc @cuviper thoughts?

How could I say no? 😉 But yes, indeed, those look like prime candidates for parallel iterators.

If you wanted to get fancy, those Builders could probably implement FromParallelIterator to directly fill their target vectors, but it's probably good enough to leave it with sub_distributions and the extend merging at first.

[bheisler]

I've tried using Rayon there, in fact. It was a while ago, though, so it might work better now. The problem I ran into is that, well, some of that code is super complex (I'm looking at mixed.rs here). There is a thread-local structure (Resamples) which holds a borrow of the shared vector c. I couldn't figure out a way to do that in Rayon without creating a new Resamples instance for every iteration (potentially expensive because it contains a random number generator as well, which needs to be initialized from the thread_rng()) or crossing the borrow checker or both. I believe I also tried to implement FromParallelIterator for the Builders and gave up eventually, though I don't remember why. The other obvious approach - using split_at_mut and giving each chunk to a different scoped thread - also doesn't work, because of the way it tries to be generic over the number of output vectors.

Anyway, perhaps you folks will have more success than I did. I always meant to rip that code out and replace it with something more maintainable, but I didn't want to make it much slower. I think I might have to compromise on the performance (accept the cost of creating a new RNG for each iteration) or the generality (stop trying to be generic over the number of outputs and unzip them explicitly elsewhere).

[gnzlbg]

FWIW I gave it a quick shot too, and decided that it was tricky enough to deserve its own PR. I started with crossbeam, but while making the changes got the feeling that rayon might actually be enough.

[cuviper]

There is a thread-local structure (Resamples) which holds a borrow of the shared vector c. I couldn't figure out a way to do that in Rayon without creating a new Resamples instance for every iteration (potentially expensive because it contains a random number generator as well, which needs to be initialized from the thread_rng()) or crossing the borrow checker or both.

I'm hoping that rayon-rs/rayon#602 will help this kind of use case.

[bheisler]

That does look like it would help, yes.

[lemmih]

Status update. Criterion contains 10 unsafe blocks and 1 unsafe function:

1. criterion.rs/src/stats/univariate/percentiles.rs

   Line 20 in cee6345

   unsafe fn at_unchecked(&self, p: A) -> A {

2. criterion.rs/src/stats/univariate/percentiles.rs

   Line 52 in cee6345

   unsafe { self.at_unchecked(p) }

3. criterion.rs/src/stats/univariate/percentiles.rs

   Line 57 in cee6345

   unsafe {

4. criterion.rs/src/stats/univariate/percentiles.rs

   Line 67 in cee6345

   unsafe { self.at_unchecked(A::cast(50)) }

5. criterion.rs/src/stats/univariate/percentiles.rs

   Line 72 in cee6345

   unsafe {

6. criterion.rs/src/stats/univariate/sample.rs

   Line 31 in cee6345

   unsafe { mem::transmute(slice) }

7. criterion.rs/src/stats/univariate/sample.rs

   Line 133 in cee6345

   unsafe { mem::transmute(v) }

8. criterion.rs/src/stats/mod.rs

   Line 84 in cee6345

   unsafe { mem::transmute(slice) }

9. criterion.rs/src/stats/univariate/resamples.rs

   Line 55 in 976d517

   unsafe { mem::transmute::<&[_], _>(v) }

10. criterion.rs/src/stats/bivariate/mod.rs

    Line 126 in e228444

    unsafe { Some((self.data.0.get_unchecked(i), self.data.1.get_unchecked(i))) }

11. criterion.rs/src/lib.rs

    Line 173 in 976d517

    unsafe {

• One of them is due to black_box and is pretty unavoidable.
• Two of them are for transmuting between &[A] and &Sample<A>. This is UB since Sample<A> is not guaranteed to share the representation of [A]. We need to add #[repr(transparent)].
• The rest are for unchecked slice indexing. Surely we can remove these without hurting performance too much. Some kind of benchmarking library would be helpful here.

[bheisler]

The black_box unsafe block may be fixable with a future version of rustc; the various teams have been (slowly) working towards standardizing a black box in the std::hint namespace. When that lands (which could be a while, mind) we can finally get rid of our fake black_box function entirely, or just make it delegate directly to the one in std.