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 `LazyTokenStream`. #58476

Merged

bors merged 5 commits into rust-lang:master from nnethercote:rm-LazyTokenStream

Feb 23, 2019

Contributor

nnethercote commented Feb 15, 2019

LazyTokenStream was added in #40939. Perhaps it was an effective optimization then, but no longer. This PR removes it, making the code both simpler and faster.

r? @alexcrichton

rust-highfive assigned alexcrichton

rust-highfive added the S-waiting-on-review label

nnethercote force-pushed the rm-LazyTokenStream branch from 6b6d80c to 082b649 Compare

February 15, 2019 03:53

Contributor Author

nnethercote commented Feb 15, 2019

Contributor

bors commented Feb 15, 2019

⌛ Trying commit 082b649 with merge 25f58f3...

bors added a commit that referenced this pull request


          Auto merge of #58476 - nnethercote:rm-LazyTokenStream, r=<try>

25f58f3

Remove `LazyTokenStream`.

`LazyTokenStream` was added in #40939. Perhaps it was an effective optimization then, but no longer. This PR removes it, making the code both simpler and faster.

r? @alexcrichton

Mark-Simulacrum approved these changes

View reviewed changes

src/librustc/hir/lowering.rs Outdated Show resolved Hide resolved

Contributor

bors commented Feb 15, 2019

☀️ Test successful - checks-travis
State: approved= try=True

petrochenkov self-assigned this

petrochenkov reviewed

View reviewed changes

src/libsyntax/parse/token.rs Outdated Show resolved Hide resolved

Contributor

petrochenkov commented Feb 15, 2019

I'm sorry for the amount of hacks you had to witness while doing this.

Contributor

petrochenkov commented Feb 15, 2019

Waiting on perf, otherwise r=me with nits addressed.

petrochenkov added S-waiting-on-perf and removed S-waiting-on-review labels

petrochenkov unassigned alexcrichton

Contributor Author

nnethercote commented Feb 15, 2019

@rust-timer build 25f58f3

Collaborator

rust-timer commented Feb 15, 2019

Success: Queued 25f58f3 with parent f47ec2a, comparison URL.

Contributor Author

nnethercote commented Feb 15, 2019

I'm sorry for the amount of hacks you had to witness while doing this.

It reminds me of #57486 in which I removed ThinTokenStream -- another token stream "optimization" that actually wasn't...

Collaborator

rust-timer commented Feb 15, 2019

Finished benchmarking try commit 25f58f3

petrochenkov added S-waiting-on-author and removed S-waiting-on-perf labels

Contributor Author

nnethercote commented Feb 15, 2019

The comparison URL shows a clear instructions and max-rss win for deep-vector, a smaller instructions win for keccak, and nothing gets worse.

Member

alexcrichton commented Feb 15, 2019

r? @petrochenkov

nnethercote added 5 commits

February 18, 2019 09:38


          Change Token::interpolated_to_tokenstream().

d26bf74

It is currently a method of `Token`, but it only is valid to call if
`self` is a `Token::Interpolated`. This commit eliminates the
possibility of misuse by changing it to an associated function that
takes a `Nonterminal`, which also simplifies the call sites.

This requires splitting out a new function, `nonterminal_to_string`.


          Remove LazyTokenStream.

f8801f3

It's present within `Token::Interpolated` as an optimization, so that if
a nonterminal is converted to a `TokenStream` multiple times, the
first-computed value is saved and reused.

But in practice it's not needed. `interpolated_to_tokenstream()` is a
cold function: it's only called a few dozen times while compiling rustc
itself, and a few hundred times across the entire `rustc-perf` suite.
Furthermore, when it is called, it is almost always the first
conversion, so no benefit is gained from it.

So this commit removes `LazyTokenStream`, along with the now-unnecessary
`Token::interpolated()`.

As well as a significant simplification, the removal speeds things up
slightly, mostly due to not having to `drop` the `LazyTokenStream`
instances.


          Avoid a clone() in transcribe().

f0d8fbd

The current code (expensively) clones the value within an `Rc`. This
commit changes things so that the `Rc` itself is (cheaply) cloned
instead, avoid some allocations.

This requires converting a few `Rc` instances to `Lrc`.


          Make interpolated_to_tokenstream a method on Nonterminal.

82ad4f1


          Remove some unnecessary into() calls.

895a794

These are probably leftovers from recent `TokenStream` simplifications.

nnethercote force-pushed the rm-LazyTokenStream branch from 082b649 to 895a794 Compare

February 17, 2019 23:56

Contributor Author

nnethercote commented Feb 17, 2019

@petrochenkov: I've pushed two new commits, one to address your review comment, and another very simple clean-up.

Contributor

petrochenkov commented Feb 18, 2019

Contributor

bors commented Feb 18, 2019

📌 Commit 895a794 has been approved by petrochenkov

bors added S-waiting-on-bors and removed S-waiting-on-author labels

Centril added a commit to Centril/rust that referenced this pull request


          Rollup merge of rust-lang#58476 - nnethercote:rm-LazyTokenStream, r=p…

64b0c0d

…etrochenkov

Remove `LazyTokenStream`.

`LazyTokenStream` was added in rust-lang#40939. Perhaps it was an effective optimization then, but no longer. This PR removes it, making the code both simpler and faster.

r? @alexcrichton

Centril mentioned this pull request

Rollup of 12 pull requests #58594

Closed

Centril added a commit to Centril/rust that referenced this pull request


          Rollup merge of rust-lang#58476 - nnethercote:rm-LazyTokenStream, r=p…

b440487

…etrochenkov

Remove `LazyTokenStream`.

`LazyTokenStream` was added in rust-lang#40939. Perhaps it was an effective optimization then, but no longer. This PR removes it, making the code both simpler and faster.

r? @alexcrichton

Centril mentioned this pull request

Rollup of 16 pull requests #58664

Closed

Centril added a commit to Centril/rust that referenced this pull request


          Rollup merge of rust-lang#58476 - nnethercote:rm-LazyTokenStream, r=p…

117747d

…etrochenkov

Remove `LazyTokenStream`.

`LazyTokenStream` was added in rust-lang#40939. Perhaps it was an effective optimization then, but no longer. This PR removes it, making the code both simpler and faster.

r? @alexcrichton

Centril mentioned this pull request

Rollup of 16 pull requests #58665

Closed

Centril added a commit to Centril/rust that referenced this pull request


          Rollup merge of rust-lang#58476 - nnethercote:rm-LazyTokenStream, r=p…

ba9006f

…etrochenkov

Remove `LazyTokenStream`.

`LazyTokenStream` was added in rust-lang#40939. Perhaps it was an effective optimization then, but no longer. This PR removes it, making the code both simpler and faster.

r? @alexcrichton

Centril mentioned this pull request

Rollup of 15 pull requests #58666

Closed

Centril added a commit to Centril/rust that referenced this pull request


          Rollup merge of rust-lang#58476 - nnethercote:rm-LazyTokenStream, r=p…

c8ade46

…etrochenkov

Remove `LazyTokenStream`.

`LazyTokenStream` was added in rust-lang#40939. Perhaps it was an effective optimization then, but no longer. This PR removes it, making the code both simpler and faster.

r? @alexcrichton

Centril mentioned this pull request

Rollup of 16 pull requests #58668

Closed

Centril added a commit to Centril/rust that referenced this pull request


          Rollup merge of rust-lang#58476 - nnethercote:rm-LazyTokenStream, r=p…

585d4d2

…etrochenkov

Remove `LazyTokenStream`.

`LazyTokenStream` was added in rust-lang#40939. Perhaps it was an effective optimization then, but no longer. This PR removes it, making the code both simpler and faster.

r? @alexcrichton

Centril mentioned this pull request

Rollup of 16 pull requests #58669

Merged

bors added a commit that referenced this pull request


          Auto merge of #58669 - Centril:rollup, r=Centril

7f19f16

Rollup of 16 pull requests

Successful merges:

 - #58100 (Transition librustdoc to Rust 2018)
 - #58122 (RangeInclusive internal iteration performance improvement.)
 - #58199 (Add better error message for partial move)
 - #58227 (Updated RELEASES.md for 1.33.0)
 - #58353 (Check the Self-type of inherent associated constants)
 - #58453 (SGX target: fix panic = abort)
 - #58476 (Remove `LazyTokenStream`.)
 - #58526 (Special suggestion for illegal unicode curly quote pairs)
 - #58595 (Turn duration consts into associated consts)
 - #58609 (Allow Self::Module to be mutated.)
 - #58628 (Optimise vec![false; N] to zero-alloc)
 - #58643 (Don't generate minification variables if minification disabled)
 - #58648 (Update tests to account for cross-platform testing and miri.)
 - #58654 (Do not underflow after resetting unmatched braces count)
 - #58658 (Add expected/provided byte alignments to validation error message)
 - #58667 (Reduce Miri-related Code Repetition `like (n << amt) >> amt`)

Failed merges:

r? @ghost

bors merged commit 895a794 into rust-lang:master

nnethercote deleted the rm-LazyTokenStream branch

February 26, 2019 00:04

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

Labels

S-waiting-on-bors