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

Remove TokenStreamBuilder #102692

Merged
merged 3 commits into from
Oct 12, 2022
Merged

Conversation

nnethercote
Copy link
Contributor

TokenStreamBuilder is used to combine multiple token streams. It can be removed, leaving the code a little simpler and a little faster.

r? @Aaron1011

`TokenTree::Punct` is handled outside the `match`. This commits moves it
inside the `match`, avoiding the need for the `return`s and making it
easier to read.
`TokenStreamBuilder` exists to concatenate multiple `TokenStream`s
together. This commit removes it, and moves the concatenation
functionality directly into `TokenStream`, via two new methods
`push_tree` and `push_stream`. This makes things both simpler and
faster.

`push_tree` is particularly important. `TokenStreamBuilder` only had a
single `push` method, which pushed a stream. But in practice most of the
time we push a single token tree rather than a stream, and `push_tree`
avoids the need to build a token stream with a single entry (which
requires two allocations, one for the `Lrc` and one for the `Vec`).

The main `push_tree` use arises from a change to one of the `ToInternal`
impls in `proc_macro_server.rs`. It now returns a `SmallVec` instead of
a `TokenStream`. This return value is then iterated over by
`concat_trees`, which does `push_tree` on each element. Furthermore, the
use of `SmallVec` avoids more allocations, because there is always only
one or two token trees.

Note: the removed `TokenStreamBuilder::push` method had some code to
deal with a quadratic blowup case from rust-lang#57735. This commit removes the
code. I tried and failed to reproduce the blowup from that PR, before
and after this change. Various other changes have happened to
`TokenStreamBuilder` in the meantime, so I suspect the original problem
is no longer relevant, though I don't have proof of this. Generally
speaking, repeatedly extending a `Vec` without pre-determining its
capacity is *not* quadratic. It's also incredibly common, within rustc
and many other Rust programs, so if there were performance problems
there you'd think it would show up in other places, too.
The return type can only appear once.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 5, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 5, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 5, 2022
@nnethercote
Copy link
Contributor Author

Best reviewed one commit at a time.

cc @pnkfelix Because of the parts relating to #57735.
cc @mystor Because it touches some proc macro stuff.
cc @petrochenkov Because it involves tokens.

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

bors commented Oct 5, 2022

⌛ Trying commit ed6f481 with merge 168fa6991448154472709aac01ef2a0051741518...

@bors
Copy link
Contributor

bors commented Oct 5, 2022

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

@rust-timer
Copy link
Collaborator

Queued 168fa6991448154472709aac01ef2a0051741518 with parent d4846f9, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (168fa6991448154472709aac01ef2a0051741518): comparison URL.

Overall result: ✅ improvements - no action needed

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)
-0.5% [-0.7%, -0.4%] 4
Improvements ✅
(secondary)
-4.1% [-9.5%, -1.1%] 9
All ❌✅ (primary) -0.5% [-0.7%, -0.4%] 4

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.5% [2.3%, 6.1%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-4.3%, -2.0%] 4
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-5.7% [-5.8%, -5.4%] 3
All ❌✅ (primary) - - 0

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 5, 2022
@Aaron1011
Copy link
Member

r=me, assuming that there are no objections from any of the other mentioned reviewers.

@nnethercote
Copy link
Contributor Author

It has been a week since the PR was filed, and I don't think it's controversial, so I will r+. It'll take a while to get merged, if anyone objects they can still r- in the meantime.

@bors r=Aaron1011

@bors
Copy link
Contributor

bors commented Oct 11, 2022

📌 Commit ed6f481 has been approved by Aaron1011

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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2022
@bors
Copy link
Contributor

bors commented Oct 12, 2022

⌛ Testing commit ed6f481 with merge 2b91cbe...

@bors
Copy link
Contributor

bors commented Oct 12, 2022

☀️ Test successful - checks-actions
Approved by: Aaron1011
Pushing 2b91cbe to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 12, 2022
@bors bors merged commit 2b91cbe into rust-lang:master Oct 12, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 12, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2b91cbe): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-0.6%, -0.3%] 4
Improvements ✅
(secondary)
-4.1% [-9.5%, -1.2%] 9
All ❌✅ (primary) -0.5% [-0.6%, -0.3%] 4

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.3% [-4.0%, -2.0%] 8
Improvements ✅
(secondary)
-2.6% [-3.8%, -1.1%] 35
All ❌✅ (primary) -3.3% [-4.0%, -2.0%] 8

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
2.7% [2.6%, 2.8%] 2
Regressions ❌
(secondary)
6.6% [6.6%, 6.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-5.2% [-5.2%, -5.2%] 2
All ❌✅ (primary) 2.7% [2.6%, 2.8%] 2

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@nnethercote nnethercote deleted the TokenStreamBuilder branch October 12, 2022 08:58
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…ron1011

Remove `TokenStreamBuilder`

`TokenStreamBuilder` is used to combine multiple token streams. It can be removed, leaving the code a little simpler and a little faster.

r? `@Aaron1011`
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.

6 participants