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

Simplify TokenStream some more #57486

Merged
merged 4 commits into from
Jan 19, 2019

Conversation

nnethercote
Copy link
Contributor

These commits simplify TokenStream, remove ThinTokenStream, and avoid some clones. The end result is simpler code and a slight perf win on some benchmarks.

r? @petrochenkov

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

(Note that the first commit is from #57004, and will be removed before landing.)

@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 Jan 10, 2019
@nnethercote
Copy link
Contributor Author

I added a new commit that changes TokenStream to uses Option.

None => true,
Some(ref stream) => {
// An empty stream should be represented as `None`, not as an empty `Vec`.
debug_assert!(stream.len() > 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, do we have operations that remove trees from the stream's vector?
It's correct that we should not create new empty streams as Some, but perhaps previously non-empty streams can turn into empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I know of. But if it does happen, the old code for this function would silently do the wrong thing, while the new code will assert. I could change it to check for a length of zero in the Some case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, could you use the correct Some(ref stream) => stream.is_empty(), that would be more future-proof.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 11, 2019

📌 Commit ea2784b8701bfd63dada95ea12bd854479b88e45 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 Jan 11, 2019
@petrochenkov
Copy link
Contributor

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 13, 2019
@petrochenkov
Copy link
Contributor

Also, it would be nice to make a perf run once #57004 is merged and this PR contains only the following diff.

@bors
Copy link
Contributor

bors commented Jan 13, 2019

☔ The latest upstream changes (presumably #57577) made this pull request unmergeable. Please resolve the merge conflicts.

`TokenStream::Stream` can represent a token stream containing any number
of token trees. `TokenStream::Tree` is the special case representing a
single token tree. The latter doesn't occur all that often dynamically,
so this commit removes it, which simplifies the code quite a bit.

This change has mixed performance effects.

- The size of `TokenStream` drops from 32 bytes to 8 bytes, and there
  is one less case for all the match statements.

- The conversion of a `TokenTree` to a `TokenStream` now requires two
  allocations, for the creation of a single element Lrc<Vec<_>>. (But a
  subsequent commit in this PR will reduce the main source of such
  conversions.)
`TokenStream` is now almost identical to `ThinTokenStream`. This commit
removes the latter, replacing it with the former.
Because that's the more typical way of representing an all-or-nothing
type.
@nnethercote
Copy link
Contributor Author

I updated is_empty and added a comment about empty streams not necessarily being represented with Empty.

@nnethercote
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Jan 14, 2019

⌛ Trying commit 7285724 with merge ad694da89a85d9e7fa5f5eee94b1159f186c2778...

@petrochenkov
Copy link
Contributor

Travis says the build completed successfully, but this wasn't reported to this thread for some reason.
Let's see if rust-timer works with it or not.

@rust-timer build ad694da89a85d9e7fa5f5eee94b1159f186c2778

@rust-timer
Copy link
Collaborator

Insufficient permissions to issue commands to rust-timer.

@petrochenkov petrochenkov added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 14, 2019
@petrochenkov
Copy link
Contributor

@bors r-

@petrochenkov
Copy link
Contributor

(The "comparison URL" didn't work for me previously and showed "missing commit" or something like that, but now it seems ok.)

@bors r+

@bors
Copy link
Contributor

bors commented Jan 14, 2019

📌 Commit 7285724 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 Jan 14, 2019
Centril added a commit to Centril/rust that referenced this pull request Jan 15, 2019
…re, r=petrochenkov

Simplify `TokenStream` some more

These commits simplify `TokenStream`, remove `ThinTokenStream`, and avoid some clones. The end result is simpler code and a slight perf win on some benchmarks.

r? @petrochenkov
@Centril
Copy link
Contributor

Centril commented Jan 15, 2019

Failed in rollup #57624 (comment) due to toolstate change.

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 15, 2019
@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 15, 2019
@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 15, 2019

Marking as blocked, we'll be able to merge this in few days when 1.32 is released and the pre-release toolstate restriction is lifted.

@nnethercote
Copy link
Contributor Author

What is toolstate?

@petrochenkov
Copy link
Contributor

@nnethercote
Build status of things like clippy/rustfmt/... that depend on compiler internals.
The restriction is that compiler changes must not break tools during the week before release.

@nnethercote
Copy link
Contributor Author

Rust 1.32 has been released.

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jan 17, 2019

📌 Commit 7285724 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-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jan 17, 2019
Centril added a commit to Centril/rust that referenced this pull request Jan 19, 2019
…re, r=petrochenkov

Simplify `TokenStream` some more

These commits simplify `TokenStream`, remove `ThinTokenStream`, and avoid some clones. The end result is simpler code and a slight perf win on some benchmarks.

r? @petrochenkov
bors added a commit that referenced this pull request Jan 19, 2019
Rollup of 7 pull requests

Successful merges:

 - #57486 (Simplify `TokenStream` some more)
 - #57502 (make trait-aliases work across crates)
 - #57598 (Add missing unpretty option help message)
 - #57649 (privacy: Account for associated existential types)
 - #57659 (Fix release manifest generation)
 - #57699 (add applicability to remaining suggestions)
 - #57719 (Tweak `expand_node`)

Failed merges:

r? @ghost
@bors bors merged commit 7285724 into rust-lang:master Jan 19, 2019
@nnethercote nnethercote deleted the simplify-TokenStream-more branch January 21, 2019 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants