From 9532eb5d6950685c68dd8db1a00b3b4c29c71541 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 7 Jul 2019 14:32:24 -0400 Subject: [PATCH 1/4] Make test 'iter_panic_fuse' deterministic Fixes #667 'panic_fuse' will only stop other threads 'as soon as possible' - if the other threads are fast enough, they might end up processing the entire rest of the iterator. This commit changes the test 'iter_panic_fuse' to properly take this into account, by creating a custom threadpool with only 1 thread. This makes the test deterministic - with only one thread, the panic is guaranmteed to be observed when the next item is processed, causing the desired early exit. I've also added the 'more-asserts' crate as a dev dependency, so that we can print out a more informative error message in 'iter_panic_fuse' --- Cargo.toml | 1 + tests/iter_panic.rs | 48 +++++++++++++++++++++++++-------------------- 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index a4dec8c70..8e72d39b6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,3 +34,4 @@ rand_xorshift = "0.1" serde = "1" serde_derive = "1" doc-comment = "0.3" +more-asserts = "0.2.1" diff --git a/tests/iter_panic.rs b/tests/iter_panic.rs index 0497dc870..43f53cd42 100644 --- a/tests/iter_panic.rs +++ b/tests/iter_panic.rs @@ -1,4 +1,6 @@ extern crate rayon; +#[macro_use] +extern crate more_asserts; use rayon::prelude::*; use std::ops::Range; @@ -22,26 +24,30 @@ fn iter_panic() { #[test] fn iter_panic_fuse() { - fn count(iter: impl ParallelIterator + UnwindSafe) -> usize { - let count = AtomicUsize::new(0); - let result = panic::catch_unwind(|| { - iter.for_each(|_| { - count.fetch_add(1, Ordering::Relaxed); + // We only use a single thread in order to make the behavior + // of 'panic_fuse' deterministic + rayon::ThreadPoolBuilder::new().num_threads(1).build().unwrap().install(|| { + fn count(iter: impl ParallelIterator + UnwindSafe) -> usize { + let count = AtomicUsize::new(0); + let result = panic::catch_unwind(|| { + iter.for_each(|_| { + count.fetch_add(1, Ordering::Relaxed); + }); }); - }); - assert!(result.is_err()); - count.into_inner() - } - - // Without `panic_fuse()`, we'll reach every item except the panicking one. - let expected = ITER.len() - 1; - let iter = ITER.into_par_iter().with_max_len(1); - assert_eq!(count(iter.clone().inspect(check)), expected); - - // With `panic_fuse()` anywhere in the chain, we'll reach fewer items. - assert!(count(iter.clone().inspect(check).panic_fuse()) < expected); - assert!(count(iter.clone().panic_fuse().inspect(check)) < expected); - - // Try in reverse to be sure we hit the producer case. - assert!(count(iter.clone().panic_fuse().inspect(check).rev()) < expected); + assert!(result.is_err()); + count.into_inner() + } + + // Without `panic_fuse()`, we'll reach every item except the panicking one. + let expected = ITER.len() - 1; + let iter = ITER.into_par_iter().with_max_len(1); + assert_eq!(count(iter.clone().inspect(check)), expected); + + // With `panic_fuse()` anywhere in the chain, we'll reach fewer items. + assert_lt!(count(iter.clone().inspect(check).panic_fuse()), expected); + assert_lt!(count(iter.clone().panic_fuse().inspect(check)), expected); + + // Try in reverse to be sure we hit the producer case. + assert_lt!(count(iter.clone().panic_fuse().inspect(check).rev()), expected); + }); } From 42d1d4f4ef0e94428cb920c197e4847f2249df7f Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Fri, 12 Jul 2019 19:28:25 -0400 Subject: [PATCH 2/4] Revert 'more-asserts' dependency addition --- Cargo.toml | 1 - tests/iter_panic.rs | 8 +++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 8e72d39b6..a4dec8c70 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,4 +34,3 @@ rand_xorshift = "0.1" serde = "1" serde_derive = "1" doc-comment = "0.3" -more-asserts = "0.2.1" diff --git a/tests/iter_panic.rs b/tests/iter_panic.rs index 43f53cd42..a719166e8 100644 --- a/tests/iter_panic.rs +++ b/tests/iter_panic.rs @@ -1,6 +1,4 @@ extern crate rayon; -#[macro_use] -extern crate more_asserts; use rayon::prelude::*; use std::ops::Range; @@ -44,10 +42,10 @@ fn iter_panic_fuse() { assert_eq!(count(iter.clone().inspect(check)), expected); // With `panic_fuse()` anywhere in the chain, we'll reach fewer items. - assert_lt!(count(iter.clone().inspect(check).panic_fuse()), expected); - assert_lt!(count(iter.clone().panic_fuse().inspect(check)), expected); + assert!(count(iter.clone().inspect(check).panic_fuse()) < expected); + assert!(count(iter.clone().panic_fuse().inspect(check)) < expected); // Try in reverse to be sure we hit the producer case. - assert_lt!(count(iter.clone().panic_fuse().inspect(check).rev()), expected); + assert!(count(iter.clone().panic_fuse().inspect(check).rev()) < expected); }); } From 67a9aa22e91e2b3e4884e3b9c408e6cff2a73243 Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Thu, 18 Jul 2019 13:58:14 -0700 Subject: [PATCH 3/4] Split the pool in iter_panic_fuse for formatting's sake --- tests/iter_panic.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/iter_panic.rs b/tests/iter_panic.rs index a719166e8..f40071287 100644 --- a/tests/iter_panic.rs +++ b/tests/iter_panic.rs @@ -1,6 +1,7 @@ extern crate rayon; use rayon::prelude::*; +use rayon::ThreadPoolBuilder; use std::ops::Range; use std::panic::{self, UnwindSafe}; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -24,7 +25,9 @@ fn iter_panic() { fn iter_panic_fuse() { // We only use a single thread in order to make the behavior // of 'panic_fuse' deterministic - rayon::ThreadPoolBuilder::new().num_threads(1).build().unwrap().install(|| { + let pool = ThreadPoolBuilder::new().num_threads(1).build().unwrap(); + + pool.install(|| { fn count(iter: impl ParallelIterator + UnwindSafe) -> usize { let count = AtomicUsize::new(0); let result = panic::catch_unwind(|| { From 2a2c5a8961a458215f2fc16dbc0e7bb8ddd37ad3 Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Thu, 18 Jul 2019 14:03:31 -0700 Subject: [PATCH 4/4] Use a single thread to test cmp short circuit --- src/iter/test.rs | 90 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 63 insertions(+), 27 deletions(-) diff --git a/src/iter/test.rs b/src/iter/test.rs index e920986a5..d2a6c1dee 100644 --- a/src/iter/test.rs +++ b/src/iter/test.rs @@ -447,53 +447,89 @@ fn check_cmp_gt_to_seq() { #[test] fn check_cmp_short_circuit() { + // We only use a single thread in order to make the short-circuit behavior deterministic. + let pool = ThreadPoolBuilder::new().num_threads(1).build().unwrap(); + let a = vec![0; 1024]; let mut b = a.clone(); b[42] = 1; - let counter = AtomicUsize::new(0); - let result = a - .par_iter() - .inspect(|_| { - counter.fetch_add(1, Ordering::SeqCst); - }) - .cmp(&b); - assert!(result == ::std::cmp::Ordering::Less); - assert!(counter.load(Ordering::SeqCst) < a.len()); // should not have visited every single one + pool.install(|| { + let expected = ::std::cmp::Ordering::Less; + assert_eq!(a.par_iter().cmp(&b), expected); + + for len in 1..10 { + let counter = AtomicUsize::new(0); + let result = a + .par_iter() + .with_max_len(len) + .inspect(|_| { + counter.fetch_add(1, Ordering::SeqCst); + }) + .cmp(&b); + assert_eq!(result, expected); + // should not have visited every single one + assert!(counter.into_inner() < a.len()); + } + }); } #[test] fn check_partial_cmp_short_circuit() { + // We only use a single thread to make the short-circuit behavior deterministic. + let pool = ThreadPoolBuilder::new().num_threads(1).build().unwrap(); + let a = vec![0; 1024]; let mut b = a.clone(); b[42] = 1; - let counter = AtomicUsize::new(0); - let result = a - .par_iter() - .inspect(|_| { - counter.fetch_add(1, Ordering::SeqCst); - }) - .partial_cmp(&b); - assert!(result == Some(::std::cmp::Ordering::Less)); - assert!(counter.load(Ordering::SeqCst) < a.len()); // should not have visited every single one + pool.install(|| { + let expected = Some(::std::cmp::Ordering::Less); + assert_eq!(a.par_iter().partial_cmp(&b), expected); + + for len in 1..10 { + let counter = AtomicUsize::new(0); + let result = a + .par_iter() + .with_max_len(len) + .inspect(|_| { + counter.fetch_add(1, Ordering::SeqCst); + }) + .partial_cmp(&b); + assert_eq!(result, expected); + // should not have visited every single one + assert!(counter.into_inner() < a.len()); + } + }); } #[test] fn check_partial_cmp_nan_short_circuit() { + // We only use a single thread to make the short-circuit behavior deterministic. + let pool = ThreadPoolBuilder::new().num_threads(1).build().unwrap(); + let a = vec![0.0; 1024]; let mut b = a.clone(); b[42] = f64::NAN; - let counter = AtomicUsize::new(0); - let result = a - .par_iter() - .inspect(|_| { - counter.fetch_add(1, Ordering::SeqCst); - }) - .partial_cmp(&b); - assert!(result == None); - assert!(counter.load(Ordering::SeqCst) < a.len()); // should not have visited every single one + pool.install(|| { + let expected = None; + assert_eq!(a.par_iter().partial_cmp(&b), expected); + + for len in 1..10 { + let counter = AtomicUsize::new(0); + let result = a + .par_iter() + .with_max_len(len) + .inspect(|_| { + counter.fetch_add(1, Ordering::SeqCst); + }) + .partial_cmp(&b); + assert_eq!(result, expected); + // should not have visited every single one + assert!(counter.into_inner() < a.len()); + } + }); } #[test]