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 explicit delimiter token trees from Delimited. #95797

Merged
merged 1 commit into from
Apr 9, 2022
Merged

Remove explicit delimiter token trees from Delimited. #95797

merged 1 commit into from
Apr 9, 2022

Conversation

nnethercote
Copy link
Contributor

They were introduced by the final commit in #95159 and gave a
performance win. But since the introduction of MatcherLoc they are no
longer needed. This commit reverts that change, making the code a bit
simpler.

r? @petrochenkov

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

I'm not expecting any perf changes from this.

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

bors commented Apr 8, 2022

⌛ Trying commit 3cb42b5a1222bbb13366335be6f2ffd42044e11b with merge 95a9b38ef7c10fdc5307cd0b65cd417d573560da...

@nnethercote nnethercote force-pushed the rm-Delimited-all_tts branch from 3cb42b5 to a3b93d7 Compare April 8, 2022 08:05
@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 Apr 8, 2022

⌛ Trying commit a3b93d7ec148616ee9168a2b60795d6741e0396b with merge 2bd823e91fad29a95f3d680ff7a84b46227d7a23...

@bors
Copy link
Contributor

bors commented Apr 8, 2022

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

@rust-timer
Copy link
Collaborator

Queued 2bd823e91fad29a95f3d680ff7a84b46227d7a23 with parent e745b4d, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2bd823e91fad29a95f3d680ff7a84b46227d7a23): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: no relevant changes found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 0 5 0 5
mean2 N/A N/A -0.3% N/A -0.3%
max N/A N/A -0.4% N/A -0.4%

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

I'm not expecting any perf changes from this.

Some very small improvements, so that's nice.

@petrochenkov
Copy link
Contributor

r=me with some unnecessary clones removed #95797 (comment).

@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 8, 2022
@nnethercote
Remove explicit delimiter token trees from Delimited.
7450c4e 
They were introduced by the final commit in rust-lang#95159 and gave a
performance win. But since the introduction of `MatcherLoc` they are no
longer needed. This commit reverts that change, making the code a bit
simpler.
@nnethercote nnethercote force-pushed the rm-Delimited-all_tts branch from a3b93d7 to 7450c4e Compare April 9, 2022 00:14
@nnethercote
Copy link
Contributor Author

I removed the unnecessary clone call.

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Apr 9, 2022

📌 Commit 7450c4e 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 9, 2022
@nnethercote
Copy link
Contributor Author

Perf changes here are small enough to rollup, I think.

@bors rollup=always

@Dylan-DPC Dylan-DPC mentioned this pull request Apr 9, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 9, 2022
@bors
Auto merge of rust-lang#95835 - Dylan-DPC:rollup-l5mf2ad, r=Dylan-DPC
4bb685e 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#90066 (Add new ThinBox type for 1 stack pointer wide heap allocated trait objects)
 - rust-lang#95374 (assert_uninit_valid: ensure we detect at least arrays of uninhabited types)
 - rust-lang#95599 (Strict provenance lints)
 - rust-lang#95751 (Don't report numeric inference ambiguity when we have previous errors)
 - rust-lang#95764 ([macro_metavar_expr] Add tests to ensure the feature requirement)
 - rust-lang#95787 (reword panic vs result section to remove recoverable vs unrecoverable framing)
 - rust-lang#95797 (Remove explicit delimiter token trees from `Delimited`.)
 - rust-lang#95804 (rustdoc: Fix empty doc comment with backline ICE)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 747bd16 into rust-lang:master Apr 9, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 9, 2022
@nnethercote nnethercote deleted the rm-Delimited-all_tts branch April 9, 2022 09:28
@nnethercote nnethercote mentioned this pull request Apr 22, 2022
Open
nnethercote added a commit to nnethercote/rust that referenced this pull request Apr 22, 2022
@nnethercote
Fix macro matching for NoDelim delimited sequences.
fe549f1 
PR rust-lang#95159 unintentionally changed the behaviour of declarative macro
matching for `NoDelim` delimited sequences.
- Before rust-lang#95159, delimiters were implicit in `mbe::Delimited`. When
  doing macro matching, normal delimiters were generated out of thin air
  as necessary, but `NoDelim` delimiters were not. This was done within
  `TokenTree::{get_tt,len}`.
- After rust-lang#95159, the delimiters were explicit. There was an unintentional
  change whereby `NoDelim` delimiters were represented explicitly just
  like normal delimeters.
- rust-lang#95555 introduced a new matcher representation (`MatcherLoc`) and the
  `NoDelim` delimiters were made explicit within it, just like
  `mbe::Delimited`.
- rust-lang#95797 then changed `mbe::Delimited` back to having implicit
  delimiters, but because matching is now being done on `MatcherLoc`,
  the behavioural change persisted.

The fix is simple: remove the explicit `NoDelim` delimiters in the
`MatcherLoc` representation. This gives back the original behaviour.

As for why this took so long to be found: it seems that `NoDelim`
sequences are unusual. It took a macro that generates another macro
(from the `yarte_lexer` crate, found via a crater run) to uncover this.

Fixes rust-lang#96305.
@nnethercote nnethercote mentioned this pull request Apr 22, 2022
Closed
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

@fbstj fbstj fbstj left review comments

Assignees

@petrochenkov petrochenkov

Labels
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.
7 participants
@nnethercote @rust-timer @bors @petrochenkov @fbstj @rust-highfive @rustbot