[libc++] Refactor vector<bool> assign functions and add new tests

[winner245]

This PR refactors the assign functions as well as the copy/move-assignment operators for vector<bool>, and augments them with extensive new tests to validate the refactoring.

• assign(size_type __n, const value_type& __x): This new implementation directly performs a reallocation followed by a std::fill_n call, rather than using the copy-and-swap idiom with a reserve() call followed by an assignment via std::fill_n as in the previous implementation. The benefits of this refactoring are two-fold:

  • First, the reserve() call in the previous implementation required copying all existing elements into the newly allocated space, only to be immediately overwritten by the subsequent std::fill_n call. The new implementation skips the redundant copying of old elements (an O(n) operation) by directly assigning to the newly allocated space using std::fill_n, making it approximately twice as fast in scenarios involving reallocation.

  • Second, the copy-and-swap idiom used by reserve and assign involved two additional calls to swap, which are completely avoided in this refactoring.

• __assign_with_size(_ForwardIterator, _Sentinel, difference_type): The new implementation restructures the code by replacing the __construct_at_end() call with the more lightweight std::__copy. While __construct_at_end() performs preliminary work before calling std::__copy, this refactor directly uses std::__copy, eliminating unnecessary preliminary steps.

  Furthermore, the new implementation of __assign_with_size aligns with the copy-assignment operator and is nearly identical to the new implementation of assign(size_type __n, const value_type& __x), with the only difference being the use of std::__copy instead of std::fill_n. This refactor ensures consistency across all assign functions.

• Copy-assignment operator: Refactored to call __assign_with_size directly, which is exactly equivalent to the previous implementation but avoids code duplication.

• Move-assignment operator: Instead of calling assign, it now directly calls the private helper function __assign_with_size, saving some indirections.

With the new tests added in this PR for the assignment operators and those newly added in #119163 for the assign functions, the validity of the refactored functions are tested in all possible code paths.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

This PR refactors the assign functions as well as the copy/move-assignment operators for vector<bool>, and augments them with extensive new tests to validate the refactoring.

• assign(size_type __n, const value_type& __x): Instead of using the copy-and-swap idiom inside a reserve() call followed by assignments via std::fill_n in the previous implementation, this new implementation directly performs an reallocation followed by a std::fill_n call. The beneit of this refactoring is two-folds: first, the reserve() call has to copy-over all existing elements into the reallocated new space, and then immediately overwrite them by the assignents call to std::fill_n, whereas the new implementation only needs to reallocate new space and then do the assignents by std::fill_n, without needing to copy-over old elements (which is an O(n) operation). Moreover, the copy-swap idiom used by reserve and assign requires 2 extra calls to the swap, which is completely avoided in this refacotring.

• __assign_with_size(_ForwardIterator, _Sentinel, difference_type): The main change in this refactor is replacing the __construct_at_end() call with std::__copy. While the __construct_at_end() function internally performs some preliminary work and finally calls std::__copy, this refactor saves the preliminary work and directly calls std::__copy. Moreover, in terms of code clarity, the new implementation of __assign_with_size is almost identical to the new implementation of assign(size_type __n, const value_type& __x), with the only difference being std::__copy instead of std::fill_n. This makes both assign functions consistent in terms of implementation.

• Copy-assignment operator: Refactored to call __assign_with_size directly, which is exactly equivalent to the previous implementation but avoids code duplication.

• Move-assignment operator: Instead of calling assign, it now directly calls the private helper function __assign_with_size, saving some indirections.

With the new tests added in this PR for the assignment operators and those in #119163 for the assign functions, the validity of the refactored functions are tested in all possible code paths.

---

Full diff: https://github.com/llvm/llvm-project/pull/119502.diff

3 Files Affected:

• (modified) libcxx/include/__vector/vector_bool.h (+9-23)
• (modified) libcxx/test/std/containers/sequences/vector.bool/assign_copy.pass.cpp (+51-32)
• (modified) libcxx/test/std/containers/sequences/vector.bool/assign_move.pass.cpp (+100-63)

diff --git a/libcxx/include/__vector/vector_bool.h b/libcxx/include/__vector/vector_bool.h
index 36eb7f350ac406..a5f0eb0751e0f3 100644
--- a/libcxx/include/__vector/vector_bool.h
+++ b/libcxx/include/__vector/vector_bool.h
@@ -699,14 +699,7 @@ template <class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 vector<bool, _Allocator>& vector<bool, _Allocator>::operator=(const vector& __v) {
   if (this != std::addressof(__v)) {
     __copy_assign_alloc(__v);
-    if (__v.__size_) {
-      if (__v.__size_ > capacity()) {
-        __vdeallocate();
-        __vallocate(__v.__size_);
-      }
-      std::copy(__v.__begin_, __v.__begin_ + __external_cap_to_internal(__v.__size_), __begin_);
-    }
-    __size_ = __v.__size_;
+    __assign_with_size(__v.begin(), __v.end(), __v.size());
   }
   return *this;
 }
@@ -754,7 +747,7 @@ vector<bool, _Allocator>::operator=(vector&& __v)
 template <class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<bool, _Allocator>::__move_assign(vector& __c, false_type) {
   if (__alloc_ != __c.__alloc_)
-    assign(__c.begin(), __c.end());
+    __assign_with_size(__c.begin(), __c.end(), __c.size());
   else
     __move_assign(__c, true_type());
 }
@@ -773,19 +766,14 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<bool, _Allocator>::__move_assign(vecto
 
 template <class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<bool, _Allocator>::assign(size_type __n, const value_type& __x) {
-  __size_ = 0;
   if (__n > 0) {
-    size_type __c = capacity();
-    if (__n <= __c)
-      __size_ = __n;
-    else {
-      vector __v(get_allocator());
-      __v.reserve(__recommend(__n));
-      __v.__size_ = __n;
-      swap(__v);
+    if (__n > capacity()) {
+      __vdeallocate();
+      __vallocate(__n);
     }
     std::fill_n(begin(), __n, __x);
   }
+  this->__size_ = __n;
 }
 
 template <class _Allocator>
@@ -814,17 +802,15 @@ template <class _ForwardIterator, class _Sentinel>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void
 vector<bool, _Allocator>::__assign_with_size(_ForwardIterator __first, _Sentinel __last, difference_type __ns) {
   _LIBCPP_ASSERT_VALID_INPUT_RANGE(__ns >= 0, "invalid range specified");
-
-  clear();
-
-  const size_t __n = static_cast<size_type>(__ns);
+  const size_type __n = static_cast<size_type>(__ns);
   if (__n) {
     if (__n > capacity()) {
       __vdeallocate();
       __vallocate(__n);
     }
-    __construct_at_end(__first, __last, __n);
+    std::__copy(__first, __last, this->begin());
   }
+  this->__size_ = __n;
 }
 
 template <class _Allocator>
diff --git a/libcxx/test/std/containers/sequences/vector.bool/assign_copy.pass.cpp b/libcxx/test/std/containers/sequences/vector.bool/assign_copy.pass.cpp
index c4866ea4c9b45d..889acb88bfcec7 100644
--- a/libcxx/test/std/containers/sequences/vector.bool/assign_copy.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector.bool/assign_copy.pass.cpp
@@ -10,46 +10,65 @@
 
 // vector& operator=(const vector& c);
 
-#include <vector>
 #include <cassert>
-#include "test_macros.h"
-#include "test_allocator.h"
+#include <vector>
+
 #include "min_allocator.h"
+#include "test_allocator.h"
+#include "test_macros.h"
 
-TEST_CONSTEXPR_CXX20 bool tests()
-{
-    {
-        std::vector<bool, test_allocator<bool> > l(3, true, test_allocator<bool>(5));
-        std::vector<bool, test_allocator<bool> > l2(l, test_allocator<bool>(3));
-        l2 = l;
-        assert(l2 == l);
-        assert(l2.get_allocator() == test_allocator<bool>(3));
-    }
-    {
-        std::vector<bool, other_allocator<bool> > l(3, true, other_allocator<bool>(5));
-        std::vector<bool, other_allocator<bool> > l2(l, other_allocator<bool>(3));
-        l2 = l;
-        assert(l2 == l);
-        assert(l2.get_allocator() == other_allocator<bool>(5));
-    }
+TEST_CONSTEXPR_CXX20 bool tests() {
+  // Test with insufficient space where reallocation occurs during assignment
+  {
+    std::vector<bool, test_allocator<bool> > l(3, true, test_allocator<bool>(5));
+    std::vector<bool, test_allocator<bool> > l2(l, test_allocator<bool>(3));
+    l2 = l;
+    assert(l2 == l);
+    assert(l2.get_allocator() == test_allocator<bool>(3));
+  }
+  {
+    std::vector<bool, other_allocator<bool> > l(3, true, other_allocator<bool>(5));
+    std::vector<bool, other_allocator<bool> > l2(l, other_allocator<bool>(3));
+    l2 = l;
+    assert(l2 == l);
+    assert(l2.get_allocator() == other_allocator<bool>(5));
+  }
 #if TEST_STD_VER >= 11
-    {
-        std::vector<bool, min_allocator<bool> > l(3, true, min_allocator<bool>());
-        std::vector<bool, min_allocator<bool> > l2(l, min_allocator<bool>());
-        l2 = l;
-        assert(l2 == l);
-        assert(l2.get_allocator() == min_allocator<bool>());
-    }
+  {
+    std::vector<bool, min_allocator<bool> > l(3, true, min_allocator<bool>());
+    std::vector<bool, min_allocator<bool> > l2(l, min_allocator<bool>());
+    l2 = l;
+    assert(l2 == l);
+    assert(l2.get_allocator() == min_allocator<bool>());
+  }
 #endif
 
-    return true;
+  // Test with sufficient size where no reallocation occurs during assignment
+  {
+    std::vector<bool, test_allocator<bool> > l(4, true, test_allocator<bool>(5));
+    std::vector<bool, test_allocator<bool> > l2(5, false, test_allocator<bool>(3));
+    l2 = l;
+    assert(l2 == l);
+    assert(l2.get_allocator() == test_allocator<bool>(3));
+  }
+
+  // Test with sufficient spare space where no reallocation occurs during assignment
+  {
+    std::vector<bool, test_allocator<bool> > l(4, true, test_allocator<bool>(5));
+    std::vector<bool, test_allocator<bool> > l2(2, false, test_allocator<bool>(3));
+    l2.reserve(5);
+    l2 = l;
+    assert(l2 == l);
+    assert(l2.get_allocator() == test_allocator<bool>(3));
+  }
+
+  return true;
 }
 
-int main(int, char**)
-{
-    tests();
+int main(int, char**) {
+  tests();
 #if TEST_STD_VER > 17
-    static_assert(tests());
+  static_assert(tests());
 #endif
-    return 0;
+  return 0;
 }
diff --git a/libcxx/test/std/containers/sequences/vector.bool/assign_move.pass.cpp b/libcxx/test/std/containers/sequences/vector.bool/assign_move.pass.cpp
index 10271efc3f4038..e047807d68f735 100644
--- a/libcxx/test/std/containers/sequences/vector.bool/assign_move.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector.bool/assign_move.pass.cpp
@@ -12,79 +12,116 @@
 
 // vector& operator=(vector&& c);
 
-#include <vector>
 #include <cassert>
-#include "test_macros.h"
-#include "test_allocator.h"
+#include <vector>
+
 #include "min_allocator.h"
+#include "test_allocator.h"
+#include "test_macros.h"
 
-TEST_CONSTEXPR_CXX20 bool tests()
-{
-    {
-        std::vector<bool, test_allocator<bool> > l(test_allocator<bool>(5));
-        std::vector<bool, test_allocator<bool> > lo(test_allocator<bool>(5));
-        for (int i = 1; i <= 3; ++i)
-        {
-            l.push_back(i);
-            lo.push_back(i);
-        }
-        std::vector<bool, test_allocator<bool> > l2(test_allocator<bool>(5));
-        l2 = std::move(l);
-        assert(l2 == lo);
-        LIBCPP_ASSERT(l.empty());
-        assert(l2.get_allocator() == lo.get_allocator());
+TEST_CONSTEXPR_CXX20 bool tests() {
+  //
+  // Testing for O(1) ownership move
+  //
+  { // Test with pocma = true_type, thus performing an ownership move.
+    std::vector<bool, other_allocator<bool> > l(other_allocator<bool>(5));
+    std::vector<bool, other_allocator<bool> > lo(other_allocator<bool>(5));
+    std::vector<bool, other_allocator<bool> > l2(other_allocator<bool>(6));
+    for (int i = 1; i <= 3; ++i) {
+      l.push_back(i);
+      lo.push_back(i);
+    }
+    l2 = std::move(l);
+    assert(l2 == lo);
+    assert(l.empty());
+    assert(l2.get_allocator() == lo.get_allocator());
+  }
+  { // Test with pocma = false_type and equal allocators, thus performing an ownership move.
+    std::vector<bool, test_allocator<bool> > l(test_allocator<bool>(5));
+    std::vector<bool, test_allocator<bool> > lo(test_allocator<bool>(5));
+    std::vector<bool, test_allocator<bool> > l2(test_allocator<bool>(5));
+    for (int i = 1; i <= 3; ++i) {
+      l.push_back(i);
+      lo.push_back(i);
     }
-    {
-        std::vector<bool, test_allocator<bool> > l(test_allocator<bool>(5));
-        std::vector<bool, test_allocator<bool> > lo(test_allocator<bool>(5));
-        for (int i = 1; i <= 3; ++i)
-        {
-            l.push_back(i);
-            lo.push_back(i);
-        }
-        std::vector<bool, test_allocator<bool> > l2(test_allocator<bool>(6));
-        l2 = std::move(l);
-        assert(l2 == lo);
-        assert(!l.empty());
-        assert(l2.get_allocator() == test_allocator<bool>(6));
+    l2 = std::move(l);
+    assert(l2 == lo);
+    LIBCPP_ASSERT(l.empty());
+    assert(l2.get_allocator() == lo.get_allocator());
+  }
+  { // Test with pocma = false_type and equal allocators, thus performing an ownership move.
+    std::vector<bool, min_allocator<bool> > l(min_allocator<bool>{});
+    std::vector<bool, min_allocator<bool> > lo(min_allocator<bool>{});
+    std::vector<bool, min_allocator<bool> > l2(min_allocator<bool>{});
+    for (int i = 1; i <= 3; ++i) {
+      l.push_back(i);
+      lo.push_back(i);
     }
-    {
-        std::vector<bool, other_allocator<bool> > l(other_allocator<bool>(5));
-        std::vector<bool, other_allocator<bool> > lo(other_allocator<bool>(5));
-        for (int i = 1; i <= 3; ++i)
-        {
-            l.push_back(i);
-            lo.push_back(i);
-        }
-        std::vector<bool, other_allocator<bool> > l2(other_allocator<bool>(6));
-        l2 = std::move(l);
-        assert(l2 == lo);
-        assert(l.empty());
-        assert(l2.get_allocator() == lo.get_allocator());
+    l2 = std::move(l);
+    assert(l2 == lo);
+    assert(l.empty());
+    assert(l2.get_allocator() == lo.get_allocator());
+  }
+
+  //
+  // Testing for O(n) element-wise move
+  //
+  { // Test with pocma = false_type and unequal allocators, thus performing an element-wise move.
+    // Reallocation occurs during the element-wise move due to insufficient space.
+    std::vector<bool, test_allocator<bool> > l(test_allocator<bool>(5));
+    std::vector<bool, test_allocator<bool> > lo(test_allocator<bool>(5));
+    std::vector<bool, test_allocator<bool> > l2(test_allocator<bool>(6));
+    for (int i = 1; i <= 3; ++i) {
+      l.push_back(i);
+      lo.push_back(i);
+    }
+    l2 = std::move(l);
+    assert(l2 == lo);
+    assert(!l.empty());
+    assert(l2.get_allocator() == test_allocator<bool>(6));
+  }
+
+  { // Test with pocma = false_type and unequal allocators, thus performing an element-wise move.
+    // No reallocation occurs since source data fits within current size.
+    std::vector<bool, test_allocator<bool> > l(test_allocator<bool>(5));
+    std::vector<bool, test_allocator<bool> > lo(test_allocator<bool>(5));
+    std::vector<bool, test_allocator<bool> > l2(test_allocator<bool>(6));
+    for (int i = 1; i <= 3; ++i) {
+      l.push_back(i);
+      lo.push_back(i);
     }
-    {
-        std::vector<bool, min_allocator<bool> > l(min_allocator<bool>{});
-        std::vector<bool, min_allocator<bool> > lo(min_allocator<bool>{});
-        for (int i = 1; i <= 3; ++i)
-        {
-            l.push_back(i);
-            lo.push_back(i);
-        }
-        std::vector<bool, min_allocator<bool> > l2(min_allocator<bool>{});
-        l2 = std::move(l);
-        assert(l2 == lo);
-        assert(l.empty());
-        assert(l2.get_allocator() == lo.get_allocator());
+    for (int i = 1; i <= 5; ++i)
+      l2.push_back(i);
+    l2 = std::move(l);
+    assert(l2 == lo);
+    assert(!l.empty());
+    assert(l2.get_allocator() == test_allocator<bool>(6));
+  }
+  { // Test with pocma = false_type and unequal allocators, thus performing an element-wise move.
+    // No reallocation occurs since source data fits within current spare space.
+    std::vector<bool, test_allocator<bool> > l(test_allocator<bool>(5));
+    std::vector<bool, test_allocator<bool> > lo(test_allocator<bool>(5));
+    std::vector<bool, test_allocator<bool> > l2(test_allocator<bool>(6));
+    for (int i = 1; i <= 3; ++i) {
+      l.push_back(i);
+      lo.push_back(i);
     }
+    for (int i = 1; i <= 2; ++i)
+      l2.push_back(i);
+    l2.reserve(5);
+    l2 = std::move(l);
+    assert(l2 == lo);
+    assert(!l.empty());
+    assert(l2.get_allocator() == test_allocator<bool>(6));
+  }
 
-    return true;
+  return true;
 }
 
-int main(int, char**)
-{
-    tests();
+int main(int, char**) {
+  tests();
 #if TEST_STD_VER > 17
-    static_assert(tests());
+  static_assert(tests());
 #endif
-    return 0;
+  return 0;
 }

[winner245]

The old tests didn't comply with clang-format, which would cause the CI to fail. Thus, I had to apply clang-format to the old tests, mixing up the re-formatting with the new additions, making the review process a bit more complicated. I wish I could have run clang-format for the old tests separately.

[ldionne]

Why was this closed?

[winner245]

I've made further performance improvements that enhance not only the assign functions mentioned in this PR but also other operations such as copy/move constructors, insert, {assign, append, insert}_range, and reserve functions using bit manipulation techniques. Therefore, I closed this PR for now and will present the performance improvements in a more unified manner. I may need to split the improvements into smaller, self-contained PRs to ensure they are easier to review.