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

Speed up `TokenStream` concatenation #65198

Merged
merged 2 commits into from Oct 9, 2019

Conversation

@nnethercote
Copy link
Contributor

nnethercote commented Oct 8, 2019

This PR fixes the quadratic behaviour identified in #65080.

r? @Mark-Simulacrum

nnethercote added 2 commits Oct 7, 2019
Currently, this function creates a new empty stream, and then appends
the elements from each given stream onto that stream. This can cause
quadratic behaviour.

This commit changes the function so that it modifies the first stream
(which can be long) by extending it with the elements from the
subsequent streams (which are almost always short), which avoids the
quadratic behaviour.
Currently, when two tokens must be glued together, this function duplicates
large chunks of the existing streams. This can cause quadratic behaviour.

This commit changes the function so that it overwrites the last token with a
glued token, which avoids the quadratic behaviour. This removes the need for
`TokenStreamBuilder::push_all_but_{first,last}_tree`.

The commit also restructures `push` somewhat, by removing
`TokenStream::{first_tree_and_joint,last_tree_if_joint}` in favour of more
pattern matching and some comments. This makes the code shorter, and in my
opinion, more readable.
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 8, 2019

In my local measurements, this provided a very small (<1%) improvement on some of the standard benchmarks.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Oct 8, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 8, 2019

⌛️ Trying commit 75e0078 with merge c255427...

bors added a commit that referenced this pull request Oct 8, 2019
Speed up `TokenStream` concatenation

This PR fixes the quadratic behaviour identified in #65080.

r? @Mark-Simulacrum
// Get the first stream. If it's `None`, create an empty
// stream.
let mut iter = streams.drain();
let mut first_stream_lrc = match iter.next().unwrap().0 {

This comment has been minimized.

Copy link
@Centril

Centril Oct 8, 2019

Member
Suggested change
let mut first_stream_lrc = match iter.next().unwrap().0 {
let mut first_stream_lrc = match iter.next().unwrap().0.unwrap_or_default();
// space for them.
let first_vec_mut = Lrc::make_mut(&mut first_stream_lrc);
first_vec_mut.reserve(num_appends);
for stream in iter {

This comment has been minimized.

Copy link
@Centril

Centril Oct 8, 2019

Member

I think this would be cleaner using .filter_map(|x| x) and .flat_map(|s| s.iter().cloned()).

This comment has been minimized.

Copy link
@bluss

bluss Oct 8, 2019

Member

Current version is easy to read, and the alternatives seem to spend more Rust functions to do the same thing, IMO one can prefer the current.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 8, 2019

☀️ Try build successful - checks-azure
Build commit: c255427 (c25542724f393647fc93a8a0319edaa827e701d9)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Oct 8, 2019

Queued c255427 with parent d304f5c, future comparison URL.

Copy link
Member

Mark-Simulacrum left a comment

Code changes look good to me. I'd like to get @petrochenkov or perhaps @matklad to sign off too though since I'm not too familiar with this code.

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Oct 8, 2019

LGTM

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Oct 8, 2019

@bors r+ rollup=never

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 8, 2019

📌 Commit 75e0078 has been approved by Mark-Simulacrum

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 8, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 8, 2019

The perf run didn't work. Let's try it the old-fashioned way:

@rust-timer build c255427

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Oct 8, 2019

Queued c255427 with parent d304f5c, future comparison URL.

@mati865

This comment has been minimized.

Copy link
Contributor

mati865 commented Oct 8, 2019

@nnethercote it worked but still haven't finished, you can see it here https://perf.rust-lang.org/status.html

That 700000% wall time regression made queue a bit long...

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Oct 9, 2019

Finished benchmarking try commit c255427, comparison URL.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 9, 2019

Lots of small (<1%) improvements, mostly in the short-running benchmarks.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 9, 2019

⌛️ Testing commit 75e0078 with merge e59dab5...

bors added a commit that referenced this pull request Oct 9, 2019
Speed up `TokenStream` concatenation

This PR fixes the quadratic behaviour identified in #65080.

r? @Mark-Simulacrum
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 9, 2019

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing e59dab5 to master...

@bors bors added the merged-by-bors label Oct 9, 2019
@bors bors merged commit 75e0078 into rust-lang:master Oct 9, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191008.6 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@nnethercote nnethercote deleted the nnethercote:fix-65080 branch Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.