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.

Sign up for GitHub

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

Optimize implementations of FromIterator and Extend for Vec #22681

Merged
merged 4 commits into from
Jun 17, 2015
Merged

Optimize implementations of FromIterator and Extend for Vec #22681

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a118b93
Optimize Vec::from_iter and extend
mzabaluev Feb 22, 2015
032804b
In Vec::from_iter, unroll the first iteration
mzabaluev Feb 22, 2015
f9b70ac
Desugar the implementation of extend to work with Iterator
mzabaluev Feb 22, 2015
7b464d3
Use while let
mzabaluev Feb 22, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 39 additions & 39 deletions src/libcollections/vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1410,42 +1410,26 @@ impl<T> ops::DerefMut for Vec<T> {
impl<T> FromIterator<T> for Vec<T> {
#[inline]
fn from_iter<I: IntoIterator<Item=T>>(iterable: I) -> Vec<T> {
// Unroll the first iteration, as the vector is going to be
// expanded on this iteration in every case when the iterable is not
// empty, but the loop in extend_desugared() is not going to see the
// vector being full in the few subsequent loop iterations.
// So we get better branch prediction and the possibility to
// construct the vector with initial estimated capacity.
let mut iterator = iterable.into_iter();
let (lower, _) = iterator.size_hint();
let mut vector = Vec::with_capacity(lower);

// This function should be the moral equivalent of:
//
// for item in iterator {
// vector.push(item);
// }
//
// This equivalent crucially runs the iterator precisely once. Below we
// actually in theory run the iterator twice (one without bounds checks
// and one with). To achieve the "moral equivalent", we use the `if`
// statement below to break out early.
//
// If the first loop has terminated, then we have one of two conditions.
//
// 1. The underlying iterator returned `None`. In this case we are
// guaranteed that less than `vector.capacity()` elements have been
// returned, so we break out early.
// 2. The underlying iterator yielded `vector.capacity()` elements and
// has not yielded `None` yet. In this case we run the iterator to
// its end below.
for element in iterator.by_ref().take(vector.capacity()) {
let len = vector.len();
unsafe {
ptr::write(vector.get_unchecked_mut(len), element);
vector.set_len(len + 1);
}
}

if vector.len() == vector.capacity() {
for element in iterator {
vector.push(element);
let mut vector = match iterator.next() {
Copy link
Member

Choose a reason for hiding this comment

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

Hm, does this actually improve performance, over the simpler:

let mut vector = Vec::with_capacity(iterator.size_hint().0);
vector.extend_desugared(iterator);
vector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This avoids a branch that is present in extend_desugared. That branch tends to be not taken, but in the first iteration the vector is always expanded.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, why it does it avoid that branch? It seems that if the iterator has a non-zero size hint the with_capacity will ensure that it isn't taken, and in the case that the iterator has a zero-size hint it seems both with and without this are equally bad off?

Basically, I'm asking if this was noticeable in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, and it was somehow lost on me that Vec::with_capacity(1) allocates exactly one element. I should simplify the code to your suggestion; the performance difference will likely be negligible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the suggested change, vec::bench_from_iter_0000 regresses
from 29 ns/iter (+/- 2) to 87 ns/iter (+/- 48) in my testing
(rebased against commit 3dbfa74), the other benchmarks seemingly unaffected. I wonder if it can be considered an edge case.

None => return Vec::new(),
Some(element) => {
let (lower, _) = iterator.size_hint();
let mut vector = Vec::with_capacity(1 + lower);
unsafe {
ptr::write(vector.get_unchecked_mut(0), element);
vector.set_len(1);
}
vector
}
}
};
vector.extend_desugared(iterator);
vector
}
}
Expand Down Expand Up @@ -1484,11 +1468,27 @@ impl<'a, T> IntoIterator for &'a mut Vec<T> {
impl<T> Extend<T> for Vec<T> {
#[inline]
fn extend<I: IntoIterator<Item=T>>(&mut self, iterable: I) {
let iterator = iterable.into_iter();
let (lower, _) = iterator.size_hint();
self.reserve(lower);
for element in iterator {
self.push(element)
self.extend_desugared(iterable.into_iter())
}
}

impl<T> Vec<T> {
fn extend_desugared<I: Iterator<Item=T>>(&mut self, mut iterator: I) {
// This function should be the moral equivalent of:
//
// for item in iterator {
// self.push(item);
// }
while let Some(element) = iterator.next() {
let len = self.len();
if len == self.capacity() {
let (lower, _) = iterator.size_hint();
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling size_hint in a loop seems really bad. This is not necessarily a straight-forward or cheap method. Is hoisting it out not worth it in your testing?

Copy link
Member

Choose a reason for hiding this comment

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

It is called exponentially rarely, so it seems fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh derp, sure.

self.reserve(lower + 1);
}
unsafe {
ptr::write(self.get_unchecked_mut(len), element);
self.set_len(len + 1);
}
}
}
}
Expand Down