Final derive output improvements by nnethercote · Pull Request #99046 · rust-lang/rust (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation22 Commits11 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

nnethercote

With all these changes, the derive output in deriving-all-codegen.stdout is pretty close to optimal, i.e. very similar to what you'd write by hand.

r? @ghost

@rustbot rustbot added the T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

label

Jul 8, 2022

@bors

@bors bors added the S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

label

Jul 8, 2022

@bors

@nnethercote

…est.

It's an interesting case, requiring the use of && in Debug::fmt.

@nnethercote

Because the generatedd code is different to a Copy packed struct.

@nnethercote

Because the generated code is different to fieldless enum with multiple variants.

@nnethercote

It's always ast::Mutability::Not.

@nnethercote

E.g. improving code like this:

match &*self {
    &Enum1::Single { x: ref __self_0 } => {
        ::core::hash::Hash::hash(&*__self_0, state)
    }
}

to this:

match self {
    Enum1::Single { x: __self_0 } => {
        ::core::hash::Hash::hash(&*__self_0, state)
    }
}

by removing the &*, the &, and the ref.

I suspect the current generated code predates deref-coercion.

The commit also gets rid of use_temporaries, instead passing around always_copy, which makes things a little clearer. And it fixes up some comments.

@nnethercote

By producing &T expressions for fields instead of T. This matches what the existing comments (e.g. on FieldInfo) claim is happening, and it's also what most of the trait-specific code needs.

The exception is PartialEq, which needs T expressions for lots of special case error messaging to work. So we now convert the &T back to a T for PartialEq.

@nnethercote

Use tag in names of things referring to tags, instead of the mysterious vi.

Also change arg_N in output to argN, which has the same length as self and so results in nicer vertical alignments.

@nnethercote

@nnethercote

To avoid computing a bunch of stuff that it doesn't need.

@nnethercote

Currently, for the enums and comparison traits we always check the tag for equality before doing anything else. This is a bit clumsy. This commit changes things so that the tags are handled very much like a zeroth field in the enum.

For eq/ne` this makes the code slightly cleaner.

For partial_cmp and cmp it's a more notable change: in the case where the tags aren't equal, instead of having a tag equality check followed by a tag comparison, it just does a single tag comparison.

The commit also improves how Hash works for enums: instead of having duplicated code to hash the tag for every arm within the match, we do it just once before the match.

All this required replacing the EnumNonMatchingCollapsed value with a new EnumTag value.

For fieldless enums the new code is particularly improved. All the code now produced is close to optimal, being very similar to what you'd write by hand.

@nnethercote

@nnethercote

@rust-timer

Awaiting bors try build completion.

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

@bors

⌛ Trying commit 6d114fb with merge 9a42766fc17474d3c9205a0dd5025acbf02de5fe...

@bors

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

@rust-timer

@rust-timer

Finished benchmarking commit (9a42766fc17474d3c9205a0dd5025acbf02de5fe): comparison url.

Instruction count

mean1 max count2
Regressions 😿 (primary) N/A N/A 0
Regressions 😿 (secondary) N/A N/A 0
Improvements 🎉 (primary) -0.6% -1.3% 32
Improvements 🎉 (secondary) -2.2% -4.1% 10
All 😿🎉 (primary) -0.6% -1.3% 32

Max RSS (memory usage)

Results

mean1 max count2
Regressions 😿 (primary) 2.7% 2.9% 2
Regressions 😿 (secondary) 2.7% 3.3% 3
Improvements 🎉 (primary) -0.7% -1.0% 6
Improvements 🎉 (secondary) N/A N/A 0
All 😿🎉 (primary) 0.2% 2.9% 8

Cycles

Results

mean1 max count2
Regressions 😿 (primary) 1.5% 1.5% 1
Regressions 😿 (secondary) 2.8% 3.4% 8
Improvements 🎉 (primary) -2.0% -2.0% 1
Improvements 🎉 (secondary) N/A N/A 0
All 😿🎉 (primary) -0.3% -2.0% 2

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

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3
  2. number of relevant changes ↩ ↩2 ↩3

@nnethercote

Because it's more concise than the let form.

@nnethercote

@scottmcm: I ended up finding a way to use your &{self.x} trick here :)

@Mark-Simulacrum

@bors

📌 Commit 1cb1d63 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors 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.

S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Jul 14, 2022

@bors

@bors

@bors bors mentioned this pull request

Jul 15, 2022

@rust-timer

Finished benchmarking commit (0fe5390): comparison url.

Instruction count

mean1 max count2
Regressions 😿 (primary) 0.5% 0.6% 8
Regressions 😿 (secondary) N/A N/A 0
Improvements 🎉 (primary) -0.6% -0.9% 19
Improvements 🎉 (secondary) -2.8% -3.7% 6
All 😿🎉 (primary) -0.2% -0.9% 27

Max RSS (memory usage)

Results

mean1 max count2
Regressions 😿 (primary) 8.7% 8.7% 1
Regressions 😿 (secondary) N/A N/A 0
Improvements 🎉 (primary) -1.0% -3.2% 8
Improvements 🎉 (secondary) -7.2% -8.7% 2
All 😿🎉 (primary) 0.1% 8.7% 9

Cycles

Results

mean1 max count2
Regressions 😿 (primary) N/A N/A 0
Regressions 😿 (secondary) N/A N/A 0
Improvements 🎉 (primary) -2.1% -2.1% 1
Improvements 🎉 (secondary) N/A N/A 0
All 😿🎉 (primary) -2.1% -2.1% 1

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3
  2. number of relevant changes ↩ ↩2 ↩3

@nnethercote

The post-merge perf results weren't as good as the pre-merge perf results, but there are still clearly more improvements than regressions, so overall I think it's still a win.

@rustbot label: +perf-regression-triaged

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request

Oct 11, 2022

@he32

Pkgsrc changes:

Upstream changes:

Version 1.64.0 (2022-09-22)

Language

Compiler

Libraries

Stabilized APIs

These types were previously stable in std::ffi, but are now also available in core and alloc:

These types were previously stable in std::os::raw, but are now also available in core::ffi and std::ffi:

These APIs are now usable in const contexts:

Cargo

Misc

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

RalfJung

RalfJung

RalfJung

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Nov 15, 2022

@matthiaskrgr

…ackh726

Deriving cleanups

Fixing some minor problems @RalfJung found in rust-lang#99046.

r? @RalfJung

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request

Nov 16, 2022

@he32

Pkgsrc changes:

Upstream changes:

Version 1.64.0 (2022-09-22)

Language

Compiler

Libraries

Stabilized APIs

These types were previously stable in std::ffi, but are now also available in core and alloc:

These types were previously stable in std::os::raw, but are now also available in core::ffi and std::ffi:

These APIs are now usable in const contexts:

Cargo

Misc

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.