If I write this code using k_smallest, and ask for usize::MAX items, this returns all the items in the iterator, which makes sense.
use itertools::Itertools;
#[test]
fn k_smallest_memory_is_fine() {
let iter = vec![1].into_iter();
let first = iter.k_smallest(usize::MAX);
itertools::assert_equal(first, vec![1]);
}
I think this is ok because this line only requires memory for an iterator up to size k
However, when swapping to k_smallest_relaxed given usize::MAX / 2 + 1 items, it panics:
use itertools::Itertools;
#[test]
fn k_smallest_memory_does_not_panic() {
let iter = vec![1].into_iter();
let first = iter.k_smallest_relaxed(usize::MAX / 2);
itertools::assert_equal(first, vec![1]);
}
#[test]
#[should_panic]
fn k_smallest_memory_does_panic() {
let iter = vec![1].into_iter();
let first = iter.k_smallest_relaxed(usize::MAX / 2 + 1);
itertools::assert_equal(first, vec![1]);
}
It seems like the problematic line is here where k is multiplied by 2, unchecked, so usize::MAX / 2 + 1 overflows it and causes the panic.
At the very least, this simple fix causes the test above to pass without panicking, since it does a saturating multiply.
diff --git a/src/k_smallest.rs b/src/k_smallest.rs
index 7e4ace2..c28831b 100644
--- a/src/k_smallest.rs
+++ b/src/k_smallest.rs
@@ -99,7 +99,10 @@ where
}
let mut iter = iter.fuse();
- let mut buf = iter.by_ref().take(2 * k).collect::<Vec<_>>();
+ let mut buf = iter
+ .by_ref()
+ .take(2usize.saturating_mul(k))
+ .collect::<Vec<_>>();
if buf.len() < k {
buf.sort_unstable_by(&mut comparator);
Given that the docs for k_smallest_relaxed explicitly call out that you need 2 * k * sizeof(Self::Item) + O(1) memory, I'm wondering if this change is fine though -- since it explicitly uses more memory than k_smallest which is k * sizeof(Self::Item) + O(1).
If I write this code using
k_smallest, and ask forusize::MAXitems, this returns all the items in the iterator, which makes sense.I think this is ok because this line only requires memory for an iterator up to size
kHowever, when swapping to
k_smallest_relaxedgivenusize::MAX / 2 + 1items, it panics:It seems like the problematic line is here where
kis multiplied by 2, unchecked, so usize::MAX / 2 + 1 overflows it and causes the panic.At the very least, this simple fix causes the test above to pass without panicking, since it does a saturating multiply.
Given that the docs for
k_smallest_relaxedexplicitly call out that you need2 * k * sizeof(Self::Item) + O(1)memory, I'm wondering if this change is fine though -- since it explicitly uses more memory thank_smallestwhich isk * sizeof(Self::Item) + O(1).