Skip to content

Commit f19bec8

Browse files
authored
Rollup merge of rust-lang#58122 - matthieu-m:range_incl_perf, r=dtolnay
RangeInclusive internal iteration performance improvement. Specialize `Iterator::try_fold` and `DoubleEndedIterator::try_rfold` to improve code generation in all internal iteration scenarios. This changes brings the performance of internal iteration with `RangeInclusive` on par with the performance of iteration with `Range`: - Single conditional jump in hot loop, - Unrolling and vectorization, - And even Closed Form substitution. Unfortunately, it only applies to internal iteration. Despite various attempts at stream-lining the implementation of `next` and `next_back`, LLVM has stubbornly refused to optimize external iteration appropriately, leaving me with a choice between: - The current implementation, for which Closed Form substitution is performed, but which uses 2 conditional jumps in the hot loop when optimization fail. - An implementation using a `is_done` boolean, which uses 1 conditional jump in the hot loop when optimization fail, allowing unrolling and vectorization, but for which Closed Form substitution fails. In the absence of any conclusive evidence as to which usecase matters most, and with no assurance that the lack of Closed Form substitution is not indicative of other optimizations being foiled, there is no way to pick one implementation over the other, and thus I defer to the statu quo as far as `next` and `next_back` are concerned.
2 parents c49da5b + 4fed67f commit f19bec8

File tree

3 files changed

+81
-6
lines changed

3 files changed

+81
-6
lines changed

src/libcore/iter/range.rs

+58-3
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use convert::TryFrom;
22
use mem;
3-
use ops::{self, Add, Sub};
3+
use ops::{self, Add, Sub, Try};
44
use usize;
55

66
use super::{FusedIterator, TrustedLen};
@@ -368,11 +368,11 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {
368368
Some(Less) => {
369369
self.is_empty = Some(false);
370370
self.start = plus_n.add_one();
371-
return Some(plus_n)
371+
return Some(plus_n);
372372
}
373373
Some(Equal) => {
374374
self.is_empty = Some(true);
375-
return Some(plus_n)
375+
return Some(plus_n);
376376
}
377377
_ => {}
378378
}
@@ -382,6 +382,34 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {
382382
None
383383
}
384384

385+
#[inline]
386+
fn try_fold<B, F, R>(&mut self, init: B, mut f: F) -> R
387+
where
388+
Self: Sized, F: FnMut(B, Self::Item) -> R, R: Try<Ok=B>
389+
{
390+
self.compute_is_empty();
391+
392+
if self.is_empty() {
393+
return Try::from_ok(init);
394+
}
395+
396+
let mut accum = init;
397+
398+
while self.start < self.end {
399+
let n = self.start.add_one();
400+
let n = mem::replace(&mut self.start, n);
401+
accum = f(accum, n)?;
402+
}
403+
404+
self.is_empty = Some(true);
405+
406+
if self.start == self.end {
407+
accum = f(accum, self.start.clone())?;
408+
}
409+
410+
Try::from_ok(accum)
411+
}
412+
385413
#[inline]
386414
fn last(mut self) -> Option<A> {
387415
self.next_back()
@@ -415,6 +443,33 @@ impl<A: Step> DoubleEndedIterator for ops::RangeInclusive<A> {
415443
self.end.clone()
416444
})
417445
}
446+
447+
#[inline]
448+
fn try_rfold<B, F, R>(&mut self, init: B, mut f: F) -> R where
449+
Self: Sized, F: FnMut(B, Self::Item) -> R, R: Try<Ok=B>
450+
{
451+
self.compute_is_empty();
452+
453+
if self.is_empty() {
454+
return Try::from_ok(init);
455+
}
456+
457+
let mut accum = init;
458+
459+
while self.start < self.end {
460+
let n = self.end.sub_one();
461+
let n = mem::replace(&mut self.end, n);
462+
accum = f(accum, n)?;
463+
}
464+
465+
self.is_empty = Some(true);
466+
467+
if self.start == self.end {
468+
accum = f(accum, self.start.clone())?;
469+
}
470+
471+
Try::from_ok(accum)
472+
}
418473
}
419474

420475
#[stable(feature = "fused", since = "1.26.0")]

src/libcore/ops/range.rs

+2
Original file line numberDiff line numberDiff line change
@@ -334,12 +334,14 @@ pub struct RangeInclusive<Idx> {
334334
trait RangeInclusiveEquality: Sized {
335335
fn canonicalized_is_empty(range: &RangeInclusive<Self>) -> bool;
336336
}
337+
337338
impl<T> RangeInclusiveEquality for T {
338339
#[inline]
339340
default fn canonicalized_is_empty(range: &RangeInclusive<Self>) -> bool {
340341
range.is_empty.unwrap_or_default()
341342
}
342343
}
344+
343345
impl<T: PartialOrd> RangeInclusiveEquality for T {
344346
#[inline]
345347
fn canonicalized_is_empty(range: &RangeInclusive<Self>) -> bool {

src/libcore/tests/iter.rs

+21-3
Original file line numberDiff line numberDiff line change
@@ -1741,19 +1741,37 @@ fn test_range_inclusive_folds() {
17411741
assert_eq!((1..=10).sum::<i32>(), 55);
17421742
assert_eq!((1..=10).rev().sum::<i32>(), 55);
17431743

1744-
let mut it = 40..=50;
1744+
let mut it = 44..=50;
17451745
assert_eq!(it.try_fold(0, i8::checked_add), None);
1746-
assert_eq!(it, 44..=50);
1746+
assert_eq!(it, 47..=50);
1747+
assert_eq!(it.try_fold(0, i8::checked_add), None);
1748+
assert_eq!(it, 50..=50);
1749+
assert_eq!(it.try_fold(0, i8::checked_add), Some(50));
1750+
assert!(it.is_empty());
1751+
assert_eq!(it.try_fold(0, i8::checked_add), Some(0));
1752+
assert!(it.is_empty());
1753+
1754+
let mut it = 40..=47;
1755+
assert_eq!(it.try_rfold(0, i8::checked_add), None);
1756+
assert_eq!(it, 40..=44);
17471757
assert_eq!(it.try_rfold(0, i8::checked_add), None);
1748-
assert_eq!(it, 44..=47);
1758+
assert_eq!(it, 40..=41);
1759+
assert_eq!(it.try_rfold(0, i8::checked_add), Some(81));
1760+
assert!(it.is_empty());
1761+
assert_eq!(it.try_rfold(0, i8::checked_add), Some(0));
1762+
assert!(it.is_empty());
17491763

17501764
let mut it = 10..=20;
17511765
assert_eq!(it.try_fold(0, |a,b| Some(a+b)), Some(165));
17521766
assert!(it.is_empty());
1767+
assert_eq!(it.try_fold(0, |a,b| Some(a+b)), Some(0));
1768+
assert!(it.is_empty());
17531769

17541770
let mut it = 10..=20;
17551771
assert_eq!(it.try_rfold(0, |a,b| Some(a+b)), Some(165));
17561772
assert!(it.is_empty());
1773+
assert_eq!(it.try_rfold(0, |a,b| Some(a+b)), Some(0));
1774+
assert!(it.is_empty());
17571775
}
17581776

17591777
#[test]

0 commit comments

Comments
 (0)