TokenStream::extend #53304
TokenStream::extend #53304
Conversation
rust-highfive
added
the
S-waiting-on-review
|
Requires dtolnay/proc-macro2#114 in order to see the performance improvement. |
| offset: 0, | ||
| len: vec.len() as u32, | ||
| data: Lrc::new(vec), | ||
| } |
There was a problem hiding this comment.
Could this delegate to new_preserving_capacity after the shrink_to_fit?
src/libsyntax/util/rc_vec.rs
Outdated
| vec.truncate(self.offset as usize + self.len as usize); | ||
| // Drop any elements before our view of the data. | ||
| if self.offset != 0 { | ||
| vec.drain(..self.offset as usize); |
There was a problem hiding this comment.
Switching these two may be a bit more readable perhaps?
vec.drain(..self.offset as usize);
vec.truncate(self.len as usize);Additionally, how come the drain is conditional?
There was a problem hiding this comment.
I removed the conditional but I kept the order of truncate first then drain. I added a comment to explain -- draining first would involve unnecessarily shifting all the vector elements that are past the end of the current rc's view of the data.
|
@bors: delegate=dtolnay Looks great to me, very nice find! r=me with a few nits, and otherwise cc @rust-lang/libs, @nrc, and @petrochenkov on the API addition here. |
|
✌️ @dtolnay can now approve this pull request |
|
@bors r+ |
|
📌 Commit 69b9c23 has been approved by |
bors
added
S-waiting-on-bors
|
@dtolnay I'd like to hear how you found this; just profiling while building a |
|
@BurntPizza no profiling involved this time, although that would have revealed the problem. I think I've built enough of Serde / syn / quote / proc-macro2 to know where the skeletons are hidden and I finally got a chance to pursue this one. The original code in alexcrichton/proc-macro2#114 performs a quadratic amount of memory allocation and copying when called the way |
|
Ah, the "Because I'm a guru" methodology. |
TokenStream::extend Two new insta-stable impls in libproc_macro: ```rust impl Extend<TokenTree> for TokenStream impl Extend<TokenStream> for TokenStream ``` `proc_macro::TokenStream` already implements `FromIterator<TokenTree>` and `FromIterator<TokenStream>` so I elected to support the same input types for `Extend`. **This commit reduces compile time of Serde derives by 60% (takes less than half as long to compile)** as measured by building our test suite: ```console $ git clone https://github.com/serde-rs/serde $ cd serde/test_suite $ cargo check --tests --features proc-macro2/nightly $ rm -f ../target/debug/deps/libtest_*.rmeta $ time cargo check --tests --features proc-macro2/nightly Before: 20.8 seconds After: 8.6 seconds ``` r? @alexcrichton
|
☀️ Test successful - status-appveyor, status-travis |
There was a problem hiding this comment.
Perf looks clean, I see no regression from changing RcSlice to RcVec. perf comparison
dtolnay
added
T-libs-api
Two new insta-stable impls in libproc_macro:
proc_macro::TokenStreamalready implementsFromIterator<TokenTree>andFromIterator<TokenStream>so I elected to support the same input types forExtend.This commit reduces compile time of Serde derives by 60% (takes less than half as long to compile) as measured by building our test suite:
r? @alexcrichton