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

[scottmcm]

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]

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]

@bors try @rust-timer queue

[rust-bors]

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

[rust-timer]

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%)

[scottmcm]

This doing nothing for primary icount doesn't really surprise me. I doubt it's used that much in the compiler, especially not with tricky types. (For primitives since the llvm.[su]cmp change the .cmp().is_ge() works fine.)

"No action needed" is great.

[JohnTitor]

Ok, makes sense! Marking as iffy as this could cause some potential perf issues and we might want to bisect in the future

@bors r+ rollup=iffy

View changes since this review

[rust-bors]

📌 Commit d34fd62 has been approved by JohnTitor

It is now in the queue for this repository.

[scottmcm]

This will hopefully make #161081 more effective too, using those overridden min/max in more places.