Skip to content

Iterator::{min,max}(_by_key) should use overridden min/max/lt - #160203

Open
scottmcm wants to merge 1 commit into
rust-lang:mainfrom
scottmcm:cmp-with-key
Open

Iterator::{min,max}(_by_key) should use overridden min/max/lt#160203
scottmcm wants to merge 1 commit into
rust-lang:mainfrom
scottmcm:cmp-with-key

Conversation

@scottmcm

Copy link
Copy Markdown
Member

Two related changes to provided Iterator implementations:

  • Iterator::min and Iterator::max are currently implemented via min_by and max_by, which means they don't use Ord::{min,max} despite those being overridable to do something more efficient. Move these to just being .reduce(Ord::min) and .reduce(Ord::max) to take advantage of potential overrides.
  • Iterator::min_by_key and Iterator::max_by_key are implemented by mapping to a tuple then using min_by/max_by with a comparator that only looks at one field in the tuple. That means they end up doing things like a.cmp(b).is_le(), which is wasteful if there an overloaded -> bool method it could use instead. So rephrase these two to work as .map(…).min()/.map(…).max() by mapping to a type that's not a tuple and which can thus override more things instead of just passing a Fn(…) -> Ordering.

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

rustbot commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

r? @JohnTitor

rustbot has assigned @JohnTitor.
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: libs
  • libs expanded to 12 candidates
  • Random selection from 7 candidates

@JohnTitor

Copy link
Copy Markdown
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 31, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Jul 31, 2026
`Iterator::{min,max}(_by_key)` should use overridden `min`/`max`/`lt`
@rust-bors

rust-bors Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: e9abde2 (e9abde2ca8e6335565f6eba3891707e77b0c5247)
Base parent: 922325b (922325bb13bfea5b41454318563f2a65e83c2336)

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (e9abde2): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@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)
0.2% [0.1%, 0.5%] 5
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.1% [-0.1%, -0.1%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 2.1%, secondary 0.9%)

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

mean range count
Regressions ❌
(primary)
2.1% [0.5%, 3.8%] 3
Regressions ❌
(secondary)
2.9% [1.0%, 6.2%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-5.4%, -0.4%] 2
All ❌✅ (primary) 2.1% [0.5%, 3.8%] 3

Cycles

Results (primary 0.1%, secondary -0.9%)

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

mean range count
Regressions ❌
(primary)
0.9% [0.4%, 1.7%] 6
Regressions ❌
(secondary)
1.1% [0.4%, 3.2%] 10
Improvements ✅
(primary)
-1.4% [-2.6%, -0.7%] 3
Improvements ✅
(secondary)
-5.0% [-8.6%, -0.5%] 5
All ❌✅ (primary) 0.1% [-2.6%, 1.7%] 9

Binary size

Results (primary -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.1%, 0.1%] 6
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.1% [-0.8%, -0.0%] 14
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.0% [-0.8%, 0.1%] 20

Bootstrap: 490.333s -> 489.5s (-0.17%)
Artifact size: 392.58 MiB -> 390.57 MiB (-0.51%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants