Introduce `TtParser` #95159

nnethercote · 2022-03-21T00:20:19Z

These commits make a number of changes to declarative macro expansion, resulting in code that is shorter, simpler, and faster.

Best reviewed one commit at a time.

r? @petrochenkov

It currently has no state, just the three methods `parse_tt`, `parse_tt_inner`, and `bb_items_ambiguity_error`. This commit is large but trivial, and mostly consists of changes to the indentation of those methods. Subsequent commits will do more.

Because it involves `next_items` as well as `bb_items`.

Instead of passing it into `parse_tt`.

Doc comments cannot appear in a matcher.

This type was a small performance win for `html5ever`, which uses a macro with hundreds of very simple rules that don't contain any metavariables. But this type is complicated (extra lifetimes) and perf-neutral for macros that do have metavariables. This commit removes `MatcherPosHandle`, simplifying things a lot. This increases the allocation rate for `html5ever` and similar cases a bit, but makes things easier for follow-up changes that will improve performance more than what we lost here.

By putting them in `TtParser`, we can reuse them for every rule in a macro. With that done, they can be `SmallVec` instead of `Vec`, and this is a performance win because these vectors are hot and `SmallVec` operations are a bit slower due to always needing an "inline or heap?" check.

nnethercote · 2022-03-21T00:24:17Z

Here are some instruction count results for the cargo check full scenario. The ones marked with * are not in the rustc-perf suite.

Benchmark & Profile	Scenario	% Change	Significance Factor?
*async-std-1.10.0 check	full	-11.54%	57.69x
*quote-stress check	full	-9.81%	49.06x
*time-macros-0.2.3 check	full	-9.25%	46.25x
*yansi-0.5.0 check	full	-8.23%	41.13x
*inotify-0.10.0 check	full	-3.32%	16.59x
deep-vector check	full	-3.21%	16.05x
*num-derive-0.3.3 check	full	-2.64%	13.21x
*pest_generator-2.1.3 check	full	-2.51%	12.56x
*scroll_derive-0.11.0 check	full	-2.50%	12.48x
*ctor-0.1.21 check	full	-2.38%	11.88x
*mockall_derive-0.11.0 check	full	-2.27%	11.35x
*futures-macro-0.3.19 check	full	-2.22%	11.09x
diesel check	full	-0.89%	4.46x
html5ever check	full	-0.73%	3.66x
tokio-webpush-simple check	full	-0.37%	1.85x
syn check	full	-0.36%	1.81x
token-stream-stress check	full	0.44%	2.19x

compiler/rustc_expand/src/mbe/macro_parser.rs

compiler/rustc_expand/src/mbe.rs

compiler/rustc_expand/src/mbe/macro_parser.rs

compiler/rustc_expand/src/mbe.rs

compiler/rustc_expand/src/mbe/macro_parser.rs

compiler/rustc_expand/src/mbe.rs

nnethercote · 2022-03-21T23:56:28Z

Thank you for the fast review. I have added a commit that addresses your comments.

nnethercote · 2022-03-21T23:57:46Z

@bors try @rust-timer queue

rust-timer · 2022-03-21T23:57:47Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-03-21T23:57:55Z

⌛ Trying commit 84960a1d972494a28fd3af4c57612afd1548eb42 with merge ca34b004599cc8dd6871c7ba9bb1d3ddaa73ed2f...

bors · 2022-03-22T01:32:32Z

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

rust-timer · 2022-03-22T01:32:34Z

Queued ca34b004599cc8dd6871c7ba9bb1d3ddaa73ed2f with parent 3c17c84, future comparison URL.

rust-timer · 2022-03-22T02:54:10Z

Finished benchmarking commit (ca34b004599cc8dd6871c7ba9bb1d3ddaa73ed2f): comparison url.

Summary: This benchmark run shows 25 relevant improvements 🎉 but 7 relevant regressions 😿 to instruction counts.

Arithmetic mean of relevant regressions: 0.4%
Arithmetic mean of relevant improvements: -2.0%
Arithmetic mean of all relevant changes: -1.5%
Largest improvement in instruction counts: -6.3% on incr-unchanged builds of deep-vector check
Largest regression in instruction counts: 0.6% on incr-unchanged builds of inflate 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.

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

nnethercote · 2022-03-22T03:40:53Z

Some nice performance wins here, and a handful of tiny regressions that are probably just noise.

@rustbot label: +perf-regression-triaged

petrochenkov · 2022-03-22T16:38:21Z

r=me after squashing the review commit into the previous relevant commits (or at least into the "Eliminate TokenTreeOrTokenTreeSlice" commit)

bors · 2022-03-22T21:47:00Z

⌛ Testing commit 31df680 with merge a4a5e79...

bors · 2022-03-23T00:27:38Z

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

rust-timer · 2022-03-23T01:44:06Z

Finished benchmarking commit (a4a5e79): comparison url.

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

Arithmetic mean of relevant regressions: 0.3%
Arithmetic mean of relevant improvements: -1.9%
Arithmetic mean of all relevant changes: -1.7%
Largest improvement in instruction counts: -6.2% on incr-unchanged builds of deep-vector check

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

@rustbot label: -perf-regression

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.

lqd · 2022-03-25T11:30:37Z

I think this caused the unexpected side effect seen in #95267, apparently this used to compile:

macro_rules! f {
    (
        /// ab
    ) => {};
}

fn main() {
    f!();
}

Fixes rust-lang#95267. Reverts to the old behaviour before rust-lang#95159 introduced a regression.

…acros, r=petrochenkov Ignore doc comments in a declarative macro matcher. Fixes rust-lang#95267. Reverts to the old behaviour before rust-lang#95159 introduced a regression. r? `@petrochenkov`

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.

…=petrochenkov Remove explicit delimiter token trees from `Delimited`. 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. r? `@petrochenkov`

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.

Remove box syntax from rustc_mir_dataflow and rustc_mir_transform Continuation of rust-lang#87781, inspired by rust-lang#97239. The usages that this PR removes have not appeared from nothing, instead the usage in `rustc_mir_dataflow` and `rustc_mir_transform` was from rust-lang#80522 which split up `rustc_mir`, and which was filed before I filed rust-lang#87781, so it was using the state from before my PR. But it was merged after my PR was merged, so the `box_syntax` uses were able to survive here. Outside of this introduction due to the code being outside of the master branch at the point of merging of my PR, there was only one other introduction of box syntax, in rust-lang#95159. That box syntax was removed again though in rust-lang#95555. Outside of that, `box_syntax` has not made its reoccurrance in compiler crates.

nnethercote added 6 commits March 19, 2022 07:47

Introduce TtParser.

d21b4f3

It currently has no state, just the three methods `parse_tt`, `parse_tt_inner`, and `bb_items_ambiguity_error`. This commit is large but trivial, and mostly consists of changes to the indentation of those methods. Subsequent commits will do more.

Rename bb_items_ambiguity_error as ambiguity_error.

354bd10

Because it involves `next_items` as well as `bb_items`.

Add TtParser::macro_name.

39810a8

Instead of passing it into `parse_tt`.

Remove an impossible code path.

10644e0

Doc comments cannot appear in a matcher.

rust-highfive assigned petrochenkov Mar 21, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 21, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2022

petrochenkov reviewed Mar 21, 2022

View reviewed changes

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 Mar 21, 2022

nnethercote mentioned this pull request Mar 21, 2022

[1/2] Implement macro meta-variable expressions #94368

Merged

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 21, 2022

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 Mar 22, 2022

rustbot added the perf-regression-triaged The performance regression has been triaged. label Mar 22, 2022

petrochenkov removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 22, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 23, 2022

bors merged commit a4a5e79 into rust-lang:master Mar 23, 2022

rustbot added this to the 1.61.0 milestone Mar 23, 2022

bors mentioned this pull request Mar 23, 2022

Use niche-filling optimization even when multiple variants have data. #94075

Merged

rustbot removed the perf-regression Performance regression. label Mar 23, 2022

nnethercote deleted the TtParser branch March 23, 2022 06:43

lqd mentioned this pull request Mar 25, 2022

Failed to compile ring under nightly Rust (1.61.0, 9f4dc0b4d 2022-03-23) #95267

Closed

nnethercote added a commit to nnethercote/rust that referenced this pull request Mar 27, 2022

Ignore doc comments in a declarative macro matcher.

9967594

Fixes rust-lang#95267. Reverts to the old behaviour before rust-lang#95159 introduced a regression.

nnethercote mentioned this pull request Mar 27, 2022

Ignore doc comments in a declarative macro matcher. #95390

Merged

nnethercote mentioned this pull request Apr 8, 2022

Remove explicit delimiter token trees from Delimited. #95797

Merged

nnethercote mentioned this pull request Apr 22, 2022

yarte_lexer 0.0.1 compilation regression #96305

Open

nnethercote mentioned this pull request Apr 22, 2022

Fix macro matching for NoDelim delimited sequences. #96307

Closed

ascjones mentioned this pull request May 16, 2022

Invisible delimiters quoted for Group #97076

Closed

est31 mentioned this pull request May 22, 2022

Remove box syntax from rustc_mir_dataflow and rustc_mir_transform #97281

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `TtParser` #95159

Introduce `TtParser` #95159

nnethercote commented Mar 21, 2022

nnethercote commented Mar 21, 2022

nnethercote commented Mar 21, 2022

nnethercote commented Mar 21, 2022

rust-timer commented Mar 21, 2022

bors commented Mar 21, 2022

bors commented Mar 22, 2022

rust-timer commented Mar 22, 2022

rust-timer commented Mar 22, 2022

nnethercote commented Mar 22, 2022

petrochenkov commented Mar 22, 2022

bors commented Mar 22, 2022

bors commented Mar 23, 2022

rust-timer commented Mar 23, 2022

lqd commented Mar 25, 2022

Introduce TtParser #95159

Introduce TtParser #95159

Conversation

nnethercote commented Mar 21, 2022

nnethercote commented Mar 21, 2022

nnethercote commented Mar 21, 2022

nnethercote commented Mar 21, 2022

rust-timer commented Mar 21, 2022

bors commented Mar 21, 2022

bors commented Mar 22, 2022

rust-timer commented Mar 22, 2022

rust-timer commented Mar 22, 2022

nnethercote commented Mar 22, 2022

petrochenkov commented Mar 22, 2022

bors commented Mar 22, 2022

bors commented Mar 23, 2022

rust-timer commented Mar 23, 2022

lqd commented Mar 25, 2022

Introduce `TtParser` #95159

Introduce `TtParser` #95159