From a118b936faa7f802c41214cb908622a7893bebfd Mon Sep 17 00:00:00 2001 From: Mikhail Zabaluev Date: Sun, 22 Feb 2015 17:22:48 +0200 Subject: [PATCH 1/4] Optimize Vec::from_iter and extend Use one loop, efficient for both sized and size-ignorant iterators (including iterators lying about their size). --- src/libcollections/vec.rs | 68 +++++++++++++++------------------------ 1 file changed, 26 insertions(+), 42 deletions(-) diff --git a/src/libcollections/vec.rs b/src/libcollections/vec.rs index 2f9577c08deba..cb1199a59f608 100644 --- a/src/libcollections/vec.rs +++ b/src/libcollections/vec.rs @@ -1410,42 +1410,8 @@ impl ops::DerefMut for Vec { impl FromIterator for Vec { #[inline] fn from_iter>(iterable: I) -> Vec { - 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 = Vec::new(); + vector.extend(iterable); vector } } @@ -1482,13 +1448,31 @@ impl<'a, T> IntoIterator for &'a mut Vec { #[unstable(feature = "collections", reason = "waiting on Extend stability")] impl Extend for Vec { - #[inline] fn extend>(&mut self, iterable: I) { - let iterator = iterable.into_iter(); - let (lower, _) = iterator.size_hint(); - self.reserve(lower); - for element in iterator { - self.push(element) + let mut iterator = iterable.into_iter(); + + // This function should be the moral equivalent of: + // + // for item in iterator { + // self.push(item); + // } + loop { + match iterator.next() { + None => { + break; + } + Some(element) => { + let len = self.len(); + if len == self.capacity() { + let (lower, _) = iterator.size_hint(); + self.reserve(lower + 1); + } + unsafe { + ptr::write(self.get_unchecked_mut(len), element); + self.set_len(len + 1); + } + } + } } } } From 032804bf68e2ac13add895206f2173409acff836 Mon Sep 17 00:00:00 2001 From: Mikhail Zabaluev Date: Sun, 22 Feb 2015 19:20:18 +0200 Subject: [PATCH 2/4] In Vec::from_iter, unroll the first iteration For the first ever element to put into a vector, the branching conditions are more predictable. --- src/libcollections/vec.rs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/libcollections/vec.rs b/src/libcollections/vec.rs index cb1199a59f608..3db55b82fe84a 100644 --- a/src/libcollections/vec.rs +++ b/src/libcollections/vec.rs @@ -1410,7 +1410,25 @@ impl ops::DerefMut for Vec { impl FromIterator for Vec { #[inline] fn from_iter>(iterable: I) -> Vec { - let mut vector = Vec::new(); + // 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 mut vector = match iterator.next() { + 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(iterable); vector } From f9b70acfd4167f5911c1be4e2dc960c370adb266 Mon Sep 17 00:00:00 2001 From: Mikhail Zabaluev Date: Sun, 22 Feb 2015 19:22:50 +0200 Subject: [PATCH 3/4] Desugar the implementation of extend to work with Iterator Implement both Vec::from_iter and extend in terms of an internal method working with Iterator. Otherwise, the code below ends up using two monomorphizations of extend, differing only in the implementation of IntoIterator: let mut v = Vector::from_iter(iterable1); v.extend(iterable2); --- src/libcollections/vec.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libcollections/vec.rs b/src/libcollections/vec.rs index 3db55b82fe84a..3ed9c154e47a4 100644 --- a/src/libcollections/vec.rs +++ b/src/libcollections/vec.rs @@ -1429,7 +1429,7 @@ impl FromIterator for Vec { vector } }; - vector.extend(iterable); + vector.extend_desugared(iterator); vector } } @@ -1466,9 +1466,14 @@ impl<'a, T> IntoIterator for &'a mut Vec { #[unstable(feature = "collections", reason = "waiting on Extend stability")] impl Extend for Vec { + #[inline] fn extend>(&mut self, iterable: I) { - let mut iterator = iterable.into_iter(); + self.extend_desugared(iterable.into_iter()) + } +} +impl Vec { + fn extend_desugared>(&mut self, mut iterator: I) { // This function should be the moral equivalent of: // // for item in iterator { From 7b464d364b33915b88f67213a3329e0a3d997e4d Mon Sep 17 00:00:00 2001 From: Mikhail Zabaluev Date: Sun, 22 Feb 2015 23:46:36 +0200 Subject: [PATCH 4/4] Use while let --- src/libcollections/vec.rs | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/src/libcollections/vec.rs b/src/libcollections/vec.rs index 3ed9c154e47a4..8e87754274605 100644 --- a/src/libcollections/vec.rs +++ b/src/libcollections/vec.rs @@ -1479,22 +1479,15 @@ impl Vec { // for item in iterator { // self.push(item); // } - loop { - match iterator.next() { - None => { - break; - } - Some(element) => { - let len = self.len(); - if len == self.capacity() { - let (lower, _) = iterator.size_hint(); - self.reserve(lower + 1); - } - unsafe { - ptr::write(self.get_unchecked_mut(len), element); - self.set_len(len + 1); - } - } + while let Some(element) = iterator.next() { + let len = self.len(); + if len == self.capacity() { + let (lower, _) = iterator.size_hint(); + self.reserve(lower + 1); + } + unsafe { + ptr::write(self.get_unchecked_mut(len), element); + self.set_len(len + 1); } } }