Skip to content

More macro expansion optimizations #95259

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
Merged
merged 4 commits into from
Mar 25, 2022
Merged

More macro expansion optimizations #95259

merged 4 commits into from
Mar 25, 2022

Conversation

nnethercote
Copy link
Contributor

A few nice wins for macro-heavy crates.

r? @petrochenkov

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 24, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 24, 2022
@nnethercote
Copy link
Contributor Author

Some check full results on a few crates. Those marked with * are in the rustc-perf suite.

Benchmark & Profile Scenario % Change Significance Factor?
quote-stress check full -7.44% 37.19x
async-std-1.10.0 check full -7.06% 35.30x
time-macros-0.2.3 check full -3.92% 19.62x
yansi-0.5.0 check full -2.38% 11.89x
inotify-0.10.0 check full 0.94% 4.70x
*token-stream-stress check full 0.53% 2.63x
scroll_derive-0.11.0 check full -0.51% 2.56x
ctor-0.1.21 check full -0.51% 2.56x
num-derive-0.3.3 check full -0.47% 2.35x
pest_generator-2.1.3 check full -0.46% 2.28x
futures-macro-0.3.19 check full -0.44% 2.22x
mockall_derive-0.11.0 check full -0.44% 2.20x
*diesel check full -0.44% 2.18x

@nnethercote
Copy link
Contributor Author

There likely won't be much change within the rustc-perf benchmarks, but let's check just in case.

@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 Mar 24, 2022
@bors
Copy link
Collaborator

bors commented Mar 24, 2022

⌛ Trying commit 19cb13688ffeff3e17f4ce1626620f81c1c74b7d with merge d8f7c9a8b9640de025c4eacb58c71dd15db53b74...

@bors
Copy link
Collaborator

bors commented Mar 24, 2022

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

@rust-timer
Copy link
Collaborator

Queued d8f7c9a8b9640de025c4eacb58c71dd15db53b74 with parent 37b55c8, future comparison URL.

@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 24, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d8f7c9a8b9640de025c4eacb58c71dd15db53b74): comparison url.

Summary: This benchmark run shows 5 relevant improvements 🎉 to instruction counts.

  • Arithmetic mean of relevant improvements: -1.3%
  • Largest improvement in instruction counts: -2.0% on incr-unchanged builds of diesel check

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 led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@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 Mar 24, 2022
@petrochenkov
Copy link
Contributor

Nonterminal::NtTT is an implementation detail of rustc_expand::mbe, it can never be encountered by parsing functions in rustc_parse etc, so it doesn't need to be defined in rustc_ast in theory.
It's quite possible that after 460f39f it can be removed entirely.

@petrochenkov
Copy link
Contributor

@bors r+

@slanterns
Copy link
Contributor

bors did not respond.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 24, 2022

📌 Commit 19cb13688ffeff3e17f4ce1626620f81c1c74b7d has been approved by petrochenkov

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

Nonterminal::NtTT is an implementation detail of rustc_expand::mbe, it can never be encountered by parsing functions in rustc_parse etc, so it doesn't need to be defined in rustc_ast in theory. It's quite possible that after 460f39f it can be removed entirely.

Interesting! I will take a look, I have plans to do more here anyway.

@nnethercote
Split NamedMatch::MatchNonterminal in two.
6817442 
The `Lrc` is only relevant within `transcribe()`. There, the `Lrc` is
helpful for the non-`NtTT` cases, because the entire nonterminal is
cloned. But for the `NtTT` cases the inner token tree is cloned (a full
clone) and so the `Lrc` is of no help.

This commit splits the `NtTT` and non-`NtTT` cases, avoiding the useless
`Lrc` in the former case, for the following effect on macro-heavy
crates.
- It reduces the total number of allocations a lot.
- It increases the size of some of the remaining allocations.
- It doesn't affect *peak* memory usage, because the larger allocations
  are short-lived.

This overall gives a speed win.
@nnethercote
Shrink NamedMatchVec to one inline element.
cad5f1e 
This counters the `NamedMatchVec` size increase from the previous
commit, leaving `NamedMatchVec` smaller than before.
@nnethercote
Shrink MatcherPosRepetition.
fdec26d 
Currently it copies a `KleeneOp` and a `Token` out of a
`SequenceRepetition`. It's better to store a reference to the
`SequenceRepetition`, which is now possible due to rust-lang#95159 having changed
the lifetimes.
@nnethercote nnethercote force-pushed the more-macro-expansion-optimizations branch from 19cb136 to fdec26d Compare March 25, 2022 01:37
@nnethercote
Copy link
Contributor Author

I pushed an update to remove an "njn:" comment that I accidentally left behind.

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented Mar 25, 2022

📌 Commit fdec26d has been approved by petrochenkov

@bors
Copy link
Collaborator

bors commented Mar 25, 2022

⌛ Testing commit fdec26d with merge 8a0c550...

@bors
Copy link
Collaborator

bors commented Mar 25, 2022

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 8a0c550 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 25, 2022
@bors bors merged commit 8a0c550 into rust-lang:master Mar 25, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 25, 2022
@nnethercote nnethercote deleted the more-macro-expansion-optimizations branch March 25, 2022 09:22
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8a0c550): comparison url.

Summary: This benchmark run shows 4 relevant improvements 🎉 but 1 relevant regression 😿 to instruction counts.

  • Arithmetic mean of relevant improvements: -1.5%
  • Arithmetic mean of all relevant changes: -1.1%
  • Largest improvement in instruction counts: -2.0% on incr-unchanged builds of diesel check
  • Largest regression in instruction counts: 0.4% on incr-full builds of unicode-normalization-0.1.19 opt

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

@nnethercote
Copy link
Contributor Author

The perf wins clearly outweigh the losses here.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@petrochenkov petrochenkov

Labels
merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.
7 participants
@nnethercote @rust-timer @bors @petrochenkov @slanterns @rust-highfive @rustbot