Skip to content

Simplify derive(Debug) output for fieldless enums #106884

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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 21, 2023

Conversation

clubby789
Copy link
Contributor

Fixes #106875

@rustbot
Copy link
Collaborator

rustbot commented Jan 14, 2023

r? @michaelwoerister

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 14, 2023
@clubby789 clubby789 changed the title Simplify derive(Debug) for fieldless enums Simplify derive(Debug) output for fieldless enums Jan 14, 2023
@saethlin
Copy link
Member

This will get queued behind another PR but I'm quite curious
@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 14, 2023
@bors
Copy link
Collaborator

bors commented Jan 14, 2023

⌛ Trying commit 70091926e458fd7ddd3df29b33de3cfdb624c8dc with merge ab530986fdc49d3802aaf309560b6c5e75515da0...

@clubby789 clubby789 force-pushed the fieldless-enum-debug branch from 7009192 to 45143c4 Compare January 14, 2023 23:20
@saethlin
Copy link
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Jan 15, 2023

⌛ Trying commit 45143c4fbec94a8323fefb0dd7a8dd5d54ede0e7 with merge 3dda7981e1a61d170870bacbe64351e30aaa3ac4...

@bors
Copy link
Collaborator

bors commented Jan 15, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3dda7981e1a61d170870bacbe64351e30aaa3ac4): comparison URL.

Overall result: ✅ 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 a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.7% [0.7%, 0.7%] 1
Improvements ✅
(primary)
-0.6% [-1.9%, -0.3%] 30
Improvements ✅
(secondary)
-1.2% [-3.1%, -0.3%] 12
All ❌✅ (primary) -0.6% [-1.9%, -0.3%] 30

Max RSS (memory usage)

Results

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)
2.0% [2.0%, 2.0%] 1
Regressions ❌
(secondary)
5.1% [2.6%, 7.7%] 2
Improvements ✅
(primary)
-0.6% [-0.7%, -0.5%] 3
Improvements ✅
(secondary)
-1.3% [-1.4%, -1.2%] 5
All ❌✅ (primary) 0.1% [-0.7%, 2.0%] 4

Cycles

Results

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
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.2% [-1.2%, -1.2%] 1
Improvements ✅
(secondary)
-1.8% [-2.6%, -1.1%] 5
All ❌✅ (primary) -1.2% [-1.2%, -1.2%] 1

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 15, 2023
@michaelwoerister
Copy link
Member

Thanks for the PR, @clubby789! I'll take a closer look some time this week.

Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PR, @clubby789!

It looks like the changes have the intended effect, but I think we can improve the clarity of the implementation (mostly by renaming and documenting a few things).

@clubby789 clubby789 force-pushed the fieldless-enum-debug branch from 45143c4 to 95a824c Compare January 19, 2023 15:53
@michaelwoerister
Copy link
Member

Thanks, @clubby789!

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 20, 2023

📌 Commit 95a824c has been approved by michaelwoerister

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. labels Jan 20, 2023
@bors
Copy link
Collaborator

bors commented Jan 21, 2023

⌛ Testing commit 95a824c with merge b22aa57...

@bors
Copy link
Collaborator

bors commented Jan 21, 2023

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing b22aa57 to master...

@bors
Copy link
Collaborator

bors commented Jan 21, 2023

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing b22aa57 to master...

@bors bors added merged-by-bors This PR was explicitly merged by bors. labels Jan 21, 2023
@bors bors merged commit b22aa57 into rust-lang:master Jan 21, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 21, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b22aa57): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.8% [0.7%, 1.0%] 7
Improvements ✅
(primary)
-0.5% [-1.7%, -0.2%] 22
Improvements ✅
(secondary)
-1.7% [-2.9%, -0.5%] 8
All ❌✅ (primary) -0.5% [-1.7%, -0.2%] 22

Max RSS (memory usage)

Results

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)
5.5% [5.5%, 5.5%] 1
Regressions ❌
(secondary)
2.4% [2.4%, 2.4%] 1
Improvements ✅
(primary)
-2.9% [-2.9%, -2.9%] 1
Improvements ✅
(secondary)
-1.1% [-1.3%, -1.1%] 3
All ❌✅ (primary) 1.3% [-2.9%, 5.5%] 2

Cycles

Results

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
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.4% [-2.0%, -0.7%] 21
Improvements ✅
(secondary)
-2.9% [-4.7%, -1.5%] 24
All ❌✅ (primary) -1.4% [-2.0%, -0.7%] 21

@rustbot rustbot added the perf-regression Performance regression. label Jan 21, 2023
@therealprof
Copy link
Contributor

@clubby789 Very exciting change, thank you!

@rylev
Copy link
Member

rylev commented Jan 25, 2023

Improvements outweigh the regressions so I think it's fine if we just take this performance as is.

@rustbot label: +perf-regression-triaged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

derive(Debug) on simple enums should emit a single call to Formatter::write_str
8 participants