From 2578cc7d9ea3ad3433c7952b300569774ea40b6b Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Tue, 22 Aug 2023 12:34:43 -0700 Subject: [PATCH] QArrayData: make calculateBlockSize() account for the extra null element Instead of adding it after the block size was calculated. This makes no difference for non-growing (exact) blocks. For growing blocks, this means we take that extra element into account before rounding to the next power of two, instead of after. That results in a change of the thresholds of when a block grows and also what capacity it will contain. For example, for a QString growing to 22-25 elements: Request | Previously | Now | elements | bytes | malloc()ed | capacity() | malloc()ed | capacity() | 22 | 44 | 66 | 24 | 64 | 23 | 23 | 46 | 66 | 24 | 64 | 23 | 24 | 48 | 66 | 24 | 128 | 55 | 25 | 50 | 130 | 56 | 128 | 55 | To avoid wasting elementSize - 2 bytes in this footer, we only include this footer if elementSize <= 2. Thus, for a QList growing to 11-13 elements: Request | Previously | Now | elements | bytes | malloc()ed | capacity() | malloc()ed | capacity() | 11 | 44 | 66 | 12 | 64 | 12 | 12 | 48 | 66 | 12 | 128 | 28 | 13 | 52 | 130 | 28 | 128 | 28 | In both cases, we now only allocate powers of two while growing, which may be beneficial to some allocators. Change-Id: Ifa1111900d6945ea8e05fffd177dcb96e251d0a1 Reviewed-by: Fabian Kosmale Reviewed-by: Qt CI Bot (cherry picked from commit 961620824ca0ae764b3c6ce98b16ecce951168c8) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/tools/qarraydata.cpp | 36 ++++++++----------- .../tools/qarraydata/tst_qarraydata.cpp | 2 +- tests/auto/corelib/tools/qlist/tst_qlist.cpp | 3 +- 3 files changed, 18 insertions(+), 23 deletions(-) diff --git a/src/corelib/tools/qarraydata.cpp b/src/corelib/tools/qarraydata.cpp index 4306e004427..56f14616449 100644 --- a/src/corelib/tools/qarraydata.cpp +++ b/src/corelib/tools/qarraydata.cpp @@ -107,26 +107,24 @@ qCalculateGrowingBlockSize(qsizetype elementCount, qsizetype elementSize, qsizet return result; } -/*! - \internal - - Returns \a allocSize plus extra reserved bytes necessary to store '\0'. - */ -static inline qsizetype reserveExtraBytes(qsizetype allocSize) -{ - // We deal with QByteArray and QString only - constexpr qsizetype extra = qMax(sizeof(QByteArray::value_type), sizeof(QString::value_type)); - if (Q_UNLIKELY(allocSize < 0)) - return -1; - if (Q_UNLIKELY(qAddOverflow(allocSize, extra, &allocSize))) - return -1; - return allocSize; -} +/* + Calculate the byte size for a block of \a capacity objects of size \a + objectSize, with a header of size \a headerSize. If the \a option is + QArrayData::Grow, the capacity itself adjusted up, preallocating room for + more elements to be added later; otherwise, it is an exact calculation. + Returns a structure containing the size in bytes and elements available. +*/ static inline CalculateGrowingBlockSizeResult calculateBlockSize(qsizetype capacity, qsizetype objectSize, qsizetype headerSize, QArrayData::AllocationOption option) { - // Calculate the byte size + // Adjust the header size up to account for the trailing null for QString + // and QByteArray. This is not checked for overflow because headers sizes + // should not be anywhere near the overflow limit. + constexpr qsizetype FooterSize = qMax(sizeof(QString::value_type), sizeof(QByteArray::value_type)); + if (objectSize <= FooterSize) + headerSize += FooterSize; + // allocSize = objectSize * capacity + headerSize, but checked for overflow // plus padded to grow in size if (option == QArrayData::Grow) { @@ -182,7 +180,7 @@ void *QArrayData::allocate(QArrayData **dptr, qsizetype objectSize, qsizetype al auto blockSize = calculateBlockSize(capacity, objectSize, headerSize, option); capacity = blockSize.elementCount; - qsizetype allocSize = reserveExtraBytes(blockSize.size); + qsizetype allocSize = blockSize.size; if (Q_UNLIKELY(allocSize < 0)) { // handle overflow. cannot allocate reliably *dptr = nullptr; return nullptr; @@ -219,10 +217,6 @@ QArrayData::reallocateUnaligned(QArrayData *data, void *dataPointer, Q_ASSERT(offset > 0); Q_ASSERT(offset <= allocSize); // equals when all free space is at the beginning - allocSize = reserveExtraBytes(allocSize); - if (Q_UNLIKELY(allocSize < 0)) // handle overflow. cannot reallocate reliably - return qMakePair(data, dataPointer); - QArrayData *header = static_cast(::realloc(data, size_t(allocSize))); if (header) { header->alloc = capacity; diff --git a/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp b/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp index 987cee03490..c20f2af6e86 100644 --- a/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp +++ b/tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp @@ -483,7 +483,7 @@ void tst_QArrayData::allocate() keeper.headers.append(data); if (grow) - QVERIFY(data->allocatedCapacity() > capacity); + QCOMPARE_GE(data->allocatedCapacity(), capacity); else QCOMPARE(data->allocatedCapacity(), capacity); diff --git a/tests/auto/corelib/tools/qlist/tst_qlist.cpp b/tests/auto/corelib/tools/qlist/tst_qlist.cpp index f52d4650d28..afced64b42c 100644 --- a/tests/auto/corelib/tools/qlist/tst_qlist.cpp +++ b/tests/auto/corelib/tools/qlist/tst_qlist.cpp @@ -1090,6 +1090,7 @@ void tst_QList::appendList() const // Using operators // << QList v6; + v6.reserve(4); v6 << (QList() << 1 << 2); v6 << (QList() << 3 << 4); QCOMPARE(v6, expectedFour); @@ -3762,7 +3763,7 @@ void tst_QList::stability_append() const std::generate(v.begin(), v.end(), [&k]() { return SimpleValue::at(k++); }); QList src(1, SimpleValue::at(0)); v.append(src.begin(), src.end()); - QVERIFY(v.size() < v.capacity()); + QCOMPARE_LE(v.size(), v.capacity()); for (int i = 0; i < v.capacity() - v.size(); ++i) { auto [copy, reference] = qlistCopyAndReferenceFromRange(v.begin(), v.end());