Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Shrink hir::Ty and hir::Pat #101467

Merged
merged 2 commits into from
Sep 8, 2022
Merged

Shrink hir::Ty and hir::Pat #101467

merged 2 commits into from
Sep 8, 2022

Conversation

nnethercote
Copy link
Contributor

r? @ghost

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Sep 6, 2022
@rustbot
Copy link
Collaborator

rustbot commented Sep 6, 2022

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 6, 2022
@bors
Copy link
Contributor

bors commented Sep 6, 2022

⌛ Trying commit feec8bc1b82627df07ab20c8a8b26116b1cb2168 with merge b9ccbaf07a4c57df630c2d88a9cdbc6ca80b25b9...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Sep 6, 2022

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

@rust-timer
Copy link
Collaborator

Queued b9ccbaf07a4c57df630c2d88a9cdbc6ca80b25b9 with parent b44197a, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b9ccbaf07a4c57df630c2d88a9cdbc6ca80b25b9): 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-review -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.

mean1 range count2
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.7% [-1.6%, -0.2%] 22
Improvements ✅
(secondary)		 -1.3% [-1.8%, -0.9%] 8
All ❌✅ (primary) -0.7% [-1.6%, -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.

mean1 range count2
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.9% [2.9%, 2.9%] 1
Improvements ✅
(primary)		 -0.9% [-1.2%, -0.6%] 2
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.9% [-1.2%, -0.6%] 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.

mean1 range count2
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -2.8% [-2.8%, -2.8%] 1
Improvements ✅
(secondary)		 -3.3% [-3.3%, -3.3%] 1
All ❌✅ (primary) -2.8% [-2.8%, -2.8%] 1

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@rustbot 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 Sep 6, 2022
@nnethercote
Copy link
Contributor Author

There is a lot of noise in the benchmark suite at the moment, but after looking in detail at the cycles and max-rss results I am confident that there is a small but real performance improvement here, and it is worth proceeding.

r? @spastorino

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@spastorino
Copy link
Member

@nnethercote looks good. Saw that you have force pushed some stuff after running perf, unsure if you wanted to run perf again but in one case or another feel free to r=me when you think it's ready.

@nnethercote
Copy link
Contributor Author

Ha ha, this one is ready, thanks!

@bors r=spastorino

@bors
Copy link
Contributor

bors commented Sep 7, 2022

📌 Commit ae1a9c439146c4ac0b3f722ff652ba62d5452687 has been approved by spastorino

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 Sep 7, 2022
@bors
Copy link
Contributor

bors commented Sep 8, 2022

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 8, 2022
@nnethercote
Arena-allocate hir::Lifetime.
4314615 
This shrinks `hir::Ty` from 72 to 48 bytes.

`visit_lifetime` is added to the HIR stats collector because these types
are now stored in memory on their own, instead of being within other
types.
@nnethercote
Introduce DotDotPos.
e67f39f 
This shrinks `hir::Pat` from 88 to 72 bytes.
@nnethercote
Copy link
Contributor Author

Rebased.

@bors r=spastorino

@bors
Copy link
Contributor

bors commented Sep 8, 2022

📌 Commit e67f39f has been approved by spastorino

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 8, 2022
@bors
Copy link
Contributor

bors commented Sep 8, 2022

⌛ Testing commit e67f39f with merge 24d6992...

@bors
Copy link
Contributor

bors commented Sep 8, 2022

☀️ Test successful - checks-actions
Approved by: spastorino
Pushing 24d6992 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 8, 2022
@bors bors merged commit 24d6992 into rust-lang:master Sep 8, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 8, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (24d6992): comparison URL.

Overall result: ❌ regressions - 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.

mean1 range count2
Regressions ❌
(primary)		 0.8% [0.8%, 1.0%] 6
Regressions ❌
(secondary)		 7.5% [7.5%, 7.5%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.8% [0.8%, 1.0%] 6

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.

mean1 range count2
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.3% [2.3%, 2.3%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.5% [-2.5%, -2.5%] 1
All ❌✅ (primary) - - 0

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.

mean1 range count2
Regressions ❌
(primary)		 2.5% [2.5%, 2.5%] 1
Regressions ❌
(secondary)		 3.3% [3.3%, 3.3%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.8% [-4.9%, -2.0%] 5
All ❌✅ (primary) 2.5% [2.5%, 2.5%] 1

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@rustbot rustbot added the perf-regression Performance regression. label Sep 8, 2022
@nnethercote nnethercote deleted the shrink-hir-Ty-Pat branch September 8, 2022 20:39
@nnethercote
Copy link
Contributor Author

The html5ever regressions are noise, otherwise, this is very slightly perf-positive.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 8, 2022
flip1995 pushed a commit to flip1995/rust that referenced this pull request Oct 6, 2022
@bors
Auto merge of rust-lang#101467 - nnethercote:shrink-hir-Ty-Pat, r=spa…
30a5815 
…storino

Shrink `hir::Ty` and `hir::Pat`

r? `@ghost`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@spastorino spastorino

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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.65.0
Development

Successfully merging this pull request may close these issues.
6 participants
@nnethercote @rustbot @rust-timer @bors @rust-log-analyzer @spastorino