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

Projects
None yet
7 participants
@nnethercote
Copy link
Contributor

nnethercote commented Jan 10, 2019

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

@nnethercote

This comment has been minimized.

Copy link
Contributor

nnethercote commented Jan 10, 2019

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

@nnethercote

This comment has been minimized.

Copy link
Contributor

nnethercote commented Jan 11, 2019

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);

This comment has been minimized.

@petrochenkov

petrochenkov Jan 11, 2019

Contributor

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?

This comment has been minimized.

@nnethercote

nnethercote Jan 11, 2019

Contributor

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.

This comment has been minimized.

@petrochenkov

petrochenkov Jan 13, 2019

Contributor

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

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 11, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 11, 2019

📌 Commit ea2784b has been approved by petrochenkov

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 13, 2019

@bors r-

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 13, 2019

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

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 13, 2019

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

nnethercote added some commits Jan 9, 2019

Remove `TokenStream::Tree` variant.
`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.)
Remove `ThinTokenStream`.
`TokenStream` is now almost identical to `ThinTokenStream`. This commit
removes the latter, replacing it with the former.
Make `TokenStream` use `Option`.
Because that's the more typical way of representing an all-or-nothing
type.

@nnethercote nnethercote force-pushed the nnethercote:simplify-TokenStream-more branch from ea2784b to 7285724 Jan 14, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor

nnethercote commented Jan 14, 2019

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

@nnethercote

This comment has been minimized.

Copy link
Contributor

nnethercote commented Jan 14, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2019

⌛️ Trying commit 7285724 with merge ad694da...

bors added a commit that referenced this pull request Jan 14, 2019

Auto merge of #57486 - nnethercote:simplify-TokenStream-more, r=<try>
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
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 14, 2019

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 ad694da

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 14, 2019

Insufficient permissions to issue commands to rust-timer.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 14, 2019

@bors try

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 14, 2019

@bors r-

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 14, 2019

(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

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2019

📌 Commit 7285724 has been approved by petrochenkov

Centril added a commit to Centril/rust that referenced this pull request Jan 15, 2019

Rollup merge of rust-lang#57486 - nnethercote:simplify-TokenStream-mo…
…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 15, 2019

Auto merge of #57624 - Centril:rollup, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #57253 (Make privacy checking, intrinsic checking and liveness checking incremental)
 - #57352 (forbid manually impl'ing one of an object type's marker traits)
 - #57486 (Simplify `TokenStream` some more)
 - #57537 (Small perf improvement for fmt)
 - #57579 (Add core::iter::once_with())
 - #57587 (Add 'rustc-env:RUST_BACKTRACE=0' to const-pat-ice test)
 - #57608 (Simplify 'product' factorial example)
 - #57614 ([rustdoc] Fix crates filtering box not being filled)

Failed merges:

r? @ghost
@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 15, 2019

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

@bors r-

@petrochenkov

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

nnethercote commented Jan 15, 2019

What is toolstate?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 15, 2019

@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

This comment has been minimized.

Copy link
Contributor

nnethercote commented Jan 17, 2019

Rust 1.32 has been released.

@bors r=petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 17, 2019

📌 Commit 7285724 has been approved by petrochenkov

@bors bors added S-waiting-on-bors and removed S-blocked labels Jan 17, 2019

Centril added a commit to Centril/rust that referenced this pull request Jan 19, 2019

Rollup merge of rust-lang#57486 - nnethercote:simplify-TokenStream-mo…
…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

Auto merge of #57755 - Centril:rollup, r=Centril
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

1 check was pending

homu Trying commit 728572440171d8d9c0557c89c3d71cc8d7cf6c2e with merge ad694da89a85d9e7fa5f5eee94b1159f186c2778...
Details

@nnethercote nnethercote deleted the nnethercote:simplify-TokenStream-more branch Jan 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment