Improve some deriving code and add a test #98376

nnethercote · 2022-06-22T07:59:10Z

The .stdout test is particularly useful.

r? @petrochenkov

compiler/rustc_builtin_macros/src/deriving/generic/mod.rs

compiler/rustc_builtin_macros/src/deriving/cmp/partial_eq.rs

petrochenkov · 2022-06-22T10:07:40Z

@bors try @rust-timer queue

rust-timer · 2022-06-22T10:07:41Z

Awaiting bors try build completion.

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

bors · 2022-06-22T10:07:50Z

⌛ Trying commit 6dbf740b2dc586a3390b3b15ff6c1d2a8e514544 with merge 85aa84416c76589d4348da14583233ee4de50e8d...

bors · 2022-06-22T13:14:58Z

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

rust-timer · 2022-06-22T13:15:00Z

Queued 85aa84416c76589d4348da14583233ee4de50e8d with parent 3d829a0, future comparison URL.

rust-timer · 2022-06-22T14:33:20Z

Finished benchmarking commit (85aa84416c76589d4348da14583233ee4de50e8d): comparison url.

Instruction count

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	0.7%	1.1%	2
Improvements 🎉 (primary)	-0.8%	-1.8%	65
Improvements 🎉 (secondary)	-5.8%	-11.4%	27
All 😿🎉 (primary)	-0.8%	-1.8%	65

Max RSS (memory usage)

Results

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

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.5%	3.0%	3
Improvements 🎉 (primary)	-1.2%	-1.8%	6
Improvements 🎉 (secondary)	-2.9%	-5.1%	14
All 😿🎉 (primary)	-1.2%	-1.8%	6

Cycles

Results

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	3.0%	3.0%	1
Regressions 😿 (secondary)	2.4%	2.4%	1
Improvements 🎉 (primary)	-2.0%	-3.8%	10
Improvements 🎉 (secondary)	-7.6%	-13.6%	24
All 😿🎉 (primary)	-1.6%	-3.8%	11

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.

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

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

Kobzol · 2022-06-22T19:37:57Z

I wonder how would perf. look like without the last commit? Without the ne change.

nnethercote · 2022-06-22T20:48:26Z

I wonder how would perf. look like without the last commit? Without the ne change.

I haven't measured but I'm confident that the last commit is responsible for 99% the perf wins. The first commit is the only other one that affects the generated code, and it just removes one true && per method, which is a tiny change.

I liked @jyn514's comment on Zulip, about the possibility of behavioural changes with this PR if you derive PartialEq on a type that contains a type that has a buggy hand-written PartialEq impl: "we already document that you have to obey this constraint [that eq and ne are inverses], I don't think it's reasonable to expect us to be bug for bug compatible".

petrochenkov · 2022-06-23T09:55:06Z

@bors r+

bors · 2022-06-23T09:55:07Z

📌 Commit fb58effead00fd320ddf6be51ebbf4779f4e9e9d has been approved by petrochenkov

bors · 2022-06-23T09:55:08Z

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

Currently the generated code for methods like `eq`, `ne`, and `partial_cmp` includes stuff like this: ``` let __self_vi = ::core::intrinsics::discriminant_value(&*self); let __arg_1_vi = ::core::intrinsics::discriminant_value(&*other); if true && __self_vi == __arg_1_vi { ... } ``` This commit removes the unnecessary `true &&`, and makes the generating code a little easier to read in the process. It also fixes some errors in comments.

Because the latter just calls the former. The commit also updates some details in a comment.

lqd · 2022-06-28T16:22:10Z

I think a remnant of PartialEq::ne is still contained within the coverage report in that test, and may require being regenerated (I'm not sure how that's done though).

Do the run-make and run-make-fulldeps tests pass for you locally ?

nnethercote · 2022-06-28T20:53:09Z

I think a remnant of PartialEq::ne is still contained within the coverage report in that test, and may require being regenerated (I'm not sure how that's done though).

Do the run-make and run-make-fulldeps tests pass for you locally ?

They do, both via ./x.py test and by running them directly.

The problem appears to be this:

2022-06-28T13:25:51.5003686Z +++ /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make-fulldeps/coverage-reports/coverage-reports/actual_show_coverage.issue-84561.txt	2022-06-28 13:25:27.256476002 +0000
2022-06-28T13:25:51.5003860Z @@ -2,12 +2,6 @@
2022-06-28T13:25:51.5003962Z      2|       |
2022-06-28T13:25:51.5004183Z      3|       |// expect-exit-status-101
2022-06-28T13:25:51.5004324Z      4|     21|#[derive(PartialEq, Eq)]
2022-06-28T13:25:51.5004492Z -  ------------------
2022-06-28T13:25:51.5005071Z -  | <issue_84561::Foo as core::cmp::PartialEq>::eq:
2022-06-28T13:25:51.5005312Z -  |    4|     21|#[derive(PartialEq, Eq)]
2022-06-28T13:25:51.5005479Z -  ------------------
2022-06-28T13:25:51.5005864Z -  | Unexecuted instantiation: <issue_84561::Foo as core::cmp::PartialEq>::ne
2022-06-28T13:25:51.5006026Z -  ------------------
2022-06-28T13:25:51.5006150Z      5|       |struct Foo(u32);
2022-06-28T13:25:51.5006266Z      6|      1|fn test3() {
2022-06-28T13:25:51.5006462Z      7|      1|    let is_true = std::env::args().len() == 1;
2022-06-28T13:25:51.5006686Z ------------------------------------------

I don't understand these coverage tests at all, so I'm not sure if just updating the expected output to remove those lines is reasonable...

nnethercote · 2022-06-28T20:56:07Z

Oh, it seems the coverage-reports test is ignored when I run it locally, bleh.

lqd · 2022-06-28T21:04:58Z

I don't understand these coverage tests at all, so I'm not sure if just updating the expected output to remove those lines is reasonable...

Neither do I, but at least seeing an "unexecuted instantiation of PartialEq::ne" made me think the generated ne wasn't called nor covered before, even if the test calls assert_ne!.

And that seemed to match the idea that it was a safe function to delete indeed.

Mark-Simulacrum · 2022-06-28T22:21:44Z

I think this PR should have a T-libs-api FCP on it, since it does modify stable behavior. It's likely okay to change that behavior - we document that eq and ne should match - but I could see this breaking users if they're writing custom impls and so I think it merits additional consideration.

At minimum, marking relnotes and will edit the description with a short note for why.

nnethercote · 2022-06-28T22:50:40Z

I'm going to remove the last commit from this PR, which is the one that removes the ne derive. Reason being that the earlier commits (especially the first one) are blocking my other work on derives (#98446 and other stuff I haven't yet created PRs for).

I will then file a new PR just for the ne removal, and we can scrutinize that separately.

nnethercote · 2022-06-28T23:03:08Z

I removed the final commit. I'll file the new PR for the ne removal once this PR is merged.

@bors r=petrochenkov

bors · 2022-06-28T23:03:10Z

📌 Commit 02d2cdf has been approved by petrochenkov

bors · 2022-06-29T00:21:06Z

⌛ Testing commit 02d2cdf with merge 126e3df...

bors · 2022-06-29T03:01:44Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 126e3df to master...

nnethercote · 2022-06-29T04:16:10Z

I'll file the new PR for the ne removal once this PR is merged.

That is #98655.

rust-timer · 2022-06-29T04:39:08Z

Finished benchmarking commit (126e3df): comparison url.

Instruction count

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

Max RSS (memory usage)

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: 😿 relevant regression found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	3.3%	3.3%	1
Improvements 🎉 (primary)	-5.1%	-5.1%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-5.1%	-5.1%	1

Cycles

Results

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

	mean¹	max	count²
Regressions 😿 (primary)	2.6%	2.6%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-2.4%	-2.4%	1
Improvements 🎉 (secondary)	-2.8%	-2.8%	1
All 😿🎉 (primary)	0.1%	2.6%	2

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

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 22, 2022

rust-highfive assigned petrochenkov Jun 22, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 22, 2022

petrochenkov reviewed Jun 22, 2022

View reviewed changes

compiler/rustc_builtin_macros/src/deriving/generic/mod.rs Show resolved Hide resolved

compiler/rustc_builtin_macros/src/deriving/cmp/partial_eq.rs Outdated Show resolved Hide resolved

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 22, 2022

petrochenkov 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 Jun 22, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 22, 2022

petrochenkov removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 22, 2022

nnethercote force-pushed the improve-derive-PartialEq branch from 6dbf740 to fb58eff Compare June 23, 2022 01:11

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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 23, 2022

This was referenced Jun 24, 2022

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

Merged

[Experiment] [WIP] Add clone_from to clone derive #98445

Closed

nnethercote added 3 commits June 27, 2022 07:58

Add a test checking the output of builtin derives.

ea74997

Merge build_enum_match_tuple into expand_enum_method_body.

e739668

Because the latter just calls the former. The commit also updates some details in a comment.

nnethercote force-pushed the improve-derive-PartialEq branch from 4e5ceba to 02d2cdf Compare June 28, 2022 23:00

nnethercote changed the title ~~Improve derive(PartialEq)~~ Improve some deriving code and add a test Jun 28, 2022

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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 28, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 29, 2022

bors merged commit 126e3df into rust-lang:master Jun 29, 2022

rustbot added this to the 1.64.0 milestone Jun 29, 2022

bors mentioned this pull request Jun 29, 2022

clone_from derive macro impl #97528

Closed

nnethercote deleted the improve-derive-PartialEq branch June 29, 2022 04:14

nnethercote mentioned this pull request Jun 29, 2022

Don't derive PartialEq::ne. #98655

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve some deriving code and add a test #98376

Improve some deriving code and add a test #98376

nnethercote commented Jun 22, 2022 •

edited

Loading

petrochenkov commented Jun 22, 2022

rust-timer commented Jun 22, 2022

bors commented Jun 22, 2022

bors commented Jun 22, 2022

rust-timer commented Jun 22, 2022

rust-timer commented Jun 22, 2022

Kobzol commented Jun 22, 2022

nnethercote commented Jun 22, 2022

petrochenkov commented Jun 23, 2022

bors commented Jun 23, 2022

bors commented Jun 23, 2022

lqd commented Jun 28, 2022 •

edited

Loading

nnethercote commented Jun 28, 2022

nnethercote commented Jun 28, 2022

lqd commented Jun 28, 2022 •

edited

Loading

Mark-Simulacrum commented Jun 28, 2022

nnethercote commented Jun 28, 2022

nnethercote commented Jun 28, 2022

bors commented Jun 28, 2022

bors commented Jun 29, 2022

bors commented Jun 29, 2022

nnethercote commented Jun 29, 2022

rust-timer commented Jun 29, 2022

Improve some deriving code and add a test #98376

Improve some deriving code and add a test #98376

Conversation

nnethercote commented Jun 22, 2022 • edited Loading

petrochenkov commented Jun 22, 2022

rust-timer commented Jun 22, 2022

bors commented Jun 22, 2022

bors commented Jun 22, 2022

rust-timer commented Jun 22, 2022

rust-timer commented Jun 22, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Kobzol commented Jun 22, 2022

nnethercote commented Jun 22, 2022

petrochenkov commented Jun 23, 2022

bors commented Jun 23, 2022

bors commented Jun 23, 2022

lqd commented Jun 28, 2022 • edited Loading

nnethercote commented Jun 28, 2022

nnethercote commented Jun 28, 2022

lqd commented Jun 28, 2022 • edited Loading

Mark-Simulacrum commented Jun 28, 2022

nnethercote commented Jun 28, 2022

nnethercote commented Jun 28, 2022

bors commented Jun 28, 2022

bors commented Jun 29, 2022

bors commented Jun 29, 2022

nnethercote commented Jun 29, 2022

rust-timer commented Jun 29, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

nnethercote commented Jun 22, 2022 •

edited

Loading

lqd commented Jun 28, 2022 •

edited

Loading

lqd commented Jun 28, 2022 •

edited

Loading