specialize some collection and iterator operations to run in-place #70793
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
cc @Centril isn't there a moratorium on new usage of specialization? |
|
Yeah, there is. However, @matthewjasper recently landed #68970, which provides a mechanism for limited and sound specialization. I believe we need to complete the first two points in their PR description before we can add more specialization:
|
|
☔ The latest upstream changes (presumably #70800) made this pull request unmergeable. Please resolve the merge conflicts. |
the8472
force-pushed
the
in-place-iter-collect
branch
from
09268c2
to
be03b6a
Compare
This comment has been minimized.
This comment has been minimized.
the8472
force-pushed
the
in-place-iter-collect
branch
from
be03b6a
to
6e28e84
Compare
joelpalmer
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #71230) made this pull request unmergeable. Please resolve the merge conflicts. |
joelpalmer
added
S-waiting-on-review
Dylan-DPC-zz
added
S-waiting-on-author
|
@the8472 if you can resolve the conflicts we can get this reviewed |
This comment has been minimized.
This comment has been minimized.
LukasKalbertodt
added
S-blocked
the8472
force-pushed
the
in-place-iter-collect
branch
from
6e28e84
to
4cce024
Compare
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
the8472
commented
May 24, 2020
|
@Dylan-DPC it's ready for review now. |
|
This could also use a new perf run. |
dtolnay
added
the
T-libs
bors
added
the
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
This was referenced Sep 3, 2020
RalfJung
reviewed
Sep 13, 2020
| #[test] | ||
| fn test_from_iter_specialization_with_iterator_adapters() { | ||
| fn assert_in_place_trait<T: InPlaceIterable>(_: &T) {}; | ||
| let src: Vec<usize> = vec![0usize; 65535]; |
There was a problem hiding this comment.
FWIW, this test takes forever to run in Miri.^^ I'll reduce it to 512 elements when cfg(miri) is set to make https://github.com/RalfJung/miri-test-libstd not take forever.
RalfJung
added a commit
to RalfJung/rust
that referenced
this pull request
Sep 13, 2020
…lacrum Fix liballoc test suite for Miri Mostly, fix the regression introduced by rust-lang#75207 that caused slices (i.e., references) to be created to invalid memory or memory that has aliasing pointers that we want to keep valid. @dylni this changes the type of `check_range` to only require the length, not the full reference to the slice, which indeed is all the information this function requires. Also reduce the size of a test introduced in rust-lang#70793 to make it not take 3 minutes in Miri. This makes https://github.com/RalfJung/miri-test-libstd work again.
RalfJung
added a commit
to RalfJung/rust
that referenced
this pull request
Sep 13, 2020
…lacrum Fix liballoc test suite for Miri Mostly, fix the regression introduced by rust-lang#75207 that caused slices (i.e., references) to be created to invalid memory or memory that has aliasing pointers that we want to keep valid. @dylni this changes the type of `check_range` to only require the length, not the full reference to the slice, which indeed is all the information this function requires. Also reduce the size of a test introduced in rust-lang#70793 to make it not take 3 minutes in Miri. This makes https://github.com/RalfJung/miri-test-libstd work again.
RalfJung
added a commit
to RalfJung/rust
that referenced
this pull request
Sep 16, 2020
…lacrum Fix liballoc test suite for Miri Mostly, fix the regression introduced by rust-lang#75207 that caused slices (i.e., references) to be created to invalid memory or memory that has aliasing pointers that we want to keep valid. @dylni this changes the type of `check_range` to only require the length, not the full reference to the slice, which indeed is all the information this function requires. Also reduce the size of a test introduced in rust-lang#70793 to make it not take 3 minutes in Miri. This makes https://github.com/RalfJung/miri-test-libstd work again.
|
@the8472 fwiw, on future PRs, especially if rebasing it would be great to squash out commits that are fixups (e.g., tidy fixes) and such to have a cleaner git history. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a rebase and update of #66383 which was closed due inactivity.
Recent rustc changes made the compile time regressions disappear, at least for webrender-wrench. Running a stage2 compile and the rustc-perf suite takes hours on the hardware I have at the moment, so I can't do much more than that.
In the best case of the
vec::bench_in_place_recyclesynthetic microbenchmark these optimizations can provide a 15x speedup over the regular implementation which allocates a new vec for every benchmark iteration. Benchmark results. In real code the speedups are tiny, but it also depends on the allocator used, a system allocator that uses a process-wide mutex will benefit more than one with thread-local pools.What was changed
SpecExtendwhich coveredfrom_iterandextendspecializations was split into separate traitsextendandfrom_iternow reuse theappend_elementsif passed iterators are from slices.vec.into_iter().collect::<Vec<_>>()optimization that passed through the original vec has been generalized further to also cover cases where the original has been partially drained.IntoIters through various iterator adapters collected into Vec and BinaryHeap will be performed in place as long asTandUhave the same alignment and size and aren't ZSTs.SourceIterandInPlaceIterabletraits have been added. The first allows reaching through the iterator pipeline to grab a pointer to the source memory. The latter is a marker that promises that the read pointer will advance as fast or faster than the write pointer and thus in-place operation is possible in the first place.vec::IntoIterimplementsTrustedRandomAccessforT: Copyto allow in-place collection when there is aZipadapter in the iterator. TRA had to be made an unstable public trait to support this.In-place collectible adapters
MapMapWhileFilterFilterMapFuseSkipSkipWhileTakeTakeWhileEnumerateZip(left hand side only,Copytypes only)PeekScanInspectConcerns
vec.into_iter().filter(|_| false).collect()will no longer return a vec with 0 capacity, instead it will return its original allocation. This avoids the cost of doing any allocation or deallocation but could lead to large allocations living longer than expected.If that's not acceptable some resizing policy at the end of the attempted in-place collect would be necessary, which in the worst case could result in one more memcopy than the non-specialized case.
Possible followup work
ignore-tidy-filelengthvec.into_iter().skip(1).collect::<Vec<)>>()to compile to amemmove(currently compiles to a pile of SIMD, see Missed optimization: repeated pointer increments don't compile to a memcpy #69187 )HashSetto be in-place collected into aVec