Implement all mix/max functions in a (hopefully) more optimization amendable way by WaffleLapkin · Pull Request #136307 · rust-lang/rust

WaffleLapkin · 2025-01-30T16:36:29Z

Previously the graph was like this:

min -> Ord::min -> min_by -> match on compare() (in these cases compare = Ord::cmp)
                                      ^
                                      |
                                 min_by_key

now it looks like this:

min -> Ord::min -> `<=` <- min_by_key

min_by -> `Ordering::is_le` of `compare()`

(max* and minmax* are the exact same, i.e. they also use <= and is_le)

I'm not sure how to test this, but it should probably be easier for the backend to optimize.

r? @scottmcm
cc #115939 (comment)

scottmcm · 2025-01-30T17:06:08Z

 #[stable(feature = "cmp_min_max_by", since = "1.53.0")]
 pub fn max_by_key<T, F: FnMut(&T) -> K, K: Ord>(v1: T, v2: T, mut f: F) -> T {
-    max_by(v1, v2, |v1, v2| f(v1).cmp(&f(v2)))
+    if f(&v1) <= f(&v2) { v2 } else { v1 }


Hmm, my brain always thinks of this as

Suggested change

if f(&v1) <= f(&v2) { v2 } else { v1 }

if f(&v1) > f(&v2) { v1 } else { v2 }

flipping the operation instead of the blocks.

Meh, either way is fine.

scottmcm

I think it's fine to not have a codegen test for this, though if you really wanted to you could probably make one with tuples, since those have lt and friends overridden separately. It'd be fragile, though, since #133984 and the LLVM upgrade are probably going to change our output.

So I think a couple of updated doctests would be nice, then you're good. I have some other comments, but those are all up to your discretion depending whether you think they're worth bothering.

- add tests for `a == b` where missing - try to make all the tests more similar - try to use more illustrative test values

`<` seems to be the "lucky one" for llvm

scottmcm · 2025-01-31T05:20:55Z

+///     fn cmp(&self, other: &Self) -> Ordering { Ordering::Equal }
+/// }
+///
+/// assert_eq!(cmp::min(Equal("v1"), Equal("v2")).0, "v1");


suggestion: also include

Suggested change

/// assert_eq!(cmp::min(Equal("v1"), Equal("v2")).0, "v1");

/// assert_eq!(cmp::min(Equal("v1"), Equal("v2")).0, "v1");

/// assert_eq!(cmp::min(Equal("v2"), Equal("v1")).0, "v2");

for emphasis, because cmp::min("v1", "v2") == "v1" too.

The values were chosen after the function's argument names, this feels confusing...

scottmcm · 2025-01-31T05:23:41Z

+    ///     fn cmp(&self, other: &Self) -> Ordering { Ordering::Equal }
+    /// }
+    ///
+    /// assert_eq!(Equal("self").min(Equal("other")).0, "self");


Ditto here, for emphasis:

Suggested change

/// assert_eq!(Equal("self").min(Equal("other")).0, "self");

/// assert_eq!(Equal("self").min(Equal("other")).0, "self");

/// assert_eq!(Equal("other").min(Equal("self")).0, "other");

(though I guess the values work less well here)

scottmcm · 2025-02-01T08:13:04Z

Ok, this is already better, so my other thoughts aren't blocking. We can always improve the examples more later

@bors r+ rollup

bors · 2025-02-01T08:13:06Z

📌 Commit c5835cd has been approved by scottmcm

It is now in the queue for this repository.

scottmcm · 2025-02-02T21:27:21Z

Out of curiosity, since everything else in the rollup seemed uninteresting but there were some perf changes:
@rust-timer build 2032576

(Not that the results were bad! This might have just actually showed up, which I wouldn't have guessed.)

rust-timer · 2025-02-02T22:46:54Z

Finished benchmarking commit (2032576): comparison URL.

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.1%, 0.1%]	1
Regressions ❌ (secondary)	0.4%	[0.3%, 0.4%]	2
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.4%, 0.1%]	2

Max RSS (memory usage)

Results (primary 0.8%, secondary 2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	4.7%	[3.5%, 6.6%]	3
Regressions ❌ (secondary)	3.2%	[1.4%, 6.3%]	8
Improvements ✅ (primary)	-3.0%	[-4.4%, -1.2%]	3
Improvements ✅ (secondary)	-1.2%	[-1.3%, -1.2%]	2
All ❌✅ (primary)	0.8%	[-4.4%, 6.6%]	6

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (primary -0.0%, secondary -0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.0%, 0.5%]	15
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	18
Improvements ✅ (primary)	-0.1%	[-0.4%, -0.0%]	36
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	3
All ❌✅ (primary)	-0.0%	[-0.4%, 0.5%]	51

Bootstrap: 777.851s -> 778.832s (0.13%)
Artifact size: 328.84 MiB -> 328.84 MiB (0.00%)

rustbot assigned scottmcm Jan 30, 2025

rustbot added 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. labels Jan 30, 2025

scottmcm reviewed Jan 30, 2025

View reviewed changes

Comment thread library/core/src/cmp.rs Outdated

scottmcm reviewed Jan 30, 2025

View reviewed changes

Comment thread library/core/src/cmp.rs Outdated

scottmcm reviewed Jan 30, 2025

View reviewed changes

scottmcm requested changes Jan 30, 2025

View reviewed changes

rustbot 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 Jan 30, 2025

scottmcm reviewed Jan 30, 2025

View reviewed changes

Comment thread library/core/src/cmp.rs Outdated

WaffleLapkin added 2 commits January 31, 2025 05:30

improve doc tests for (min/max/minmax).* functions

b20307b

- add tests for `a == b` where missing - try to make all the tests more similar - try to use more illustrative test values

implement all min/max fns in terms of </is_lt

c5835cd

`<` seems to be the "lucky one" for llvm

WaffleLapkin force-pushed the minminmin branch from f9d2512 to c5835cd Compare January 31, 2025 04:45

scottmcm reviewed Jan 31, 2025

View reviewed changes

bors 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 Feb 1, 2025

matthiaskrgr mentioned this pull request Feb 1, 2025

Rollup of 6 pull requests #136389

Merged

bors merged commit a56e85a into rust-lang:master Feb 1, 2025

rustbot added this to the 1.86.0 milestone Feb 1, 2025

This comment has been minimized.

Sign in to view

WaffleLapkin deleted the minminmin branch February 3, 2025 17:06

miried mentioned this pull request Apr 4, 2025

Fix parameter order for _by() variants of min / max/ minmax in std::cmp #139357

Merged

Vastargazing mentioned this pull request Apr 3, 2026

coretests: add argument order regression tests for min_by/max_by/minmax_by #154761

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Implement all mix/max functions in a (hopefully) more optimization amendable way#136307

Implement all mix/max functions in a (hopefully) more optimization amendable way#136307
bors merged 2 commits into
rust-lang:masterfrom
WaffleLapkin:minminmin

WaffleLapkin commented Jan 30, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

scottmcm Jan 30, 2025

Uh oh!

scottmcm left a comment

Uh oh!

Uh oh!

scottmcm Jan 31, 2025

Uh oh!

WaffleLapkin Jan 31, 2025

Uh oh!

scottmcm Jan 31, 2025

Uh oh!

scottmcm commented Feb 1, 2025

Uh oh!

bors commented Feb 1, 2025

Uh oh!

scottmcm commented Feb 2, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Feb 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	if f(&v1) <= f(&v2) { v2 } else { v1 }
	if f(&v1) > f(&v2) { v1 } else { v2 }

	/// assert_eq!(cmp::min(Equal("v1"), Equal("v2")).0, "v1");
	/// assert_eq!(cmp::min(Equal("v1"), Equal("v2")).0, "v1");
	/// assert_eq!(cmp::min(Equal("v2"), Equal("v1")).0, "v2");

	/// assert_eq!(Equal("self").min(Equal("other")).0, "self");
	/// assert_eq!(Equal("self").min(Equal("other")).0, "self");
	/// assert_eq!(Equal("other").min(Equal("self")).0, "other");

Uh oh!

Uh oh!

Conversation

WaffleLapkin commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottmcm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottmcm commented Feb 1, 2025

Uh oh!

bors commented Feb 1, 2025

Uh oh!

scottmcm commented Feb 2, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Feb 2, 2025

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

WaffleLapkin commented Jan 30, 2025 •

edited

Loading