Add Step::forward/backward_overflowing to enable RangeInclusive loop optimizations

[pitaj]

View all comments

ACP: rust-lang/libs-team#767

This adds new required methods to the Step trait:

trait Step: ... {
    // ... existing functions

    // New required functions
    fn forward_overflowing(start: Self, count: usize) -> (Self, bool);
    fn backward_overflowing(start: Self, count: usize) -> (Self, bool);
}

It was found that using these to implement RangeInclusive's Iterator impl enabled optimizations previously only applicable to the exclusive-ended Range.

This required changing how "exhaustion" works for RangeInclusive. I've nominated this for libs-api discussion because of one insta-stable change:

The new implementations now only set exhausted when overflow occurs, and start is now advanced past end otherwise. I doubt anyone depends on the prior behavior, but it's probably worth a crater run.

The exhaustion changes also affect Debug but my understanding is that debug formatting is never guaranteed stable.

I have now changed the nth impls to use the new functions as well.

r? libs

Closes #45222

[Amanieu]

We discussed this in the @rust-lang/libs-api meeting.

The PartialEq implementation as proposed violates the reflexivity requirement of the Eq trait which states that a == a must always be true. Instead, could equality be computed by ignoring the start field if the range is exhausted? This would treat the combination of start and exhausted as an Option<Idx> for the purposes of comparison.

If the PartialEq implementation can be fixed, we are happy to accept this after a crater run.

[pitaj]

Right, I forgot this implements Eq even though I implemented it manually 🤦‍♂️. exhausted.then_some(self.start) could work though it's less meaningful if the range was exhausted by reverse iteration.

@Amanieu I'm not sure it gains much over just going back to the derive, would that be acceptable?

[Amanieu]

RangeInclusive doesn't support reverse iteration.

The derive is also acceptable. We explicitly document that start has an unspecified value after exhaustion, so it makes sense for equality to also be unspecified.

[pitaj]

RangeInclusive doesn't support reverse iteration.

It implements DoubleEndedIterator but regardless yeah I'll change it back to the derive

[pitaj]

@rustbot ready

[theemathas]

I found a bug with this PR.

fn main() {
    let mut range = 255_u8..=255_u8;
    let _ = range.next();
    println!("{range:?}");
    println!("{}", range.contains(&100_u8));
}

Output of above code on nightly:

255..=255 (exhausted)
false

Output of above code with this PR:

0..=255 (exhausted)
true

(The desired behavior of the RangeBounds methods also need to be figured out.)

[theemathas]

And here's some strange behavior that's possibly a bug:

fn main() {
    let mut range = 0_usize..=0_usize;
    let _ = range.next_back();
    println!("{range:?}");
    let data = [0; 1000];
    println!("{:?}", &data[range]);
}

Output of above code on nightly:

0..=0 (exhausted)
[]

Output of above code with this PR:

0..=18446744073709551615 (exhausted)

thread 'main' (188575) panicked at src/main.rs:6:27:
range end index 18446744073709551615 out of range for slice of length 1000
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[pitaj]

@theemathas

I found a bug with this PR.

Good find on contains, that should be fixed now. Added some tests as well.

And here's some strange behavior that's possibly a bug

This is expected behavior from the new overflowing behavior. Given that the values of start and end have always been unspecified when exhausted, the indexing behavior is also unspecified - so I don't think this is an issue.

(The desired behavior of the RangeBounds methods also need to be figured out.)

I think the end_bound change I just made should resolve this. Now end_bound will return Excluded(start) when exhausted, which ensures (start_bound, end_bound) is always empty. This is fine since the values are unspecified anyways. Excluded(end) no longer works since end is no longer held equal to start on exhaustion.

I also changed into_bounds to panic when converting if exhausted. This matches the behavior of the conversion to the new RangeInclusive type, and is the only viable way of implementing IntoBounds unrestricted with the change. IntoBounds is unstable, so this can still be changed.

Nominating for libs-api approval of those changes

[Amanieu]

This was discussed the @rust-lang/libs-api meeting. We're happy with the end_bound change. Let's get a crater run started.

@bors try

[theemathas]

I made a PR #158770, which only touches the From docs and doctest. It also updates the test in a different way. I think that it would be a good idea to have the From change as a separate change anyway, since it would need to be backported if it merges after the 1.98 beta branch promotion.

[tgross35]

Did you happen to test whether something along the lines of @matthieu-m's proposal at rust-lang/rfcs#3550 (comment) happens to get the desired optimization?

Sorry if this has been discussed already, I haven't been keeping up with this thread but noticed #45222 didn't seem to be linked anywhere.

[pitaj]

@tgross35

That optimization would only be applicable to the new RangeInclusive iterator, whereas this one applies to both legacy and new.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[pitaj]

@tgross35

Did you happen to test whether something along the lines of @matthieu-m's proposal at rust-lang/rfcs#3550 (comment) happens to get the desired optimization?

I've now checked this on Godbolt. The solution provided here optimizes as well or better in all cases I examined.

That optimization would only be applicable to the new RangeInclusive iterator, whereas this one applies to both legacy and new.

Clarifying this: The legacy RangeInclusive API needs to be able to return the bounds by reference, so we cannot apply the enum approach there as it requires storing one or more bounds offset by 1. Therefore, the enum approach can only be applied to the new RangeInclusive iterator. In contrast, the overflowing approach here applies to both legacy and new.

I've now added that this closes #45222, thanks for bringing that to my attention.

[pitaj]

@Mark-Simulacrum you might care more about this range diff which compares from before I added my version of changing the From docs (the point of your last review).

[matthieu-m]

@pitaj Congratulations on solving (hopefully!) the RangeInclusive iteration optimization issue. This issue has been plaguing Rust since its inception, and many folks tried their hand at it, only ever managing partial solutions.

[Mark-Simulacrum]

@bors r+ rollup=never

[rust-bors]

📌 Commit b3c94df has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[jhpratt]

@bors p=6 scheduling

[rust-bors]

☀️ Test successful - CI
Approved by: Mark-Simulacrum
Duration: 3h 13m 8s
Pushing 71c6416 to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 14cae68 (parent) -> 71c6416 (this PR)

Test differences

Show 119 test diffs

Stage 1

• [ui (polonius)] tests/ui/range/step_overflowing.rs: [missing] -> pass (J2)
• [codegen] tests/codegen-llvm/range-iter-loop-opts.rs: [missing] -> pass (J3)
• [ui] tests/ui/range/step_overflowing.rs: [missing] -> pass (J4)

Stage 2

• [ui] tests/ui/range/step_overflowing.rs: [missing] -> pass (J0)
• [codegen] tests/codegen-llvm/range-iter-loop-opts.rs: [missing] -> pass (J1)

Additionally, 114 doctest diffs were found. These are ignored, as they are noisy.

Job group index

• J0: aarch64-apple, aarch64-apple-macos-26, aarch64-gnu, aarch64-gnu-llvm-21-1, aarch64-msvc-1, arm-android, armhf-gnu, dist-i586-gnu-i586-i686-musl, i686-gnu-1, i686-gnu-nopt-1, i686-msvc-1, optional-x86_64-gnu-parallel-frontend, test-various, x86_64-gnu, x86_64-gnu-debug, x86_64-gnu-gcc, x86_64-gnu-llvm-21, x86_64-gnu-llvm-21-2, x86_64-gnu-llvm-22-2, x86_64-gnu-nopt, x86_64-gnu-stable, x86_64-mingw-1, x86_64-msvc-1
• J1: aarch64-apple, aarch64-apple-macos-26, aarch64-gnu, aarch64-gnu-llvm-21-1, aarch64-msvc-1, arm-android, armhf-gnu, dist-i586-gnu-i586-i686-musl, i686-gnu-1, i686-gnu-nopt-1, i686-msvc-1, optional-x86_64-gnu-parallel-frontend, test-various, x86_64-gnu, x86_64-gnu-llvm-21-2, x86_64-gnu-llvm-22-2, x86_64-gnu-nopt, x86_64-gnu-stable, x86_64-mingw-1, x86_64-msvc-1
• J2: x86_64-gnu-next-trait-solver-polonius
• J3: pr-check-2, x86_64-gnu-llvm-21-3, x86_64-gnu-llvm-22-3
• J4: x86_64-gnu-llvm-21-3, x86_64-gnu-llvm-22-3, x86_64-gnu-next-trait-solver-polonius

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 71c64160bd0f471bffdbdf7061fd62bab053d5b4 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-i686-mingw: 1h 40m -> 2h 22m (+41.8%)
2. i686-gnu-2: 1h 44m -> 1h 5m (-38.0%)
3. x86_64-gnu-debug: 1h 36m -> 2h 8m (+33.0%)
4. dist-arm-linux-musl: 1h 38m -> 1h 19m (-19.3%)
5. test-various: 2h 5m -> 1h 41m (-18.8%)
6. x86_64-gnu-llvm-22-1: 1h 11m -> 1h 25m (+18.8%)
7. aarch64-apple-macos-26: 3h 14m -> 2h 39m (-17.9%)
8. pr-check-2: 39m 15s -> 32m 59s (-16.0%)
9. x86_64-gnu-next-trait-solver-polonius: 53m 18s -> 1h 1m (+15.0%)
10. i686-gnu-nopt-1: 2h 27m -> 2h 9m (-12.0%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[tgross35]

@tgross35

Did you happen to test whether something along the lines of @matthieu-m's proposal at rust-lang/rfcs#3550 (comment) happens to get the desired optimization?

I've now checked this on Godbolt. The solution provided here optimizes as well or better in all cases I examined.

To be a bit more clear, my initial concern was about new API surface when we don't strictly need it, given it's possible to fix the new RangeInclusiveIter without it and eventually everything should migrate there. But fixing everything is going to be much better (can't wait to see the perf fun) and considering the trait is (perma-?) unstable anyway, sounds great to me 👍 though mildly amusing that we didn't even need the new range types for this 🙂.

[rust-timer]

Finished benchmarking commit (71c6416): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary 1.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.2%	[1.2%, 1.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[1.2%, 1.2%]	1

Cycles

Results (primary 2.1%, secondary -3.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-3.3%, -3.3%]	1
All ❌✅ (primary)	2.1%	[2.1%, 2.1%]	1

Binary size

Results (primary 0.1%, secondary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.1%]	65
Regressions ❌ (secondary)	0.1%	[0.0%, 0.1%]	23
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	1
All ❌✅ (primary)	0.1%	[0.0%, 0.1%]	65

Bootstrap: 488.761s -> 490.207s (0.30%)
Artifact size: 388.78 MiB -> 388.42 MiB (-0.09%)