Implement fast path for derive(PartialOrd) when deriving Ord

[makai410]

View all comments

Closes: #155537

Unfortunately, this PR shares the same issue with #124794, which doesn't work when derive(PartialOrd) is before derive(Ord).

[rustbot]

Changes to the code generated for builtin derived traits.

cc @nnethercote

[rustbot]

r? @ShoyuVanilla

rustbot has assigned @ShoyuVanilla.
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: compiler
• compiler expanded to 72 candidates
• Random selection from 18 candidates

[nnethercote]

I can review this: r? @nnethercote

cc @theemathas, who has made good comments on similar PRs in the past.

[makai410]

@bors try @rust-timer queue

[petrochenkov]

We had multiple flags here in the past, and they indeed used flags, and one map in the resolver instead of two containers_deriving_(copy,ord) maps.

View changes since the review

[petrochenkov]

• Remove PartialEq and Eq from the SpecialDerives. #65892

[makai410]

Thanks for providing the context. I was thinking about doing one more round of scanning derives to collect all flags such that it might solve #124794. I don't know if that had been discussed before, and if it did I'd really want to know the reason of why not doing that.

[petrochenkov]

one more round of scanning derives to collect all flags

If I correctly understand what you are talking about, it's a much worse hack and it cannot work correctly.
We already doing something like that to support deprecation lint legacy_derive_helpers, and that logic is going to be removed soon.

[petrochenkov]

Similarly to the existing derive(Clone, Copy) I'd classify this as adding hacks to the expansion infra, and would recommend against this by default, unless there are really strong performance reasons.

Moreover, the Copy hack can be in theory removed in favor of some MIR transform, but for this one it would be harder to do.

[rust-bors]

💔 Test for f5daf9f failed: CI

[makai410]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 803fd19 (803fd197cdeec49615d500f71f89872b51cfc6a9, parent: 30837cb66de032fbc8072c8ecbf76c39c5b14478)

[rust-bors]

💔 Test for 3f43613 failed: CI. Failed job:

• auto - test-various (web logs, enhanced plaintext logs)

[nnethercote]

@bors r+

[rust-bors]

📌 Commit 766428b has been approved by nnethercote

It is now in the queue for this repository.

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

[rust-bors]

☀️ Test successful - CI
Approved by: nnethercote
Duration: 3h 9m 29s
Pushing dd8b2d6 to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 23a3312 (parent) -> dd8b2d6 (this PR)

Test differences

Show 2 test diffs

2 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard dd8b2d6162c5616d7030238c29f892a431093cdd --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. x86_64-gnu-llvm-22-2: 1h -> 1h 36m (+57.6%)
2. x86_64-gnu-llvm-21-2: 1h 36m -> 1h 2m (-34.7%)
3. dist-ohos-armv7: 1h 12m -> 48m 20s (-33.2%)
4. x86_64-gnu-llvm-22-3: 1h 30m -> 1h 56m (+28.9%)
5. x86_64-msvc-ext2: 1h 30m -> 1h 52m (+25.2%)
6. x86_64-mingw-2: 2h -> 2h 29m (+24.9%)
7. dist-x86_64-freebsd: 1h 8m -> 1h 25m (+24.3%)
8. dist-i686-mingw: 2h 41m -> 2h 7m (-20.9%)
9. dist-aarch64-llvm-mingw: 1h 36m -> 1h 49m (+13.7%)
10. x86_64-gnu-llvm-22-1: 1h 8m -> 1h 15m (+9.9%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (dd8b2d6): comparison URL.

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

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

• If the regression was expected or you think it can be justified,
  please write a comment with sufficient written justification, and add
  @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
• If you think that you know of a way to resolve the regression, try to create
  a new PR with a fix for the regression.
• If you do not understand the regression or you think that it is just noise,
  you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
  were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.7%	[0.5%, 0.8%]	3
Regressions ❌ (secondary)	0.7%	[0.2%, 1.0%]	9
Improvements ✅ (primary)	-0.3%	[-0.5%, -0.2%]	20
Improvements ✅ (secondary)	-0.3%	[-0.5%, -0.2%]	5
All ❌✅ (primary)	-0.2%	[-0.5%, 0.8%]	23

Max RSS (memory usage)

Results (primary -2.3%, secondary 7.0%)

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)	7.0%	[5.4%, 8.6%]	2
Improvements ✅ (primary)	-2.3%	[-4.0%, -0.6%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.3%	[-4.0%, -0.6%]	2

Cycles

Results (primary -2.2%, secondary 3.7%)

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)	4.7%	[3.7%, 5.7%]	7
Improvements ✅ (primary)	-2.2%	[-2.2%, -2.2%]	1
Improvements ✅ (secondary)	-3.4%	[-3.4%, -3.4%]	1
All ❌✅ (primary)	-2.2%	[-2.2%, -2.2%]	1

Binary size

Results (primary -0.2%, secondary -0.1%)

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

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.1%, 0.9%]	2
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-0.3%	[-1.0%, -0.0%]	9
Improvements ✅ (secondary)	-0.1%	[-0.4%, -0.0%]	16
All ❌✅ (primary)	-0.2%	[-1.0%, 0.9%]	11

Bootstrap: 511.109s -> 511.311s (0.04%)
Artifact size: 400.56 MiB -> 400.72 MiB (0.04%)

[panstromek]

perf triage:

Improvements mostly outweigh regressions. Results roughly match pre-merge results, so I assume the comments above still hold and this is considered fine.

@rustbot label: +perf-regression-triaged

[petrochenkov]

Using a single hash map instead of two (#155598 (comment)) may help with the derive stress benchmark.

[makai410]

Using a single hash map instead of two (#155598 (comment)) may help with the derive stress benchmark.

Hi @petrochenkov ! I did this in #157675 . I was wondering if you'd be willing to take a look at this ^^

[theemathas]

This requires release notes due to changing user-visible behavior that user code can (incorrectly) depend on, and this has observed breakage in the wild. See #159519.