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

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)

library/core/src/cmp.rs

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

library/core/src/cmp.rs

@@ -1535,7 +1529,7 @@ pub fn max_by<T, F: FnOnce(&T, &T) -> Ordering>(v1: T, v2: T, compare: F) -> T {
 #[must_use]
 #[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.

library/core/src/cmp.rs

- 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

library/core/src/cmp.rs

+///     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

library/core/src/cmp.rs

+    ///     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.

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#130514 (Implement MIR lowering for unsafe binders) - rust-lang#135684 (docs: Documented Send and Sync requirements for Mutex + MutexGuard) - rust-lang#136307 (Implement all mix/max functions in a (hopefully) more optimization amendable way) - rust-lang#136360 (Stabilize `once_wait`) - rust-lang#136364 (document that ptr cmp is unsigned) - rust-lang#136374 (Add link attribute for Enzyme's LLVMRust FFI) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#136307 - WaffleLapkin:minminmin, r=scottmcm Implement all mix/max functions in a (hopefully) more optimization amendable way 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 rust-lang#115939 (comment)

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

library/core/src/cmp.rs Outdated Show resolved Hide resolved

scottmcm reviewed Jan 30, 2025

View reviewed changes

library/core/src/cmp.rs Outdated Show resolved Hide resolved

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

library/core/src/cmp.rs Outdated Show resolved Hide resolved

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
6 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

WaffleLapkin commented Jan 30, 2025 •

edited

Loading

scottmcm Jan 30, 2025

scottmcm left a comment

scottmcm Jan 31, 2025

WaffleLapkin Jan 31, 2025

scottmcm Jan 31, 2025

scottmcm commented Feb 1, 2025

bors commented Feb 1, 2025

scottmcm commented Feb 2, 2025

This comment has been minimized.

rust-timer commented Feb 2, 2025

	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");

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

Conversation

WaffleLapkin commented Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

scottmcm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm commented Feb 1, 2025

bors commented Feb 1, 2025

scottmcm commented Feb 2, 2025

This comment has been minimized.

rust-timer commented Feb 2, 2025

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

WaffleLapkin commented Jan 30, 2025 •

edited

Loading