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

Use ThinVec more in the AST #104754

Merged
merged 13 commits into from
Feb 21, 2023
Merged

Use ThinVec more in the AST #104754

merged 13 commits into from
Feb 21, 2023

Conversation

nnethercote
Copy link
Contributor

r? @ghost

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 23, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 23, 2022

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 23, 2022
@bors
Copy link
Contributor

bors commented Nov 23, 2022

⌛ Trying commit 26324d59c93e8eb9d08792a57b02990185a45852 with merge 10ea77cebafccd3c9085348ddf5b678008e8a87c...

@bors
Copy link
Contributor

bors commented Nov 23, 2022

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

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (10ea77cebafccd3c9085348ddf5b678008e8a87c): comparison URL.

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

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

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.4% [0.4%, 0.4%] 3
Regressions ❌
(secondary)		 0.8% [0.3%, 1.2%] 14
Improvements ✅
(primary)		 -0.5% [-0.9%, -0.2%] 29
Improvements ✅
(secondary)		 -0.4% [-0.8%, -0.1%] 20
All ❌✅ (primary) -0.4% [-0.9%, 0.4%] 32

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)		 - - 0
Regressions ❌
(secondary)		 2.5% [1.2%, 3.6%] 16
Improvements ✅
(primary)		 -1.3% [-2.1%, -0.5%] 2
Improvements ✅
(secondary)		 -2.1% [-3.7%, -1.2%] 3
All ❌✅ (primary) -1.3% [-2.1%, -0.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)		 - - 0
Improvements ✅
(secondary)		 -4.4% [-4.4%, -4.4%] 1
All ❌✅ (primary) - - 0

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 23, 2022
@nnethercote
Copy link
Contributor Author

Let's try again, see if the bootstrapping improvement persists.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 24, 2022
@bors
Copy link
Contributor

bors commented Nov 24, 2022

⌛ Trying commit 4ce53b1508cf82969ed83a009865aaece57280e8 with merge e027f83413d67db20d22ecd9ad55315b279b09e2...

@bors
Copy link
Contributor

bors commented Nov 25, 2022

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

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e027f83413d67db20d22ecd9ad55315b279b09e2): comparison URL.

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

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

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.5%, 1.2%] 5
Improvements ✅
(primary)		 -0.5% [-1.3%, -0.2%] 77
Improvements ✅
(secondary)		 -0.6% [-1.3%, -0.2%] 38
All ❌✅ (primary) -0.5% [-1.3%, -0.2%] 77

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)		 - - 0
Regressions ❌
(secondary)		 3.2% [1.8%, 6.8%] 18
Improvements ✅
(primary)		 -1.3% [-2.9%, -0.5%] 3
Improvements ✅
(secondary)		 -2.7% [-3.2%, -2.1%] 2
All ❌✅ (primary) -1.3% [-2.9%, -0.5%] 3

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)		 2.4% [2.3%, 2.4%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.2% [-2.2%, -2.2%] 1
All ❌✅ (primary) - - 0

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 25, 2022
@nnethercote
Copy link
Contributor Author

Wow, this time the bootstrap improvement was 7.9 seconds (1.15%). And the instruction count results were good too. I think this is worth going forward.

r? @the8472

the8472
the8472 reviewed Nov 30, 2022
View reviewed changes
Copy link
Member

@the8472 the8472 left a comment

Choose a reason for hiding this comment

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

Overall a fairly mechanical replacement that looks fine. Only two issues in clippy code. Those probably would only become an issue if someone code-generates huge patterns and then runs clippy on them

src/tools/clippy/clippy_lints/src/unnested_or_patterns.rs Outdated Show resolved Hide resolved
src/tools/clippy/clippy_lints/src/unnested_or_patterns.rs Show resolved Hide resolved
compiler/rustc_ast/src/ast.rs Show resolved Hide resolved
src/test/ui/stats/hir-stats.stderr Outdated Show resolved Hide resolved
@nnethercote
Copy link
Contributor Author

I rebased.

@nnethercote
Copy link
Contributor Author

It works!

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 21, 2023
@bors
Copy link
Contributor

bors commented Feb 21, 2023

⌛ Trying commit 9c7570b with merge 501502d1c672413aad29cf862cc45a53ac8c53c8...

@bors
Copy link
Contributor

bors commented Feb 21, 2023

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

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (501502d1c672413aad29cf862cc45a53ac8c53c8): 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.5% [0.4%, 0.8%] 8
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.5% [-1.5%, -0.2%] 76
Improvements ✅
(secondary)		 -0.8% [-3.3%, -0.2%] 42
All ❌✅ (primary) -0.4% [-1.5%, 0.8%] 84

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)		 1.0% [1.0%, 1.0%] 1
Regressions ❌
(secondary)		 3.0% [1.1%, 5.1%] 9
Improvements ✅
(primary)		 -1.1% [-1.2%, -1.1%] 2
Improvements ✅
(secondary)		 -2.8% [-5.2%, -1.2%] 9
All ❌✅ (primary) -0.4% [-1.2%, 1.0%] 3

Cycles

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

@rustbot rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Feb 21, 2023
@nnethercote
Copy link
Contributor Author

Perf results are still good.

@bors r=the8472

@bors
Copy link
Contributor

bors commented Feb 21, 2023

📌 Commit 9c7570b has been approved by the8472

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 Feb 21, 2023
@bors
Copy link
Contributor

bors commented Feb 21, 2023

⌛ Testing commit 9c7570b with merge 3fee48c...

@bors
Copy link
Contributor

bors commented Feb 21, 2023

☀️ Test successful - checks-actions
Approved by: the8472
Pushing 3fee48c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 21, 2023
@bors bors merged commit 3fee48c into rust-lang:master Feb 21, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 21, 2023
This was referenced Feb 21, 2023
Closed
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3fee48c): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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.5% [0.4%, 0.6%] 9
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.5% [-1.5%, -0.2%] 80
Improvements ✅
(secondary)		 -0.8% [-3.3%, -0.2%] 44
All ❌✅ (primary) -0.4% [-1.5%, 0.6%] 89

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)		 - - 0
Regressions ❌
(secondary)		 2.9% [2.2%, 4.1%] 8
Improvements ✅
(primary)		 -0.6% [-0.6%, -0.6%] 1
Improvements ✅
(secondary)		 -3.5% [-9.6%, -1.1%] 14
All ❌✅ (primary) -0.6% [-0.6%, -0.6%] 1

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)		 - - 0
Improvements ✅
(secondary)		 -2.8% [-2.8%, -2.8%] 1
All ❌✅ (primary) - - 0

@nnethercote nnethercote deleted the more-ThinVec-in-ast branch February 21, 2023 21:22
@klensy klensy mentioned this pull request Feb 21, 2023
Closed
@Boshen Boshen mentioned this pull request Mar 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@the8472 the8472 the8472 left review comments

@flip1995 flip1995 flip1995 left review comments

Assignees

@the8472 the8472

Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.69.0
Development

Successfully merging this pull request may close these issues.

None yet