From b4ba77dba77ec5039bcea2c9ccba2230f361955d Mon Sep 17 00:00:00 2001
From: Simon Hausmann <simon.hausmann@sixtyfps.io>
Date: Fri, 6 Aug 2021 12:05:23 +0200
Subject: [PATCH] Fix SharedVector creation from iterators with default size
 hints

* In capacity_for_grow, don't compare the number of elements
  (current_cap) with the size of an element in bytes, but
  with the required capacity instead (to detect that we don't need
  to grow). Not all call sites check for that (i.e. push), so there's
  a new test for that.

* When re-allocating due to growth and copying elements from the old
  inner to the new inner, make sure to copy all old elements from the beginning,
  not only the last element repeatedly.
---
 CHANGELOG.md                             |  1 +
 sixtyfps_runtime/corelib/sharedvector.rs | 47 +++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 04fef2b73..c2993a8a0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -30,6 +30,7 @@ All notable changes to this project will be documented in this file.
  - Support of `*=` and `/=` on types with unit such as length.
  - Proper compilation error when using a self assignment operator on an invalid type instead of a panic
  - Angle conversion for values specified in radians, gradians and turns
+ - SharedVector was sometimes not allocating big enough storage.
 
 ## [0.1.0] - 2021-06-30
 
diff --git a/sixtyfps_runtime/corelib/sharedvector.rs b/sixtyfps_runtime/corelib/sharedvector.rs
index 1ef6bf0c4..53f28ac85 100644
--- a/sixtyfps_runtime/corelib/sharedvector.rs
+++ b/sixtyfps_runtime/corelib/sharedvector.rs
@@ -65,7 +65,7 @@ fn alloc_with_capacity<T>(capacity: usize) -> NonNull<SharedVectorInner<T>> {
 /// Return a new capacity suitable for this vector
 /// Loosely based on alloc::raw_vec::RawVec::grow_amortized.
 fn capacity_for_grow(current_cap: usize, required_cap: usize, elem_size: usize) -> usize {
-    if current_cap >= elem_size {
+    if current_cap >= required_cap {
         return current_cap;
     }
     let cap = core::cmp::max(current_cap * 2, required_cap);
@@ -315,7 +315,7 @@ impl<T> FromIterator<T> for SharedVector<T> {
                         while *begin < size {
                             unsafe {
                                 core::ptr::write(
-                                    result.inner.as_mut().data.as_mut_ptr().add(size),
+                                    result.inner.as_mut().data.as_mut_ptr().add(*begin),
                                     core::ptr::read(old_inner.as_ref().data.as_ptr().add(*begin)),
                                 );
                                 *begin += 1;
@@ -483,6 +483,49 @@ fn invalid_capacity_test() {
     let _: SharedVector<u8> = SharedVector::with_capacity(usize::MAX / 2 - 1000);
 }
 
+#[test]
+fn collect_from_iter_with_no_size_hint() {
+    struct NoSizeHintIter<'a> {
+        data: &'a [&'a str],
+        i: usize,
+    }
+
+    impl<'a> Iterator for NoSizeHintIter<'a> {
+        type Item = String;
+
+        fn next(&mut self) -> Option<Self::Item> {
+            if self.i >= self.data.len() {
+                return None;
+            }
+            let item = self.data[self.i];
+            self.i += 1;
+            Some(item.to_string())
+        }
+
+        fn size_hint(&self) -> (usize, Option<usize>) {
+            (0, None)
+        }
+    }
+
+    // 5 elements to be above the initial "grow"-capacity of 4 and thus require one realloc.
+    let input = NoSizeHintIter { data: &["Hello", "sweet", "world", "of", "iterators"], i: 0 };
+
+    let shared_vec: SharedVector<String> = input.collect();
+    assert_eq!(shared_vec.as_slice(), &["Hello", "sweet", "world", "of", "iterators"]);
+}
+
+#[test]
+fn test_capacity_grows_only_when_needed() {
+    let mut vec: SharedVector<u8> = SharedVector::with_capacity(2);
+    vec.push(0);
+    assert_eq!(vec.capacity(), 2);
+    vec.push(0);
+    assert_eq!(vec.capacity(), 2);
+    vec.push(0);
+    assert_eq!(vec.len(), 3);
+    assert!(vec.capacity() > 2);
+}
+
 #[cfg(feature = "ffi")]
 pub(crate) mod ffi {
     use super::*;