Use Vec::extend in SmallVec::extend when applicable by ljedrz · Pull Request #52859 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation19 Commits2 Checks0 Files changed

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

As calculated in #52738, Vec::extend is much faster than pushing to it in a loop. We can take advantage of this method in SmallVec too - at least in cases when its underlying object is an AccumulateVec::Heap.

This approach also accidentally improves the push loop of the AccumulateVec::Array variant, because it doesn't utilize SmallVec::push which performs self.reserve(1) with every iteration; this is unnecessary, because we're already reserving the whole space we will be needing by performing self.reserve(iter.size_hint().0) at the beginning.

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

This comment has been minimized.

Seems good. If you're interested, it'd be great to write some benchmarks for smallvec to have more confidence in such changes.

@bors r+

📌 Commit c5dfc2d4d2ce94d6316b64d22933110bc9be1a01 has been approved by Mark-Simulacrum

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Jul 30, 2018

Oh, Travis failed.

@bors r-

bors added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

labels

Jul 30, 2018

And with an ICE? It seems one can never be too careful; I'll dig some more.

I suspect this is because the lower bound of the size hint may be too small so it's possible we're not switching to the vector early enough -- that is, the array is too short.

Yes, I was just about to write the same thing; I forgot the lower bound is not to be trusted.

I think adding arr.reserve(1) to the loop will fix this. This will sadly remove the accidental optimization, but the first one is still worthwhile. I'll test it shortly.

@Mark-Simulacrum ok, I fixed it and successfully ran stage 1 tests; should be all good now.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguably we should match on self here and either extend or push based on that, but maybe it's not worth it. Benchmarks would be good, but that's quite a bit of work.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean instead of is_array? I wanted to do that, but I ran into issues with the borrow checker. I'm going to think about some possible benchmarks.

📌 Commit 9169934 has been approved by Mark-Simulacrum

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Jul 30, 2018

I created some simple benchmarks and checked the results of this change; it appears to be beneficial (up to a 4x speedup) when the required capacity is known and is greater than the size of the underlying array; without it with_capacity surprisingly (or rather counter-intuitively) doesn't seem to make any difference.

before:

test small_vec::tests::fill_small_vec_1_10_with_cap  ... bench:         147 ns/iter (+/- 6)
test small_vec::tests::fill_small_vec_1_10_wo_cap    ... bench:         143 ns/iter (+/- 18)
test small_vec::tests::fill_small_vec_1_50_with_cap  ... bench:         403 ns/iter (+/- 19)
test small_vec::tests::fill_small_vec_1_50_wo_cap    ... bench:         388 ns/iter (+/- 77)
test small_vec::tests::fill_small_vec_32_10_with_cap ... bench:          84 ns/iter (+/- 15)
test small_vec::tests::fill_small_vec_32_10_wo_cap   ... bench:          70 ns/iter (+/- 0)
test small_vec::tests::fill_small_vec_32_50_with_cap ... bench:         531 ns/iter (+/- 78)
test small_vec::tests::fill_small_vec_32_50_wo_cap   ... bench:         537 ns/iter (+/- 89)
test small_vec::tests::fill_small_vec_8_10_with_cap  ... bench:         139 ns/iter (+/- 18)
test small_vec::tests::fill_small_vec_8_10_wo_cap    ... bench:         137 ns/iter (+/- 6)
test small_vec::tests::fill_small_vec_8_50_with_cap  ... bench:         372 ns/iter (+/- 18)
test small_vec::tests::fill_small_vec_8_50_wo_cap    ... bench:         367 ns/iter (+/- 18)

after:

test small_vec::tests::fill_small_vec_1_10_with_cap  ... bench:          83 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_1_10_wo_cap    ... bench:         147 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_1_50_with_cap  ... bench:          95 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_1_50_wo_cap    ... bench:         425 ns/iter (+/- 56)
test small_vec::tests::fill_small_vec_32_10_with_cap ... bench:          88 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_32_10_wo_cap   ... bench:          74 ns/iter (+/- 2)
test small_vec::tests::fill_small_vec_32_50_with_cap ... bench:         155 ns/iter (+/- 6)
test small_vec::tests::fill_small_vec_32_50_wo_cap   ... bench:         511 ns/iter (+/- 24)
test small_vec::tests::fill_small_vec_8_10_with_cap  ... bench:          88 ns/iter (+/- 15)
test small_vec::tests::fill_small_vec_8_10_wo_cap    ... bench:         134 ns/iter (+/- 5)
test small_vec::tests::fill_small_vec_8_50_with_cap  ... bench:         100 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_8_50_wo_cap    ... bench:         364 ns/iter (+/- 56)

Can you push those benchmarks as another commit? That way they will be easier for others to reuse.

Sure; are they fine as an extension of the existing small_vec.rs file (a test module like in the gist)?

📌 Commit ca52648 has been approved by Mark-Simulacrum

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request

Aug 1, 2018

…-Simulacrum

Use Vec::extend in SmallVec::extend when applicable

As calculated in rust-lang#52738, Vec::extend is much faster than pushing to it in a loop. We can take advantage of this method in SmallVec too - at least in cases when its underlying object is an AccumulateVec::Heap.

This approach also accidentally improves the push loop of the AccumulateVec::Array variant, because it doesn't utilize SmallVec::push which performs self.reserve(1) with every iteration; this is unnecessary, because we're already reserving the whole space we will be needing by performing self.reserve(iter.size_hint().0) at the beginning.

bors added a commit that referenced this pull request

Aug 1, 2018

Rollup of 31 pull requests

Successful merges:

• #52332 (dead-code lint: say "constructed" for structs)
• #52340 (Document From trait implementations for OsStr, OsString, CString, and CStr)
• #52628 (Cleanup some rustdoc code)
• #52732 (Remove unstable and deprecated APIs)
• #52745 (Update clippy to latest master)
• #52756 (rustc: Disallow machine applicability in foreign macros)
• #52771 (Clarify thread::park semantics)
• #52810 ([NLL] Don't make "fake" match variables mutable)
• #52821 (pretty print for std::collections::vecdeque)
• #52822 (Fix From)
• #52824 (Fix -Wpessimizing-move warnings in rustllvm/PassWrapper)
• #52831 (remove references to AUTHORS.txt file)
• #52835 (Fix Alias intra doc ICE)
• #52842 (update comment)
• #52846 (Add timeout to use of curl in bootstrap.py.)
• #52851 (Make the tool_lints actually usable)
• #52853 (Improve bootstrap help on stages)
• #52859 (Use Vec::extend in SmallVec::extend when applicable)
• #52861 (Add targets for HermitCore (https://hermitcore.org) to the Rust compiler and port libstd to it.)
• #52867 (releases.md: fix 2 typos)
• #52870 (Implement Unpin for FutureObj and LocalFutureObj)
• #52876 (run-pass/const-endianness: negate before to_le())
• #52878 (Fix wrong issue number in the test name)
• #52883 (Include lifetime in mutability suggestion in NLL messages)
• #52904 (NLL: sort diagnostics by span)
• #52905 (Fix a typo in unsize.rs)
• #52907 (NLL: On "cannot move out of type" error, print original before rewrite)
• #52908 (Use SetLenOnDrop in Vec::truncate())
• #52914 (Only run the sparc-abi test on sparc)
• #52918 (Backport 1.27.2 release notes)
• #52929 (Update compatibility note for 1.28.0 to be correct)

Failed merges:

• #52758 (Cleanup for librustc::session)
• #52799 (Use BitVector for global sets of AttrId)

r? @ghost

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

Aug 1, 2018

…-Simulacrum

Use Vec::extend in SmallVec::extend when applicable

As calculated in rust-lang#52738, Vec::extend is much faster than pushing to it in a loop. We can take advantage of this method in SmallVec too - at least in cases when its underlying object is an AccumulateVec::Heap.

This approach also accidentally improves the push loop of the AccumulateVec::Array variant, because it doesn't utilize SmallVec::push which performs self.reserve(1) with every iteration; this is unnecessary, because we're already reserving the whole space we will be needing by performing self.reserve(iter.size_hint().0) at the beginning.

bors added a commit that referenced this pull request

Aug 1, 2018

Rollup of 30 pull requests

Successful merges:

• #52340 (Document From trait implementations for OsStr, OsString, CString, and CStr)
• #52628 (Cleanup some rustdoc code)
• #52732 (Remove unstable and deprecated APIs)
• #52745 (Update clippy to latest master)
• #52771 (Clarify thread::park semantics)
• #52778 (Improve readability of serialize.rs)
• #52810 ([NLL] Don't make "fake" match variables mutable)
• #52821 (pretty print for std::collections::vecdeque)
• #52822 (Fix From)
• #52824 (Fix -Wpessimizing-move warnings in rustllvm/PassWrapper)
• #52825 (Make sure #47772 does not regress)
• #52831 (remove references to AUTHORS.txt file)
• #52842 (update comment)
• #52846 (Add timeout to use of curl in bootstrap.py.)
• #52851 (Make the tool_lints actually usable)
• #52853 (Improve bootstrap help on stages)
• #52859 (Use Vec::extend in SmallVec::extend when applicable)
• #52861 (Add targets for HermitCore (https://hermitcore.org) to the Rust compiler and port libstd to it.)
• #52867 (releases.md: fix 2 typos)
• #52870 (Implement Unpin for FutureObj and LocalFutureObj)
• #52876 (run-pass/const-endianness: negate before to_le())
• #52878 (Fix wrong issue number in the test name)
• #52883 (Include lifetime in mutability suggestion in NLL messages)
• #52888 (Use suggestions for shell format arguments)
• #52904 (NLL: sort diagnostics by span)
• #52905 (Fix a typo in unsize.rs)
• #52907 (NLL: On "cannot move out of type" error, print original before rewrite)
• #52914 (Only run the sparc-abi test on sparc)
• #52918 (Backport 1.27.2 release notes)
• #52929 (Update compatibility note for 1.28.0 to be correct)

Failed merges:

r? @ghost

ljedrz deleted the smallvec_true_extend branch

August 1, 2018 10:45

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

Aug 1, 2018

This improves SmallVec.extend even more over rust-lang#52859

Before (as of rust-lang#52859):

test small_vec::tests::fill_small_vec_1_10_with_cap  ... bench:          31 ns/iter (+/- 5)
test small_vec::tests::fill_small_vec_1_10_wo_cap    ... bench:          70 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_1_50_with_cap  ... bench:          36 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_1_50_wo_cap    ... bench:         256 ns/iter (+/- 17)
test small_vec::tests::fill_small_vec_32_10_with_cap ... bench:          31 ns/iter (+/- 5)
test small_vec::tests::fill_small_vec_32_10_wo_cap   ... bench:          26 ns/iter (+/- 1)
test small_vec::tests::fill_small_vec_32_50_with_cap ... bench:          49 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_32_50_wo_cap   ... bench:         219 ns/iter (+/- 11)
test small_vec::tests::fill_small_vec_8_10_with_cap  ... bench:          32 ns/iter (+/- 2)
test small_vec::tests::fill_small_vec_8_10_wo_cap    ... bench:          61 ns/iter (+/- 12)
test small_vec::tests::fill_small_vec_8_50_with_cap  ... bench:          37 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_8_50_wo_cap    ... bench:         210 ns/iter (+/- 10)

After:

test small_vec::tests::fill_small_vec_1_10_wo_cap    ... bench:          31 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_1_50_with_cap  ... bench:          39 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_1_50_wo_cap    ... bench:          35 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_32_10_with_cap ... bench:          37 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_32_10_wo_cap   ... bench:          32 ns/iter (+/- 2)
test small_vec::tests::fill_small_vec_32_50_with_cap ... bench:          52 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_32_50_wo_cap   ... bench:          46 ns/iter (+/- 0)
test small_vec::tests::fill_small_vec_8_10_with_cap  ... bench:          35 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_8_10_wo_cap    ... bench:          31 ns/iter (+/- 0)
test small_vec::tests::fill_small_vec_8_50_with_cap  ... bench:          40 ns/iter (+/- 15)
test small_vec::tests::fill_small_vec_8_50_wo_cap    ... bench:          36 ns/iter (+/- 2)

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

Aug 1, 2018

Another SmallVec.extend optimization

This improves SmallVec.extend even more over rust-lang#52859 while making the code easier to read.

Before

test small_vec::tests::fill_small_vec_1_10_with_cap  ... bench:          31 ns/iter (+/- 5)
test small_vec::tests::fill_small_vec_1_10_wo_cap    ... bench:          70 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_1_50_with_cap  ... bench:          36 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_1_50_wo_cap    ... bench:         256 ns/iter (+/- 17)
test small_vec::tests::fill_small_vec_32_10_with_cap ... bench:          31 ns/iter (+/- 5)
test small_vec::tests::fill_small_vec_32_10_wo_cap   ... bench:          26 ns/iter (+/- 1)
test small_vec::tests::fill_small_vec_32_50_with_cap ... bench:          49 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_32_50_wo_cap   ... bench:         219 ns/iter (+/- 11)
test small_vec::tests::fill_small_vec_8_10_with_cap  ... bench:          32 ns/iter (+/- 2)
test small_vec::tests::fill_small_vec_8_10_wo_cap    ... bench:          61 ns/iter (+/- 12)
test small_vec::tests::fill_small_vec_8_50_with_cap  ... bench:          37 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_8_50_wo_cap    ... bench:         210 ns/iter (+/- 10)

After:

test small_vec::tests::fill_small_vec_1_10_wo_cap    ... bench:          31 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_1_50_with_cap  ... bench:          39 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_1_50_wo_cap    ... bench:          35 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_32_10_with_cap ... bench:          37 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_32_10_wo_cap   ... bench:          32 ns/iter (+/- 2)
test small_vec::tests::fill_small_vec_32_50_with_cap ... bench:          52 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_32_50_wo_cap   ... bench:          46 ns/iter (+/- 0)
test small_vec::tests::fill_small_vec_8_10_with_cap  ... bench:          35 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_8_10_wo_cap    ... bench:          31 ns/iter (+/- 0)
test small_vec::tests::fill_small_vec_8_50_with_cap  ... bench:          40 ns/iter (+/- 15)
test small_vec::tests::fill_small_vec_8_50_wo_cap    ... bench:          36 ns/iter (+/- 2)

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.