Rework `smallest_range_containing` to handle duplicates by scottmcm · Pull Request #160198 · rust-lang/rust

scottmcm · 2026-07-30T07:43:36Z

@theemathas pointed out that this method I added in #159509 is implicitly assuming that there are no duplicates in the input. That's not a problem for its one use today -- an enum whose layout matters can't have duplicate discriminants -- but it could be a sharp edge in future, so this PR reworks it to handle duplicate values fine.

As a bonus, as I tried a couple different approaches (from just asserting to deduping to more) I found this rephrasing that I think is clearer. In particular, while the .iter().copied().cycle().skip(1) I'd written works, it's definitely not something that you look at and think "oh, obviously". I think this version using .array_windows::<2>() is easier to follow and splitting the wraparound and non-wraparound cases also simplifies the min_by_key lambda.

No changes to any layouts from this -- it just refactors this function.

rustbot · 2026-07-30T07:43:41Z

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: codegen, compiler
codegen, compiler expanded to 74 candidates
Random selection from 17 candidates

nnethercote

The new code makes sense. Some observations from someone who doesn't know anything about enum representation stuff...

WrappingRange is a really weird type.
smallest_range_containing is a really weird operation on a really weird type.
The docs for smallest_range_containing contain only one case where the WrappingRange actually wraps, which seems low because that's trickier than the non-wrapping case.

View changes since this review

nnethercote · 2026-07-30T09:20:34Z

r=me if you want it, after considering the comment above.

scottmcm · 2026-08-05T08:02:53Z

Thanks, that note about testing is a good one. I stopped cramming everything into the doc comment and made some #[test]s, including doing some thinking about more cases that weren't interesting for this implementation but could catch out other "what if it just _____" thoughts.

Since I didn't change the implementation itself from what was previously reviewed, though,
@bors r=nnethercote rollup

rust-bors · 2026-08-05T08:02:56Z

📌 Commit 5b9efe2 has been approved by nnethercote

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 10. This pull request will be tested once the tree is reopened.

Reason for tree closure: manually handling queue due to backlog

@theemathas

…ining, r=nnethercote Rework `smallest_range_containing` to handle duplicates @theemathas [pointed out](rust-lang#159438 (comment)) that this method I added in rust-lang#159509 is implicitly assuming that there are no duplicates in the input. That's not a problem for its one use today -- an enum whose layout matters can't have duplicate discriminants -- but it could be a sharp edge in future, so this PR reworks it to handle duplicate values fine. As a bonus, as I tried a couple different approaches (from just asserting to deduping to more) I found this rephrasing that I think is clearer. In particular, while the `.iter().copied().cycle().skip(1)` I'd written *works*, it's definitely not something that you look at and think "oh, obviously". I think this version using `.array_windows::<2>()` is easier to follow and splitting the wraparound and non-wraparound cases also simplifies the `min_by_key` lambda. No changes to any layouts from this -- it just refactors this function.

…uwer Rollup of 22 pull requests Successful merges: - #158147 (std: fix stack buffer overflow in Windows junction_point) - #159784 (Hint that memchr returns an in-bounds index) - #160130 (Select cache values to verify by key fingerprint, not value fingerprint) - #160343 (Rename `OutlivesPredicate` to `OutlivesClause`) - #160360 (Remove rustc_middle dependency on rustc_hir_pretty) - #160387 (rustc_codegen_ssa: Correctly apply the static `--jobs-backend` limit to backend parallelism) - #160422 (move mir-opt miri tests to CI logic) - #160444 (Avoid resolving path keywords outside `TypeNS`) - #160510 (Resolver: (un)tracked borrows for `CmRefCell` made safe my unsafe speculative flag) - #155424 ([blocked] Link to proposed LLM policy in CONTRIBUTING and pull request template) - #158726 (std: move futex implementations into sys::sync::futex) - #159225 (Split IncrCompSession out of Session) - #159820 (Make the `rustc_unsafe_specialization_marker` attribute actually `unsafe`) - #160198 (Rework `smallest_range_containing` to handle duplicates) - #160362 (Split `SpannedTypeVisitor` into its own crate, `rustc_ty_walk`) - #160390 (autodiff: Handle slice-tailed DSTs in type trees) - #160420 (Suggest `cast_signed()` for overflowing signed integer literals) - #160520 (Add some tests for specialization) - #160522 (fix(bootstrap): Normalize the names of proc macro dependency crates) - #160531 (docs: fix typo in AllowExprMetavar comment) - #160538 (Update expect messages in tcp.rs doc examples to follow the style guide) - #160548 (bootstrap: Register `coverage-map` and `coverage-run` aliases via a separate step)

@theemathas

…ining, r=nnethercote Rework `smallest_range_containing` to handle duplicates @theemathas [pointed out](rust-lang#159438 (comment)) that this method I added in rust-lang#159509 is implicitly assuming that there are no duplicates in the input. That's not a problem for its one use today -- an enum whose layout matters can't have duplicate discriminants -- but it could be a sharp edge in future, so this PR reworks it to handle duplicate values fine. As a bonus, as I tried a couple different approaches (from just asserting to deduping to more) I found this rephrasing that I think is clearer. In particular, while the `.iter().copied().cycle().skip(1)` I'd written *works*, it's definitely not something that you look at and think "oh, obviously". I think this version using `.array_windows::<2>()` is easier to follow and splitting the wraparound and non-wraparound cases also simplifies the `min_by_key` lambda. No changes to any layouts from this -- it just refactors this function.

…uwer Rollup of 25 pull requests Successful merges: - #154585 (treat no_mangle_generic_items as hard error instead of lint warning) - #158147 (std: fix stack buffer overflow in Windows junction_point) - #160130 (Select cache values to verify by key fingerprint, not value fingerprint) - #160343 (Rename `OutlivesPredicate` to `OutlivesClause`) - #160360 (Remove rustc_middle dependency on rustc_hir_pretty) - #160387 (rustc_codegen_ssa: Correctly apply the static `--jobs-backend` limit to backend parallelism) - #160422 (move mir-opt miri tests to CI logic) - #160444 (Avoid resolving path keywords outside `TypeNS`) - #160510 (Resolver: (un)tracked borrows for `CmRefCell` made safe my unsafe speculative flag) - #155424 ([blocked] Link to proposed LLM policy in CONTRIBUTING and pull request template) - #158726 (std: move futex implementations into sys::sync::futex) - #158762 (Emit thumb code on VEX V5) - #159225 (Split IncrCompSession out of Session) - #159820 (Make the `rustc_unsafe_specialization_marker` attribute actually `unsafe`) - #160198 (Rework `smallest_range_containing` to handle duplicates) - #160362 (Split `SpannedTypeVisitor` into its own crate, `rustc_ty_walk`) - #160390 (autodiff: Handle slice-tailed DSTs in type trees) - #160420 (Suggest `cast_signed()` for overflowing signed integer literals) - #160516 (Add regression test for HRTB projection in closure) - #160520 (Add some tests for specialization) - #160522 (fix(bootstrap): Normalize the names of proc macro dependency crates) - #160523 (Add regression test for opaque type) - #160531 (docs: fix typo in AllowExprMetavar comment) - #160538 (Update expect messages in tcp.rs doc examples to follow the style guide) - #160548 (bootstrap: Register `coverage-map` and `coverage-run` aliases via a separate step)

…uwer Rollup of 25 pull requests Successful merges: - #158147 (std: fix stack buffer overflow in Windows junction_point) - #160130 (Select cache values to verify by key fingerprint, not value fingerprint) - #160343 (Rename `OutlivesPredicate` to `OutlivesClause`) - #160360 (Remove rustc_middle dependency on rustc_hir_pretty) - #160387 (rustc_codegen_ssa: Correctly apply the static `--jobs-backend` limit to backend parallelism) - #160422 (move mir-opt miri tests to CI logic) - #160444 (Avoid resolving path keywords outside `TypeNS`) - #160510 (Resolver: (un)tracked borrows for `CmRefCell` made safe by unsafe speculative flag) - #155424 ([blocked] Link to proposed LLM policy in CONTRIBUTING and pull request template) - #158726 (std: move futex implementations into sys::sync::futex) - #158762 (Emit thumb code on VEX V5) - #159225 (Split IncrCompSession out of Session) - #159820 (Make the `rustc_unsafe_specialization_marker` attribute actually `unsafe`) - #160198 (Rework `smallest_range_containing` to handle duplicates) - #160362 (Split `SpannedTypeVisitor` into its own crate, `rustc_ty_walk`) - #160390 (autodiff: Handle slice-tailed DSTs in type trees) - #160420 (Suggest `cast_signed()` for overflowing signed integer literals) - #160501 (Add bootstrap CLI snapshot test for testing miri) - #160516 (Add regression test for HRTB projection in closure) - #160520 (Add some tests for specialization) - #160522 (fix(bootstrap): Normalize the names of proc macro dependency crates) - #160523 (Add regression test for opaque type) - #160531 (docs: fix typo in AllowExprMetavar comment) - #160538 (Update expect messages in tcp.rs doc examples to follow the style guide) - #160548 (bootstrap: Register `coverage-map` and `coverage-run` aliases via a separate step)

@theemathas

Rollup merge of #160198 - scottmcm:redo-smallest_range_containing, r=nnethercote Rework `smallest_range_containing` to handle duplicates @theemathas [pointed out](#159438 (comment)) that this method I added in #159509 is implicitly assuming that there are no duplicates in the input. That's not a problem for its one use today -- an enum whose layout matters can't have duplicate discriminants -- but it could be a sharp edge in future, so this PR reworks it to handle duplicate values fine. As a bonus, as I tried a couple different approaches (from just asserting to deduping to more) I found this rephrasing that I think is clearer. In particular, while the `.iter().copied().cycle().skip(1)` I'd written *works*, it's definitely not something that you look at and think "oh, obviously". I think this version using `.array_windows::<2>()` is easier to follow and splitting the wraparound and non-wraparound cases also simplifies the `min_by_key` lambda. No changes to any layouts from this -- it just refactors this function.

JonathanBrouwer · 2026-08-06T20:15:37Z

@rust-timer build df08f9f

rust-timer · 2026-08-07T03:37:15Z

Finished benchmarking commit (df08f9f): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.9%	[1.4%, 2.5%]	12
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.6%, -0.2%]	10
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 24.9%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	24.9%	[1.0%, 30.9%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary 11.8%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	17.4%	[4.9%, 31.2%]	14
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-7.6%	[-14.4%, -2.2%]	4
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%, 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.0%	[0.0%, 0.1%]	25
Regressions ❌ (secondary)	0.0%	[0.0%, 0.1%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.1%]	25

Bootstrap: 454.551s -> 455.67s (0.25%)
Artifact size: 399.93 MiB -> 399.12 MiB (-0.20%)

scottmcm · 2026-08-07T04:13:34Z

Having serde-1.0.219-threads4 get both a +2.5% icount and -45% wall-time is wild, since I would have said this didn't change anything (other than maybe some edge cases around ties) O_o

JonathanBrouwer · 2026-08-07T08:00:57Z

All PRs in the rollup are getting this identical regression, this is most likely a false positive

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 30, 2026

rustbot assigned nnethercote Jul 30, 2026

nnethercote approved these changes Jul 30, 2026

View reviewed changes

scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 30, 2026

Rework smallest_range_containing to handle duplicates

5b9efe2

scottmcm force-pushed the redo-smallest_range_containing branch from 28f46eb to 5b9efe2 Compare August 5, 2026 06:28

rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 5, 2026

JonathanBrouwer mentioned this pull request Aug 5, 2026

Rollup of 22 pull requests #160554

Closed

JonathanBrouwer mentioned this pull request Aug 5, 2026

Rollup of 25 pull requests #160557

Closed

JonathanBrouwer mentioned this pull request Aug 5, 2026

Rollup of 25 pull requests #160559

Merged

rust-bors Bot merged commit 16e49b1 into rust-lang:main Aug 5, 2026
13 checks passed

rustbot added this to the 1.99.0 milestone Aug 5, 2026

scottmcm deleted the redo-smallest_range_containing branch August 5, 2026 20:13

This comment has been minimized.

Sign in to view

rustbot added the perf-regression Performance regression. label Aug 7, 2026

scottmcm mentioned this pull request Aug 7, 2026

Usually avoid the sort in WrappingRange::smallest_range_containing #160674

Open

1 task

Navigation Menu

Uh oh!

Uh oh!

Rework `smallest_range_containing` to handle duplicates - #160198

Rework `smallest_range_containing` to handle duplicates#160198
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
scottmcm:redo-smallest_range_containing

scottmcm commented Jul 30, 2026

Uh oh!

rustbot commented Jul 30, 2026

Uh oh!

nnethercote left a comment •

edited by rustbot

Loading

Uh oh!

nnethercote commented Jul 30, 2026

Uh oh!

scottmcm commented Aug 5, 2026

Uh oh!

rust-bors Bot commented Aug 5, 2026

Uh oh!

Uh oh!

JonathanBrouwer commented Aug 6, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Aug 7, 2026

Uh oh!

scottmcm commented Aug 7, 2026

Uh oh!

JonathanBrouwer commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Uh oh!

Conversation

scottmcm commented Jul 30, 2026

Uh oh!

rustbot commented Jul 30, 2026

Uh oh!

nnethercote left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnethercote commented Jul 30, 2026

Uh oh!

scottmcm commented Aug 5, 2026

Uh oh!

rust-bors Bot commented Aug 5, 2026

Uh oh!

Uh oh!

JonathanBrouwer commented Aug 6, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Aug 7, 2026

Overall result: ❌✅ regressions and improvements - please read:

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

scottmcm commented Aug 7, 2026

Uh oh!

JonathanBrouwer commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nnethercote left a comment •

edited by rustbot

Loading