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

Remove <mbe::TokenTree as Clone> #95928

Merged
merged 5 commits into from
Apr 14, 2022
Merged

Remove <mbe::TokenTree as Clone> #95928

merged 5 commits into from
Apr 14, 2022

Conversation

nnethercote
Copy link
Contributor

mbe::TokenTree doesn't really need to implement Clone, and getting rid of that impl leads to some speed-ups.

r? @petrochenkov

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

Some cargo check instruction count results for various crates.

quote-stress check full -3.94% 19.72x
tt-muncher check full -3.06% 15.28x
async-std-1.10.0 check full -1.86% 9.30x
yansi-0.5.0 check full -1.40% 7.02x
num-derive-0.3.3 check full -0.69% 3.46x
stdweb-derive-0.5.3 check full -0.67% 3.33x
pest_generator-2.1.3 check full -0.65% 3.27x
vsdb_derive-0.21.1 check full -0.65% 3.26x
scroll_derive-0.11.0 check full -0.65% 3.26x
clap_derive-3.1.7 check full -0.61% 3.07x
wasm-bindgen-backend-0.2.79 check full -0.60% 2.99x
ctor-0.1.21 check full -0.64% 3.18x
serde_derive-1.0.136 check full -0.59% 2.95x
mockall_derive-0.11.0 check full -0.59% 2.95x
futures-macro-0.3.19 check full -0.58% 2.88x
wayland-scanner-0.29.4 check full -0.58% 2.91x
tonic-build-0.7.0 check full -0.58% 2.92x
prost-derive-0.10.0 check full -0.58% 2.90x
enum-as-inner-0.4.0 check full -0.55% 2.73x
diesel_derives-1.4.1 check full -0.51% 2.57x
pyo3-macros-backend-0.16.3 check full -0.51% 2.57x
ref-cast-impl-1.0.6 check full -0.47% 2.36x
funty-2.0.0 check full -0.45% 2.27x
enumflags2_derive-0.7.4 check full -0.44% 2.18x
inotify-0.10.0 check full -0.39% 1.96x
time-macros-0.2.3 check full -0.35% 1.73x

@nnethercote
Copy link
Contributor Author

I'm not entirely happy with the fourth commit, especially TtHandle::get(), but I can't see how to make it better.

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

bors commented Apr 11, 2022

⌛ Trying commit 538e5ee0b1397787546cfdf17d22f355ec2fa0fe with merge bb8700c7c23a09b7b629028065c25b92089b359f...

@bors
Copy link
Contributor

bors commented Apr 11, 2022

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

@rust-timer
Copy link
Collaborator

Queued bb8700c7c23a09b7b629028065c25b92089b359f with parent 48a9e10, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bb8700c7c23a09b7b629028065c25b92089b359f): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 0 40 14 40
mean2 N/A N/A -0.5% -0.4% -0.5%
max N/A N/A -0.8% -0.8% -0.8%

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.

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

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 11, 2022
@petrochenkov petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 12, 2022
@nnethercote
Copy link
Contributor Author

I have added two new commits to address review comments.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 13, 2022
@petrochenkov
Copy link
Contributor

r=me after addressing #95928 (comment) and squashing review commits back into main commits.

@petrochenkov petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 13, 2022
@nnethercote
Introduce TtHandle and use it in TokenSet.
75fd391 
This removes the last use of `<mbe::TokenTree as Clone>`. It also
removes two trivial methods on `Delimited`.
@nnethercote
mbe::TokenTree: remove Lrc around Delimited and `SequenceRepeti…
dd9028a 
…tion`.
@nnethercote
Copy link
Contributor Author

I have addressed the comments and squashed the commits.

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Apr 13, 2022

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

bors commented Apr 14, 2022

⌛ Testing commit dd9028a with merge f9d4d12...

@bors
Copy link
Contributor

bors commented Apr 14, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 14, 2022
@bors bors merged commit f9d4d12 into rust-lang:master Apr 14, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 14, 2022
@bors bors mentioned this pull request Apr 14, 2022
Merged
@nnethercote nnethercote deleted the rm-TokenTree-Clone branch April 14, 2022 09:59
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f9d4d12): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 2 25 14 25
mean2 N/A 0.3% -0.5% -2.6% -0.5%
max N/A 0.3% -0.7% -4.8% -0.7%

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

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@pnkfelix pnkfelix mentioned this pull request Apr 20, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

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
Milestone
1.62.0
Development

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