[libcxx-commits] [libcxx] [libc++] Optimize copy construction and assignment of __tree (PR #151304)

[Louis Dionne via libcxx-commits]

================
@@ -12,286 +12,251 @@
 
 // map& operator=(const map& m);
 
-#include <map>
+// We're testing self-assignment here, so don't warn on that
+// ADDITIONAL_COMPILE_FLAGS(gcc-style-warnings): -Wno-self-assign
+
 #include <algorithm>
 #include <cassert>
 #include <cstdio>
 #include <iterator>
+#include <map>
 #include <vector>
 
 #include "test_macros.h"
 #include "../../../test_compare.h"
 #include "test_allocator.h"
 #include "min_allocator.h"
 
-#if TEST_STD_VER >= 11
-std::vector<int> ca_allocs;
-std::vector<int> ca_deallocs;
-
 template <class T>
-class counting_allocatorT {
-public:
-  typedef T value_type;
-  int foo{0};
-  counting_allocatorT(int f) noexcept : foo(f) {}
+class tracking_allocator {
+  std::vector<void*>* allocs_;
 
-  using propagate_on_container_copy_assignment = std::true_type;
   template <class U>
-  counting_allocatorT(const counting_allocatorT<U>& other) noexcept {
-    foo = other.foo;
-  }
-  template <class U>
-  bool operator==(const counting_allocatorT<U>& other) const noexcept {
-    return foo == other.foo;
-  }
-  template <class U>
-  bool operator!=(const counting_allocatorT<U>& other) const noexcept {
-    return foo != other.foo;
-  }
-
-  T* allocate(std::size_t n) const {
-    ca_allocs.push_back(foo);
-    void* const pv = ::malloc(n * sizeof(T));
-    return static_cast<T*>(pv);
-  }
-  void deallocate(T* p, std::size_t) const noexcept {
-    ca_deallocs.push_back(foo);
-    free(p);
-  }
-};
+  friend class tracking_allocator;
 
-template <class T>
-class counting_allocatorF {
 public:
-  typedef T value_type;
-  int foo{0};
-  counting_allocatorF(int f) noexcept : foo(f) {}
+  using value_type                             = T;
+  using propagate_on_container_copy_assignment = std::true_type;
+
+  tracking_allocator(std::vector<void*>& allocs) : allocs_(&allocs) {}
 
-  using propagate_on_container_copy_assignment = std::false_type;
-  template <class U>
-  counting_allocatorF(const counting_allocatorF<U>& other) noexcept {
-    foo = other.foo;
-  }
   template <class U>
-  bool operator==(const counting_allocatorF<U>& other) const noexcept {
-    return foo == other.foo;
+  tracking_allocator(const tracking_allocator<U>& other) : allocs_(other.allocs_) {}
+
+  T* allocate(std::size_t n) {
+    T* allocation = std::allocator<T>().allocate(n);
+    allocs_->push_back(allocation);
+    return allocation;
   }
-  template <class U>
-  bool operator!=(const counting_allocatorF<U>& other) const noexcept {
-    return foo != other.foo;
+
+  void deallocate(T* ptr, std::size_t n) noexcept {
+    auto res = std::remove(allocs_->begin(), allocs_->end(), ptr);
+    assert(res != allocs_->end() && "Trying to deallocate memory from different allocator?");
+    allocs_->erase(res);
+    std::allocator<T>().deallocate(ptr, n);
   }
 
-  T* allocate(std::size_t n) const {
-    ca_allocs.push_back(foo);
-    void* const pv = ::malloc(n * sizeof(T));
-    return static_cast<T*>(pv);
+  friend bool operator==(const tracking_allocator& lhs, const tracking_allocator& rhs) {
+    return lhs.allocs_ == rhs.allocs_;
   }
-  void deallocate(T* p, std::size_t) const noexcept {
-    ca_deallocs.push_back(foo);
-    free(p);
+
+  friend bool operator!=(const tracking_allocator& lhs, const tracking_allocator& rhs) {
+    return lhs.allocs_ != rhs.allocs_;
   }
 };
 
-bool balanced_allocs() {
-  std::vector<int> temp1, temp2;
-
-  std::printf("Allocations = %zu, deallocations = %zu\n", ca_allocs.size(), ca_deallocs.size());
-  if (ca_allocs.size() != ca_deallocs.size())
-    return false;
-
-  temp1 = ca_allocs;
-  std::sort(temp1.begin(), temp1.end());
-  temp2.clear();
-  std::unique_copy(temp1.begin(), temp1.end(), std::back_inserter<std::vector<int>>(temp2));
-  std::printf("There were %zu different allocators\n", temp2.size());
-
-  for (std::vector<int>::const_iterator it = temp2.begin(); it != temp2.end(); ++it) {
-    std::ptrdiff_t const allocs   = std::count(ca_allocs.begin(), ca_allocs.end(), *it);
-    std::ptrdiff_t const deallocs = std::count(ca_deallocs.begin(), ca_deallocs.end(), *it);
-    std::printf("%d: %td vs %td\n", *it, allocs, deallocs);
-    if (allocs != deallocs)
-      return false;
+struct NoOp {
+  void operator()() {}
+};
+
+template <class Alloc, class AllocatorInvariant = NoOp>
+void test_alloc(const Alloc& lhs_alloc                   = Alloc(),
+                const Alloc& rhs_alloc                   = Alloc(),
+                AllocatorInvariant check_alloc_invariant = NoOp()) {
+  {   // Test empty/non-empy map combinations
+    { // assign from a non-empty container into an empty one
+      using V = std::pair<const int, int>;
+      V arr[] = {V(1, 1), V(2, 3), V(3, 6)};
+      std::map<int, int, std::less<int>, Alloc> orig(begin(arr), end(arr), rhs_alloc);
+      std::map<int, int, std::less<int>, Alloc> copy(lhs_alloc);
+      copy = orig;
+      assert(copy.size() == 3);
+      assert(*std::next(copy.begin(), 0) == V(1, 1));
+      assert(*std::next(copy.begin(), 1) == V(2, 3));
+      assert(*std::next(copy.begin(), 2) == V(3, 6));
+      assert(std::next(copy.begin(), 3) == copy.end());
+
+      // Check that orig is still what is expected
+      assert(orig.size() == 3);
+      assert(*std::next(orig.begin(), 0) == V(1, 1));
+      assert(*std::next(orig.begin(), 1) == V(2, 3));
+      assert(*std::next(orig.begin(), 2) == V(3, 6));
+      assert(std::next(orig.begin(), 3) == orig.end());
+    }
+    check_alloc_invariant();
+    { // assign from an empty container into an empty one
+      std::map<int, int, std::less<int>, Alloc> orig(rhs_alloc);
+      std::map<int, int, std::less<int>, Alloc> copy(lhs_alloc);
+      copy = orig;
+      assert(copy.size() == 0);
+      assert(copy.begin() == copy.end());
+
+      // Check that orig is still what is expected
+      assert(orig.size() == 0);
+      assert(orig.begin() == orig.end());
+    }
+    check_alloc_invariant();
+    { // assign from an empty container into a non-empty one
+      using V = std::pair<const int, int>;
+      V arr[] = {V(1, 1), V(2, 3), V(3, 6)};
+      std::map<int, int, std::less<int>, Alloc> orig(rhs_alloc);
+      std::map<int, int, std::less<int>, Alloc> copy(begin(arr), end(arr), rhs_alloc);
+      copy = orig;
+      assert(copy.size() == 0);
+      assert(copy.begin() == copy.end());
+
+      // Check that orig is still what is expected
+      assert(orig.size() == 0);
+      assert(orig.begin() == orig.end());
+    }
   }
 
-  temp1 = ca_allocs;
-  std::sort(temp1.begin(), temp1.end());
-  temp2.clear();
-  std::unique_copy(temp1.begin(), temp1.end(), std::back_inserter<std::vector<int>>(temp2));
-  std::printf("There were %zu different (de)allocators\n", temp2.size());
-
-  for (std::vector<int>::const_iterator it = ca_deallocs.begin(); it != ca_deallocs.end(); ++it) {
-    std::ptrdiff_t const allocs   = std::count(ca_allocs.begin(), ca_allocs.end(), *it);
-    std::ptrdiff_t const deallocs = std::count(ca_deallocs.begin(), ca_deallocs.end(), *it);
-    std::printf("%d: %td vs %td\n", *it, allocs, deallocs);
-    if (allocs != deallocs)
-      return false;
+  {   // Ensure that self-assignment works correctly
+    { // with a non-empty map
+      using V = std::pair<const int, int>;
+      V arr[] = {V(1, 1), V(2, 3), V(3, 6)};
+      std::map<int, int, std::less<int>, Alloc> orig(begin(arr), end(arr), rhs_alloc);
+      orig = orig;
+
+      assert(orig.size() == 3);
+      assert(*std::next(orig.begin(), 0) == V(1, 1));
+      assert(*std::next(orig.begin(), 1) == V(2, 3));
+      assert(*std::next(orig.begin(), 2) == V(3, 6));
+      assert(std::next(orig.begin(), 3) == orig.end());
+    }
+    { // with an empty map
+      std::map<int, int, std::less<int>, Alloc> orig(rhs_alloc);
+      orig = orig;
+
+      assert(orig.size() == 0);
+      assert(orig.begin() == orig.end());
+    }
   }
 
-  return true;
-}
-#endif
+  { // check assignment into a non-empty map
+    check_alloc_invariant();
+    { // LHS already contains, but fewer than needed
----------------
ldionne wrote:

```suggestion
    { // LHS already contains, but fewer than the incoming map
```

Below too.

Otherwise, you're referring too much to your optimization and it doesn't make sense out of context.

https://github.com/llvm/llvm-project/pull/151304