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

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

Merged
merged 2 commits into from Aug 1, 2018

Conversation

Projects
None yet
5 participants
@ljedrz
Contributor

ljedrz commented Jul 30, 2018

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.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Jul 30, 2018

r? @varkor

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

@rust-highfive

This comment was marked as outdated.

Collaborator

rust-highfive commented Jul 30, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:21:27] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:21:28] 
[00:21:28] error: internal compiler error: unexpected panic
[00:21:28] 
[00:21:28] note: the compiler unexpectedly panicked. this is a bug.
[00:21:28] 
[00:21:28] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:21:28] note: rustc 1.29.0-dev running on x86_64-unknown-linux-gnu
[00:21:28] 
[00:21:28] 
[00:21:28] note: compiler flags: -Z force-unstable-if-unmarked -C opt-level=2 -C prefer-dynamic -C debug-assertions=y -C link-args=-Wl,-rpath,$ORIGIN/../lib --crate-type lib
[00:21:28] 
[00:21:28] note: some of the compiler flags provided by cargo are hidden
[00:21:28] error: Could not compile `core`.
[00:21:28] 
[00:21:28] Caused by:
[00:21:28]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name core libcore/lib.rs --color always --error-format json --crate-type lib --emit=dep-info,link -C opt-level=2 -C metadata=1cbcabaa1ea822b5 -C extra-filename=-1cbcabaa1ea822b5 --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/release/deps` (exit code: 101)
---
157076 ./.git/modules/src
149124 ./src/llvm-emscripten/test
145460 ./obj/build/bootstrap/debug/incremental
130592 ./obj/build/bootstrap/debug/incremental/bootstrap-c7ee2tfsizs
130588 ./obj/build/bootstrap/debug/incremental/bootstrap-c7ee2tfsizs/s-f3dyotjymq-eow6d-3t5kexjst7huj
97532 ./obj/build/x86_64-unknown-linux-gnu/stage0/lib/rustlib/x86_64-unknown-linux-gnu/codegen-backends
89096 ./obj/build/x86_64-unknown-linux-gnu/stage1
89072 ./obj/build/x86_64-unknown-linux-gnu/stage1/lib
77604 ./.git/modules/src/tools
---
travis_time:end:0773f3cd:start=1532958086961119093,finish=1532958086968300925,duration=7181832
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:165e2760
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0cc7ec7e
travis_time:start:0cc7ec7e
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:09aaa0ce
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Jul 30, 2018

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+

@bors

This comment has been minimized.

Contributor

bors commented Jul 30, 2018

📌 Commit c5dfc2d has been approved by Mark-Simulacrum

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Jul 30, 2018

Oh, Travis failed.

@bors r-

@ljedrz

This comment has been minimized.

Contributor

ljedrz commented Jul 30, 2018

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

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Jul 30, 2018

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.

@ljedrz

This comment has been minimized.

Contributor

ljedrz commented Jul 30, 2018

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.

@ljedrz ljedrz force-pushed the ljedrz:smallvec_true_extend branch from c5dfc2d to 9169934 Jul 30, 2018

@ljedrz

This comment has been minimized.

Contributor

ljedrz commented Jul 30, 2018

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

let iter = iter.into_iter();
self.reserve(iter.size_hint().0);
for el in iter {

This comment has been minimized.

@Mark-Simulacrum

Mark-Simulacrum Jul 30, 2018

Member

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.

This comment has been minimized.

@ljedrz

ljedrz Jul 31, 2018

Contributor

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.

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Jul 30, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Jul 30, 2018

📌 Commit 9169934 has been approved by Mark-Simulacrum

@ljedrz

This comment has been minimized.

Contributor

ljedrz commented Jul 31, 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)
@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Jul 31, 2018

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

@ljedrz

This comment has been minimized.

Contributor

ljedrz commented Jul 31, 2018

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

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Jul 31, 2018

Yes, that should work.

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Jul 31, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Jul 31, 2018

📌 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

Rollup merge of rust-lang#52859 - ljedrz:smallvec_true_extend, r=Mark…
…-Simulacrum

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

As calculated in rust-lang#52738, `Vec::extend` is much faster than `push`ing 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

Auto merge of #52931 - Mark-Simulacrum:rollup, r=Mark-Simulacrum
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<LocalWaker>)
 - #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

Rollup merge of rust-lang#52859 - ljedrz:smallvec_true_extend, r=Mark…
…-Simulacrum

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

As calculated in rust-lang#52738, `Vec::extend` is much faster than `push`ing 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

Auto merge of #52937 - pietroalbini:rollup, r=pietroalbini
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<LocalWaker>)
 - #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

@bors bors merged commit ca52648 into rust-lang:master Aug 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ljedrz ljedrz deleted the ljedrz:smallvec_true_extend branch Aug 1, 2018

llogiq added a commit to llogiq/rust that referenced this pull request Aug 1, 2018

Another SmallVec.extend optimization
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

Rollup merge of rust-lang#52942 - llogiq:smallvec-opt, r=Mark-Simulacrum
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)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment