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.

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

Shrink ast::Attribute. #100441

Merged
merged 1 commit into from
Aug 16, 2022
Merged

Conversation

nnethercote
Copy link
Contributor

r? @ghost

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 12, 2022
@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 Aug 12, 2022
@bors
Copy link
Contributor

bors commented Aug 12, 2022

⌛ Trying commit 0d1133901da5599ad9f955041c4855ecc460de65 with merge bad7d1c65f1cb84b8cfebde01942c43777b59160...

@rust-log-analyzer

This comment has been minimized.

@nnethercote nnethercote force-pushed the shrink-ast-Attribute branch from 0d11339 to a69032b Compare August 12, 2022 07:05
@rustbot
Copy link
Collaborator

rustbot commented Aug 12, 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

@bors
Copy link
Contributor

bors commented Aug 12, 2022

⌛ Trying commit a69032bdeb2941eeb468fb6f9db72ba6e0a73c1e with merge b15be6b3ef71de4bedb1f1eabeb93191f85dbec3...

@rust-log-analyzer

This comment has been minimized.

@nnethercote nnethercote force-pushed the shrink-ast-Attribute branch from a69032b to c1f0e36 Compare August 12, 2022 07:20
@rustbot
Copy link
Collaborator

rustbot commented Aug 12, 2022

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@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

@bors
Copy link
Contributor

bors commented Aug 12, 2022

⌛ Trying commit c1f0e3653fc959786724c92645d51f43491ec858 with merge 4c7727694361ebbb11914ea478f7ef88149135bb...

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 12, 2022

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

@rust-timer
Copy link
Collaborator

Queued 4c7727694361ebbb11914ea478f7ef88149135bb with parent e2b52ff, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4c7727694361ebbb11914ea478f7ef88149135bb): comparison url.

Instruction count

  • Primary benchmarks: ✅ relevant improvements found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions ❌
(primary)
0.9% 0.9% 2
Regressions ❌
(secondary)
0.2% 0.3% 9
Improvements ✅
(primary)
-0.6% -1.6% 20
Improvements ✅
(secondary)
-1.0% -1.5% 22
All ❌✅ (primary) -0.5% -1.6% 22

Max RSS (memory usage)

Results
  • Primary benchmarks: ✅ relevant improvements found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.2% 4.1% 2
Improvements ✅
(primary)
-3.5% -5.8% 27
Improvements ✅
(secondary)
-4.5% -6.0% 30
All ❌✅ (primary) -3.5% -5.8% 27

Cycles

Results
  • Primary benchmarks: ❌ relevant regressions found
  • Secondary benchmarks: ❌ relevant regression found
mean1 max count2
Regressions ❌
(primary)
2.2% 2.5% 2
Regressions ❌
(secondary)
2.2% 2.2% 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.2% 2.5% 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.

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

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot added perf-regression Performance regression. 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 Aug 12, 2022
@nnethercote
Copy link
Contributor Author

Surprisingly good results on doc profiles, especially for max-rss. I don't know why doc profiles would be more sensitive to AST changes than other profilers. doc builds often use a lot more memory, too, e.g. max-rss (last two columns show the actual max-rss numbers):

Benchmark Profile Scenario % Change Significance Factor? #99464 #100441
helloworld doc full -5.80% 5.59x 244,436 230,252
helloworld check full 0.03% 0.02x 84,708 84,732

cc @jyn514

@jyn514
Copy link
Member

jyn514 commented Aug 12, 2022

I don't know why doc profiles would be more sensitive to AST changes than other profilers. doc builds often use a lot more memory, too, e.g. max-rss (last two columns show the actual max-rss numbers):

that doesn't surprise me too much - everything in https://doc.rust-lang.org/nightly/nightly-rustc/rustdoc/clean/types/index.html is basically a wrapper around rustc_ast or rustc_hir, and we duplicate a lot of stuff in DocContext that's already in TyCtxt.

Glad to see this improve the performance so much!

@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 Aug 16, 2022
@nnethercote
Copy link
Contributor Author

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Aug 16, 2022

📌 Commit b0e6d97087b1735ac94f071f815f85922bc33137 has been approved by petrochenkov

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 Aug 16, 2022
@nnethercote
Copy link
Contributor Author

@bors r-

@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 Aug 16, 2022
@nnethercote nnethercote force-pushed the shrink-ast-Attribute branch from b0e6d97 to 85a6cd6 Compare August 16, 2022 01:12
@nnethercote
Copy link
Contributor Author

Third time lucky:

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Aug 16, 2022

📌 Commit 85a6cd6 has been approved by petrochenkov

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

bors commented Aug 16, 2022

⌛ Testing commit 85a6cd6 with merge 14a459b...

@klensy
Copy link
Contributor

klensy commented Aug 16, 2022

There some ast trait that implemented for some structs with tokens. Should it be implemented for NormalAttr?

macro_rules! impl_has_tokens {
($($T:ty),+ $(,)?) => {
$(
impl HasTokens for $T {
fn tokens(&self) -> Option<&LazyTokenStream> {
self.tokens.as_ref()
}
fn tokens_mut(&mut self) -> Option<&mut Option<LazyTokenStream>> {
Some(&mut self.tokens)
}
}
)+
};
}
macro_rules! impl_has_tokens_none {
($($T:ty),+ $(,)?) => {
$(
impl HasTokens for $T {
fn tokens(&self) -> Option<&LazyTokenStream> {
None
}
fn tokens_mut(&mut self) -> Option<&mut Option<LazyTokenStream>> {
None
}
}
)+
};
}
impl_has_tokens!(AssocItem, AttrItem, Block, Expr, ForeignItem, Item, Pat, Path, Ty, Visibility);
impl_has_tokens_none!(Arm, ExprField, FieldDef, GenericParam, Param, PatField, Variant);

@nnethercote
Copy link
Contributor Author

There some ast trait that implemented for some structs with tokens. Should it be implemented for NormalAttr?

It is covered here:

impl HasTokens for Attribute {
fn tokens(&self) -> Option<&LazyTokenStream> {
match &self.kind {
AttrKind::Normal(_, tokens) => tokens.as_ref(),
kind @ AttrKind::DocComment(..) => {
panic!("Called tokens on doc comment attr {:?}", kind)
}
}
}
fn tokens_mut(&mut self) -> Option<&mut Option<LazyTokenStream>> {
Some(match &mut self.kind {
AttrKind::Normal(_, tokens) => tokens,
kind @ AttrKind::DocComment(..) => {
panic!("Called tokens_mut on doc comment attr {:?}", kind)
}
})
}
}

@bors
Copy link
Contributor

bors commented Aug 16, 2022

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 14a459b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 16, 2022
@bors bors merged commit 14a459b into rust-lang:master Aug 16, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 16, 2022
@nnethercote nnethercote deleted the shrink-ast-Attribute branch August 16, 2022 11:48
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (14a459b): comparison url.

Instruction count

  • Primary benchmarks: ✅ relevant improvements found
  • Secondary benchmarks: ✅ relevant improvements found
( ) mean1 max count2
Regressions ❌
(primary)
0.6% 0.6% 1
Regressions ❌
(secondary)
0.2% 0.2% 1
Improvements ✅
(primary)
-0.6% -1.4% 22
Improvements ✅
(secondary)
-1.0% -1.5% 23
All ❌✅ (primary) -0.5% -1.4% 23

Max RSS (memory usage)

Results
  • Primary benchmarks: ✅ relevant improvements found
  • Secondary benchmarks: ✅ relevant improvements found
( ) mean1 max count2
Regressions ❌
(primary)
2.7% 2.7% 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.8% -6.2% 24
Improvements ✅
(secondary)
-4.9% -6.5% 27
All ❌✅ (primary) -3.6% -6.2% 25

Cycles

Results
  • Primary benchmarks: ✅ relevant improvements found
  • Secondary benchmarks: no relevant changes found
( ) mean1 max count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.7% -3.5% 5
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.7% -3.5% 5

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

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

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

8 participants