Simplify reallocation handling in QList

Have one generic method for detaching and reallocations.
Use that method throughout QList to avoid duplicated
instantiations of code paths that are rarely used.

Change-Id: I5b9add3be5f17b387e2d34028b72c8f52db68444
Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

src/corelib/tools/qarraydataops.h

@@ -1311,60 +1311,37 @@ public:
 public:
     void insert(qsizetype i, qsizetype n, parameter_type t)
     {
-        if (this->needsDetach() || (n > this->freeSpaceAtBegin() && n > this->freeSpaceAtEnd())) {
+        T copy(t);
-            typename Data::GrowthPosition pos = Data::GrowsAtEnd;
-            if (this->size != 0 && i <= (this->size >> 1))
-                pos = Data::GrowsAtBeginning;
 
-            DataPointer detached(DataPointer::allocateGrow(*this, n, pos));
+        typename Data::GrowthPosition pos = Data::GrowsAtEnd;
-            const_iterator where = this->constBegin() + i;
+        if (this->size != 0 && i <= (this->size >> 1))
-            detached->copyAppend(this->constBegin(), where);
+            pos = Data::GrowsAtBeginning;
-            detached->copyAppend(n, t);
+        this->detachAndGrow(pos, n);
-            detached->copyAppend(where, this->constEnd());
+        Q_ASSERT((pos == Data::GrowsAtBeginning && this->freeSpaceAtBegin() >= n) ||
-            this->swap(detached);
+                 (pos == Data::GrowsAtEnd && this->freeSpaceAtEnd() >= n));
-        } else {
+
-            T copy(t);
+        T *where = this->begin() + i;
-            // Insert elements based on the divided distance. Good case: only 1
+        if (pos == QArrayData::GrowsAtBeginning)
-            // insert happens (either to the front part or to the back part). Bad
+            Base::insert(GrowsBackwardsTag{}, where, n, copy);
-            // case: both inserts happen, meaning that we touch all N elements in
+        else
-            // the container (this should be handled "outside" by ensuring enough
+            Base::insert(GrowsForwardTag{}, where, n, copy);
-            // free space by reallocating more frequently)
-            T *where = this->begin() + i;
-            const auto beginSize = sizeToInsertAtBegin(where, n);
-            if (beginSize)
-                Base::insert(GrowsBackwardsTag{}, where, beginSize, copy);
-            if (n - beginSize)
-                Base::insert(GrowsForwardTag{}, where, n - beginSize, copy);
-        }
     }
 
     void insert(qsizetype i, const T *data, qsizetype n)
     {
-        if (this->needsDetach() || (n > this->freeSpaceAtBegin() && n > this->freeSpaceAtEnd())) {
+        typename Data::GrowthPosition pos = Data::GrowsAtEnd;
-            typename Data::GrowthPosition pos = Data::GrowsAtEnd;
+        if (this->size != 0 && i <= (this->size >> 1))
-            if (this->size != 0 && i <= (this->size >> 1))
+            pos = Data::GrowsAtBeginning;
-                pos = Data::GrowsAtBeginning;
+        DataPointer oldData;
-
+        this->detachAndGrow(pos, n, &oldData);
-            DataPointer detached(DataPointer::allocateGrow(*this, n, pos));
+        Q_ASSERT((pos == Data::GrowsAtBeginning && this->freeSpaceAtBegin() >= n) ||
-            auto where = this->constBegin() + i;
+                 (pos == Data::GrowsAtEnd && this->freeSpaceAtEnd() >= n));
-            detached->copyAppend(this->constBegin(), where);
-            detached->copyAppend(data, data + n);
-            detached->copyAppend(where, this->constEnd());
-            this->swap(detached);
-        } else {
-            // Insert elements based on the divided distance. Good case: only 1
-            // insert happens (either to the front part or to the back part). Bad
-            // case: both inserts happen, meaning that we touch all N elements in
-            // the container (this should be handled "outside" by ensuring enough
-            // free space by reallocating more frequently)
-            T *where = this->begin() + i;
-            const auto k = sizeToInsertAtBegin(where, n);
-            if (k)
-                Base::insert(GrowsBackwardsTag{}, where, data, data + k);
-            if (k != n)
-                Base::insert(GrowsForwardTag{}, where, data + k, data + n);
-        }
 
+        T *where = this->begin() + i;
+        if (pos == QArrayData::GrowsAtBeginning)
+            Base::insert(GrowsBackwardsTag{}, where, data, data + n);
+        else
+            Base::insert(GrowsForwardTag{}, where, data, data + n);
     }
 
     template<typename... Args>

src/corelib/tools/qarraydatapointer.h

@@ -162,17 +162,55 @@ public:
         swap(tmp);
     }
 
-    QArrayDataPointer detach()
+    void detach(QArrayDataPointer *old = nullptr)
     {
-        if (needsDetach()) {
+        if (needsDetach())
-            QPair<Data *, T *> copy = clone();
+            reallocateAndGrow(QArrayData::GrowsAtEnd, 0, old);
-            QArrayDataPointer old(d, ptr, size);
+    }
-            d = copy.first;
+
-            ptr = copy.second;
+    // pass in a pointer to a default constructed QADP, to keep it alive beyond the detach() call
-            return old;
+    void detachAndGrow(QArrayData::GrowthPosition where, qsizetype n, QArrayDataPointer *old = nullptr)
+    {
+        if (!needsDetach()) {
+            if (!n ||
+                (where == QArrayData::GrowsAtBeginning && freeSpaceAtBegin() >= n) ||
+                (where == QArrayData::GrowsAtEnd && freeSpaceAtEnd() >= n))
+                return;
+        }
+        reallocateAndGrow(where, n, old);
+    }
+
+    Q_NEVER_INLINE void reallocateAndGrow(QArrayData::GrowthPosition where, qsizetype n, QArrayDataPointer *old = nullptr)
+    {
+        if constexpr (QTypeInfo<T>::isRelocatable && alignof(T) <= alignof(std::max_align_t)) {
+            if (where == QArrayData::GrowsAtEnd && !old && !needsDetach() && n > 0) {
+                (*this)->reallocate(constAllocatedCapacity() - freeSpaceAtEnd() + n, QArrayData::Grow); // fast path
+                return;
+            }
         }
 
-        return QArrayDataPointer();
+        QArrayDataPointer dp(allocateGrow(*this, n, where));
+        if (where == QArrayData::GrowsAtBeginning) {
+            dp.ptr += n;
+            Q_ASSERT(dp.freeSpaceAtBegin() >= n);
+        } else {
+            Q_ASSERT(dp.freeSpaceAtEnd() >= n);
+        }
+        if (size) {
+            qsizetype toCopy = size;
+            if (n < 0)
+                toCopy += n;
+            if (needsDetach() || old)
+                dp->copyAppend(begin(), begin() + toCopy);
+            else
+                dp->moveAppend(begin(), begin() + toCopy);
+            dp.d->flags = flags();
+            Q_ASSERT(dp.size == toCopy);
+        }
+
+        swap(dp);
+        if (old)
+            old->swap(dp);
     }
 
     // forwards from QArrayData
@@ -244,40 +282,6 @@ public:
         return lhs.data() != rhs.data() || lhs.size != rhs.size;
     }
 
-    static void reallocateGrow(QArrayDataPointer &from, qsizetype n)
-    {
-        Q_ASSERT(n > 0);
-
-        if constexpr (!QTypeInfo<T>::isRelocatable || alignof(T) > alignof(std::max_align_t)) {
-            QArrayDataPointer dd(allocateGrow(from, n, QArrayData::GrowsAtEnd));
-            dd->copyAppend(from.data(), from.data() + from.size);
-            from.swap(dd);
-        } else {
-            if (from.needsDetach()) {
-                QArrayDataPointer dd(allocateGrow(from, n, QArrayData::GrowsAtEnd));
-                dd->copyAppend(from.data(), from.data() + from.size);
-                from.swap(dd);
-            } else {
-                from->reallocate(from.constAllocatedCapacity() - from.freeSpaceAtEnd() + n, QArrayData::Grow); // fast path
-            }
-        }
-    }
-
-private:
-    [[nodiscard]] QPair<Data *, T *> clone() const
-    {
-        QPair<Data *, T *> pair = Data::allocate(detachCapacity(size), QArrayData::KeepSize);
-        QArrayDataPointer copy(pair.first, pair.second, 0);
-        if (size)
-            copy->copyAppend(begin(), end());
-
-        if (pair.first)
-            pair.first->flags = flags();
-        copy.d = nullptr;
-        copy.ptr = nullptr;
-        return pair;
-    }
-
 protected:
     Data *d;
     T *ptr;

src/corelib/tools/qlist.h

@@ -385,7 +385,8 @@ public:
     void replace(qsizetype i, parameter_type t)
     {
         Q_ASSERT_X(i >= 0 && i < d->size, "QList<T>::replace", "index out of range");
-        auto oldData = d.detach();
+        DataPointer oldData;
+        d.detach(&oldData);
         d.data()[i] = t;
     }
     void replace(qsizetype i, rvalue_ref t)
@@ -395,7 +396,8 @@ public:
             Q_UNUSED(t);
         } else {
             Q_ASSERT_X(i >= 0 && i < d->size, "QList<T>::replace", "index out of range");
-            auto oldData = d.detach();
+            DataPointer oldData;
+            d.detach(&oldData);
             d.data()[i] = std::move(t);
         }
     }
@@ -603,15 +605,8 @@ inline void QList<T>::resize_internal(qsizetype newSize)
     Q_ASSERT(newSize >= 0);
 
     if (d->needsDetach() || newSize > capacity() - d.freeSpaceAtBegin()) {
-        // must allocate memory
+        d.reallocateAndGrow(QArrayData::GrowsAtEnd, newSize - d.size);
-        DataPointer detached(Data::allocate(d->detachCapacity(newSize)));
+    } else if (newSize < size())
-        qsizetype toCopy = qMin(size(), newSize);
-        if (toCopy)
-            detached->copyAppend(constBegin(), constBegin() + toCopy);
-        d.swap(detached);
-    }
-
-    if (newSize < size())
         d->truncate(newSize);
 }
 
@@ -645,7 +640,10 @@ inline void QList<T>::squeeze()
         // must allocate memory
         DataPointer detached(Data::allocate(size()));
         if (size()) {
-            detached->copyAppend(constBegin(), constEnd());
+            if (d.needsDetach())
+                detached->copyAppend(constBegin(), constEnd());
+            else
+                detached->moveAppend(d.data(), d.data() + d.size);
         }
         d.swap(detached);
     }
@@ -662,9 +660,7 @@ inline void QList<T>::remove(qsizetype i, qsizetype n)
     if (n == 0)
         return;
 
-    if (d->needsDetach())
+    d.detach();
-        d.detach();
-
     d->erase(d->begin() + i, d->begin() + i + n);
 }
 
@@ -672,8 +668,7 @@ template <typename T>
 inline void QList<T>::removeFirst()
 {
     Q_ASSERT(!isEmpty());
-    if (d->needsDetach())
+    d.detach();
-        d.detach();
     d->eraseFirst();
 }
 
@@ -681,8 +676,7 @@ template <typename T>
 inline void QList<T>::removeLast()
 {
     Q_ASSERT(!isEmpty());
-    if (d->needsDetach())
+    d.detach();
-        detach();
     d->eraseLast();
 }
 
@@ -699,39 +693,22 @@ inline void QList<T>::append(const_iterator i1, const_iterator i2)
     if (i1 == i2)
         return;
     const auto distance = std::distance(i1, i2);
-    if (d->needsDetach() || distance > d.freeSpaceAtEnd()) {
+    DataPointer oldData;
-        DataPointer detached(DataPointer::allocateGrow(d, distance, QArrayData::GrowsAtEnd));
+    d.detachAndGrow(QArrayData::GrowsAtEnd, distance, &oldData);
-        detached->copyAppend(constBegin(), constEnd());
+    d->copyAppend(i1, i2);
-        detached->copyAppend(i1, i2);
-        d.swap(detached);
-    } else {
-        // we're detached and we can just move data around
-        d->copyAppend(i1, i2);
-    }
 }
 
 template <typename T>
 inline void QList<T>::append(QList<T> &&other)
 {
+    Q_ASSERT(&other != this);
     if (other.isEmpty())
         return;
     if (other.d->needsDetach() || !std::is_nothrow_move_constructible_v<T>)
         return append(other);
 
-    if (d->needsDetach() || other.size() > d.freeSpaceAtEnd()) {
+    d.detachAndGrow(QArrayData::GrowsAtEnd, other.size());
-        DataPointer detached(DataPointer::allocateGrow(d, other.size(), QArrayData::GrowsAtEnd));
+    d->moveAppend(other.begin(), other.end());
-
-        if (!d->needsDetach())
-            detached->moveAppend(begin(), end());
-        else
-            detached->copyAppend(cbegin(), cend());
-        detached->moveAppend(other.begin(), other.end());
-
-        d.swap(detached);
-    } else {
-        // we're detached and we can just move data around
-        d->moveAppend(other.begin(), other.end());
-    }
 }
 
 template<typename T>
@@ -739,14 +716,10 @@ template<typename... Args>
 inline typename QList<T>::reference QList<T>::emplaceFront(Args &&... args)
 {
     if (d->needsDetach() || !d.freeSpaceAtBegin()) {
-        DataPointer detached(DataPointer::allocateGrow(d, 1, QArrayData::GrowsAtBeginning));
+        // protect against args being an element of the container
-
+        T tmp(std::forward<Args>(args)...);
-        detached->emplaceBack(std::forward<Args>(args)...);
+        d.reallocateAndGrow(QArrayData::GrowsAtBeginning, 1);
-        if (!d.needsDetach())
+        d->emplaceFront(std::move(tmp));
-            detached->moveAppend(d.begin(), d.end());
-        else
-            detached->copyAppend(constBegin(), constEnd());
-        d.swap(detached);
     } else {
         d->emplaceFront(std::forward<Args>(args)...);
     }
@@ -778,10 +751,8 @@ QList<T>::emplace(qsizetype i, Args&&... args)
 
         DataPointer detached(DataPointer::allocateGrow(d, 1, pos));
         const_iterator where = constBegin() + i;
-        // Create an element here to handle cases when a user moves the element
+
-        // from a container to the same container. This is a critical step for
+        // protect against args being an element of the container
-        // COW types (e.g. Qt types) since copyAppend() done before emplace()
-        // would shallow-copy the passed element and ruin the move
         T tmp(std::forward<Args>(args)...);
 
         detached->copyAppend(constBegin(), where);
@@ -799,22 +770,10 @@ template<typename... Args>
 inline typename QList<T>::reference QList<T>::emplaceBack(Args &&... args)
 {
     if (d->needsDetach() || !d.freeSpaceAtEnd()) {
-        // condition below should follow the condition in QArrayDataPointer::reallocateGrow()
+        // protect against args being an element of the container
-        if constexpr (!QTypeInfo<T>::isRelocatable || alignof(T) > alignof(std::max_align_t)) {
+        T tmp(std::forward<Args>(args)...);
-            // avoid taking a temporary copy of Args
+        d.reallocateAndGrow(QArrayData::GrowsAtEnd, 1);
-            DataPointer detached(DataPointer::allocateGrow(d, 1, QArrayData::GrowsAtEnd));
+        d->emplaceBack(std::move(tmp));
-            detached->copyAppend(constBegin(), constEnd());
-            detached->emplace(detached.end(), std::forward<Args>(args)...);
-            d.swap(detached);
-        } else {
-            // Create an element here to handle cases when a user moves the element
-            // from a container to the same container. This is required as we call
-            // reallocate, which could delete the data args points to.
-            // This should be optimised to only take the copy when really required.
-            T tmp(std::forward<Args>(args)...);
-            DataPointer::reallocateGrow(d, 1);
-            d->emplace(d.end(), std::move(tmp));
-        }
     } else {
         d->emplaceBack(std::forward<Args>(args)...);
     }

tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp

@@ -2079,7 +2079,7 @@ void tst_QArrayData::dataPointerAllocate()
         const auto freeAtBegin = newDataPointer.freeSpaceAtBegin();
         const auto freeAtEnd = newDataPointer.freeSpaceAtEnd();
 
-        QVERIFY(newAlloc > oldDataPointer.constAllocatedCapacity());
+        QVERIFY(newAlloc >= oldDataPointer.constAllocatedCapacity());
         QCOMPARE(freeAtBegin + freeAtEnd, newAlloc);
         if (GrowthPosition == QArrayData::GrowsAtBeginning) {
             QVERIFY(freeAtBegin > 0);

tests/auto/corelib/tools/qlist/tst_qlist.cpp

@@ -874,8 +874,8 @@ void tst_QList::appendList() const
         v6 << (QList<ConstructionCounted>() << 1 << 2);
         v6 << (QList<ConstructionCounted>() << 3 << 4);
         QCOMPARE(v6, expectedFour);
-        QCOMPARE(v6.at(0).copies, 2);
+        QCOMPARE(v6.at(0).copies, 1);
-        QCOMPARE(v6.at(0).moves, 1);
+        QCOMPARE(v6.at(0).moves, 3);
 
         // +=
         QList<ConstructionCounted> v7;