Don't use match-destructuring for derived ops on structs.

[nnethercote]

r? @scottmcm

[nnethercote]

Please note!

• Best reviewed one commit at a time.
• The first six commits are from Improve some deriving code and add a test #98376, which already have r+ and don't need reviewing. The last four commits build on them.

[nnethercote]

We'll need to wait for #98376 to land and the a rebase before doing a perf run here, otherwise the perf improvements from that PR will be combined with the perf improvements from this PR. But I did local measurements and saw instruction count reductions of up to 17% on the derive stress test and up to 3% on primary benchmarks.

[scottmcm]

(Haven't looked in detail yet, but reading the commit message made me think)

If you need something short that works for packed structs, could you maybe use &{self.x}? I think that still gets the move-then-ref-the-temporary that's needed for soundness -- it certainly doesn't trigger the warning -- but using way fewer tokens and no inventing of new names.

[nnethercote]

could you maybe use &{self.x}

Oh neat! That will let me get rid of all the pattern and let stuff. Thanks for the tip, I'll do it on Monday.

[nnethercote]

We'll need to wait for #98376 to land and the a rebase before doing a perf run here, otherwise the perf improvements from that PR will be combined with the perf improvements from this PR.

Actually, there's no harm in seeing a combined measurement:

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 58cadcd4d5b7e62f77348a6d2b77ebb6d6c162c4 with merge a0fdabf17ad3155433165d33e6446ace9feeb361...

[bors]

☀️ Try build successful - checks-actions
Build commit: a0fdabf17ad3155433165d33e6446ace9feeb361 (a0fdabf17ad3155433165d33e6446ace9feeb361)

[rust-timer]

Queued a0fdabf17ad3155433165d33e6446ace9feeb361 with parent d017d59, future comparison URL.

[rust-timer]

Finished benchmarking commit (a0fdabf17ad3155433165d33e6446ace9feeb361): comparison url.

Instruction count

• Primary benchmarks: mixed results
• Secondary benchmarks: 🎉 relevant improvements found

	mean1	max	count2
Regressions 😿 (primary)	1.7%	2.3%	2
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-1.1%	-3.1%	105
Improvements 🎉 (secondary)	-6.0%	-16.8%	49
All 😿🎉 (primary)	-1.0%	-3.1%	107

Max RSS (memory usage)

Results

• Primary benchmarks: 🎉 relevant improvements found
• Secondary benchmarks: mixed results

	mean1	max	count2
Regressions 😿 (primary)	4.8%	4.8%	1
Regressions 😿 (secondary)	9.9%	9.9%	1
Improvements 🎉 (primary)	-1.2%	-2.3%	11
Improvements 🎉 (secondary)	-4.5%	-6.7%	20
All 😿🎉 (primary)	-0.7%	4.8%	12

Cycles

Results

• Primary benchmarks: mixed results
• Secondary benchmarks: 🎉 relevant improvements found

	mean1	max	count2
Regressions 😿 (primary)	4.7%	4.7%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-2.2%	-3.2%	18
Improvements 🎉 (secondary)	-10.4%	-18.7%	32
All 😿🎉 (primary)	-1.9%	4.7%	19

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[scottmcm]

It's so strange to me that regex opt full gets worse, but regex opt incr-full gets (negligibly) better.

[nnethercote]

It's so strange to me that regex opt full gets worse, but regex opt incr-full gets (negligibly) better.

I have a possible explanation here. That PR hasn't landed yet, hopefully it's effectively the same regression we're seeing, and once #98180 is merged (another precursor to this PR) the regression will disappear here...

[nnethercote]

I tried the &{self.x} approach and it's 99% great but falls down on 1% of cases. Consider this code:

#[derive(Hash)]
struct A;

#[derive(Hash)]
#[repr(packed)]
struct Foo(A);

#[derive(Hash)]
#[repr(packed)]
pub struct Bar<T: Copy>(T);

fn main() {}

Currently, rustc accepts A and Foo but rejects Bar:

error: `#[derive]` can't be used on a `#[repr(packed)]` struct with type or const parameters (error E0133)
 --> f.rs:8:10
  |
8 | #[derive(Hash)]
  |          ^^^^
  |
  = note: `#[deny(unaligned_references)]` on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #82523 <https://github.com/rust-lang/rust/issues/82523>
  = note: this error originates in the derive macro `Hash` (in Nightly builds, run with -Z macro-backtrace for more info)

The error on Bar is triggered by a check done on MIR.

Using &{self.x}, rustc accepts A and Bar but rejects Foo:

error[E0507]: cannot move out of `self.0` which is behind a shared reference
 --> f.rs:6:12
  |
4 | #[derive(Hash)]
  |          ---- in this derive macro expansion
5 | #[repr(packed)]
6 | struct Foo(A);
  |            ^ move occurs because `self.0` has type `A`, which does not implement the `Copy` trait
  |
  = note: this error originates in the derive macro `Hash` (in Nightly builds, run with -Z macro-backtrace for more info)

The error on Foo is because the &{self.x} relies on self.x implementing Copy, which isn't true here. It's also interesting that Bar is accepted -- it shows that error E0133 is possibly stricter than it needs to be.

These are just two examples, there are some other minor variations. src/test/ui/derives/deriving-with-repr-packed.rs is a test demonstrating some of these.

Given all that, I will stick to the well-tested let-pattern approach. Thanks again for the suggestion!

[nnethercote]

The final four commits here are ready for review.

[bors]

☔ The latest upstream changes (presumably #98376) made this pull request unmergeable. Please resolve the merge conflicts.

[nnethercote]

@richkadel This PR changes the code generated for derived code for various built-in traits. It's causing some changes in coverage test output.

I ran x test src/test/run-make-fulldeps/coverage-reports --bless to generate the 4th commit. The changes just seem like minor perturbations -- are they important? I don't understand these tests at all so I can't tell if the changes are reasonable.

[nnethercote]

Also, this is the second time I've hit a change in the coverage test output, and I have numerous more changes to the derive builtins in progress and planned. Is there any documentation I can read on these tests to help me understand any other future changes I might hit? Thanks.

[richkadel]

Also, this is the second time I've hit a change in the coverage test output, and I have numerous more changes to the derive builtins in progress and planned. Is there any documentation I can read on these tests to help me understand any other future changes I might hit? Thanks.

It's very unusual for the coverage to change, but when it does I just try to verify the logic in it. For instance if a coverage changes from 0 to 1 on derived trait that should mean that we're counting the execution of a trait in that derived trait that we weren't counting before, which should mean that it is executing now, and was not executing before. I'm on my mobile device because I'm on vacation, so I may have a hard time seeing the changes completely, but it looks like some counters are lost in your new version, which seems like a problem to me. There can be differences in the ordering of results in reports, especially if you are regenerating the results on Windows. Unix variants, including Linux and Mac OS, are generally very consistent in my experience. If you are dropping coverage counters, that may indicate a problem. I wouldn't expect that to happen. There is some fairly detailed documentation on the rust coverage implementation in the developer guide.

I can take a closer look at this when I return from vacation in a few days.

[richkadel]

Okay this is weird, not necessarily missing information, but it is a little verbose and confusing. It looks like the source code span for the coverage region for Debug(?) I think, has expanded from just the derive attribute line, to now including part of the struct. It makes me wonder if you had more fields in that example, would you have a bunch of overlapping coverage regions, which probably would be a bad thing. Like I said it's hard to read on my cell phone and see what is happening, but I'm not sure that you want the code span to include both the attribute and the fields within the struct. Note these coach bands are not specific to coverage but coverage is able to use them very effectively, in general. I usually do not have to special case them for coverage.

[richkadel]

My concern here is where are all of the counters for the traits other than PartialEq? Overlapping coverage spans may be causing an issue here, but that's a guess.

Note and IMPORTANT strategy of coverage generation in rust is to avoid overlapping coverage spans.

[nnethercote]

Why does the original output have seven ^0/^1 fields here when there are only six derived traits?

[nnethercote]

And are the ^0/^1 fields supposed to line up with the start of the trait name? Some do, some don't.

[richkadel]

I think these are just reordered. If you're generating the new coverage results on Windows, that's a common problem but it's not wrong. Sadly llvm coverage report generation does not have deterministic ordering of results. Generally they are very consistently ordered on Linux specifically, and just as consistently ordered in the opposite way on Windows. I'm not sure if that's the issue here or not.

[nnethercote]

Good news: fixing up a single span in the generated code was enough to revert all the coverage test changes except for the &str/alloc::vec::Vec<i32> ordering one, and it makes sense that this is a trivial change due to the nondeterminism.

[nnethercote]

@bors r=scottmcm

[bors]

📌 Commit ecc6e95 has been approved by scottmcm

[bors]

⌛ Testing commit ecc6e95 with merge d46c728...

[bors]

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing d46c728 to master...

[rust-timer]

Finished benchmarking commit (d46c728): comparison url.

Instruction count

• Primary benchmarks: 🎉 relevant improvements found
• Secondary benchmarks: mixed results

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	1.3%	1.9%	19
Improvements 🎉 (primary)	-0.6%	-1.5%	66
Improvements 🎉 (secondary)	-5.4%	-12.3%	33
All 😿🎉 (primary)	-0.6%	-1.5%	66

Max RSS (memory usage)

Results

• Primary benchmarks: mixed results
• Secondary benchmarks: 🎉 relevant improvements found

	mean1	max	count2
Regressions 😿 (primary)	2.1%	2.1%	1
Regressions 😿 (secondary)	2.1%	2.1%	1
Improvements 🎉 (primary)	-2.3%	-5.6%	3
Improvements 🎉 (secondary)	-3.5%	-5.1%	12
All 😿🎉 (primary)	-1.2%	-5.6%	4

Cycles

Results

• Primary benchmarks: 🎉 relevant improvement found
• Secondary benchmarks: mixed results

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	4.6%	8.3%	10
Improvements 🎉 (primary)	-1.8%	-1.8%	1
Improvements 🎉 (secondary)	-8.3%	-13.7%	14
All 😿🎉 (primary)	-1.8%	-1.8%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[richkadel]

Good news: fixing up a single span in the generated code was enough to revert all the coverage test changes except for the &str/alloc::vec::Vec<i32> ordering one, and it makes sense that this is a trivial change due to the nondeterminism.

That's great news!