From 1d167b515ef81ba71f3f47863e66d36ed6d06c1c Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Mon, 6 Feb 2023 15:13:51 -0800 Subject: [PATCH] QHash: fix GrowthPolicy::bucketsForCapacity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It was confusing entry capacity with the bucket capacity. The value maxNumBuckets() returned was the maximum number of entries. This issue was harmless: we would just fail to cap the maximum to an allocatable size. But the array new[] in the Data constructors would have capped the maximum anyway (by way of throwing std::bad_alloc). So instead of trying to calculate what the maximum bucket count is so we can cap at that, simplify the calculation of the next power of 2 while preventing it from overflowing in our calculations. We continue to rely on new[] throwing when we return count that is larger than the maximum allocatable. This commit changes the load factor for QHashes containing exactly a number of elements that is exactly a power of two. Previously, it would be loaded at 50%, now it's at 25%. For this reason, tst_QSet::squeeze needed to be fixed to depend less on the implementation details. Pick-to: 6.5 Change-Id: I9671dee8ceb64aa9b9cafffd17415f3856c358a0 Reviewed-by: Mårten Nordheim --- src/corelib/tools/qhash.cpp | 32 ++++++++++++++++ src/corelib/tools/qhash.h | 40 ++++++++++---------- tests/auto/corelib/tools/qhash/tst_qhash.cpp | 36 ++++++++++++++++++ tests/auto/corelib/tools/qset/tst_qset.cpp | 28 ++++++++++---- 4 files changed, 109 insertions(+), 27 deletions(-) diff --git a/src/corelib/tools/qhash.cpp b/src/corelib/tools/qhash.cpp index 59d84f5094a..db52852dc84 100644 --- a/src/corelib/tools/qhash.cpp +++ b/src/corelib/tools/qhash.cpp @@ -3918,4 +3918,36 @@ size_t qHash(long double key, size_t seed) noexcept Returns the number of elements removed, if any. */ +#ifdef QT_HAS_CONSTEXPR_BITOPS +namespace QHashPrivate { +static_assert(qPopulationCount(SpanConstants::NEntries) == 1, + "NEntries must be a power of 2 for bucketForHash() to work."); + +// ensure the size of a Span does not depend on the template parameters +using Node1 = Node; +static_assert(sizeof(Span) == sizeof(Span>)); +static_assert(sizeof(Span) == sizeof(Span>)); +static_assert(sizeof(Span) == sizeof(Span>)); +static_assert(sizeof(Span) > SpanConstants::NEntries); +static_assert(qNextPowerOfTwo(sizeof(Span)) == SpanConstants::NEntries * 2); + +// ensure allocations are always a power of two, at a minimum NEntries, +// obeying the fomula +// qNextPowerOfTwo(2 * N); +// without overflowing +static constexpr size_t NEntries = SpanConstants::NEntries; +static_assert(GrowthPolicy::bucketsForCapacity(1) == NEntries); +static_assert(GrowthPolicy::bucketsForCapacity(NEntries / 2 + 0) == NEntries); +static_assert(GrowthPolicy::bucketsForCapacity(NEntries / 2 + 1) == 2 * NEntries); +static_assert(GrowthPolicy::bucketsForCapacity(NEntries * 1 - 1) == 2 * NEntries); +static_assert(GrowthPolicy::bucketsForCapacity(NEntries * 1 + 0) == 4 * NEntries); +static_assert(GrowthPolicy::bucketsForCapacity(NEntries * 1 + 1) == 4 * NEntries); +static_assert(GrowthPolicy::bucketsForCapacity(NEntries * 2 - 1) == 4 * NEntries); +static_assert(GrowthPolicy::bucketsForCapacity(NEntries * 2 + 0) == 8 * NEntries); +static_assert(GrowthPolicy::bucketsForCapacity(SIZE_MAX / 4) == SIZE_MAX / 2 + 1); +static_assert(GrowthPolicy::bucketsForCapacity(SIZE_MAX / 2) == SIZE_MAX); +static_assert(GrowthPolicy::bucketsForCapacity(SIZE_MAX) == SIZE_MAX); +} +#endif + QT_END_NAMESPACE diff --git a/src/corelib/tools/qhash.h b/src/corelib/tools/qhash.h index b6a12a721e9..09766859a9b 100644 --- a/src/corelib/tools/qhash.h +++ b/src/corelib/tools/qhash.h @@ -5,11 +5,11 @@ #ifndef QHASH_H #define QHASH_H +#include #include #include #include #include -#include #include #include @@ -414,28 +414,25 @@ struct Span { // QHash uses a power of two growth policy. namespace GrowthPolicy { -inline constexpr size_t maxNumBuckets() noexcept -{ - // ensure the size of a Span does not depend on the template parameters - using Node1 = Node; - using Node2 = Node; - using Node3 = Node; - static_assert(sizeof(Span) == sizeof(Span)); - static_assert(sizeof(Span) == sizeof(Span)); - - // Maximum is 2^31-1 or 2^63-1 bytes (limited by qsizetype and ptrdiff_t) - size_t max = (std::numeric_limits::max)(); - return max / sizeof(Span) * SpanConstants::NEntries; -} inline constexpr size_t bucketsForCapacity(size_t requestedCapacity) noexcept { + constexpr int SizeDigits = std::numeric_limits::digits; + // We want to use at minimum a full span (128 entries), so we hardcode it for any requested // capacity <= 64. Any capacity above that gets rounded to a later power of two. if (requestedCapacity <= 64) return SpanConstants::NEntries; - if (requestedCapacity >= maxNumBuckets()) - return maxNumBuckets(); - return qNextPowerOfTwo(2 * requestedCapacity - 1); + + // Same as + // qNextPowerOfTwo(2 * requestedCapacity); + // + // but ensuring neither our multiplication nor the function overflow. + // Additionally, the maximum memory allocation is 2^31-1 or 2^63-1 bytes + // (limited by qsizetype and ptrdiff_t). + int count = qCountLeadingZeroBits(requestedCapacity); + if (count < 2) + return (std::numeric_limits::max)(); // will cause std::bad_alloc + return size_t(1) << (SizeDigits - count + 1); } inline constexpr size_t bucketForHash(size_t nBuckets, size_t hash) noexcept { @@ -460,6 +457,11 @@ struct Data size_t seed = 0; Span *spans = nullptr; + static constexpr size_t maxNumBuckets() noexcept + { + return (std::numeric_limits::max)() / sizeof(Span); + } + struct Bucket { Span *span; size_t index; @@ -1318,7 +1320,7 @@ public: float load_factor() const noexcept { return d ? d->loadFactor() : 0; } static float max_load_factor() noexcept { return 0.5; } size_t bucket_count() const noexcept { return d ? d->numBuckets : 0; } - static size_t max_bucket_count() noexcept { return QHashPrivate::GrowthPolicy::maxNumBuckets(); } + static size_t max_bucket_count() noexcept { return Data::maxNumBuckets(); } inline bool empty() const noexcept { return isEmpty(); } @@ -1948,7 +1950,7 @@ public: float load_factor() const noexcept { return d ? d->loadFactor() : 0; } static float max_load_factor() noexcept { return 0.5; } size_t bucket_count() const noexcept { return d ? d->numBuckets : 0; } - static size_t max_bucket_count() noexcept { return QHashPrivate::GrowthPolicy::maxNumBuckets(); } + static size_t max_bucket_count() noexcept { return Data::maxNumBuckets(); } inline bool empty() const noexcept { return isEmpty(); } diff --git a/tests/auto/corelib/tools/qhash/tst_qhash.cpp b/tests/auto/corelib/tools/qhash/tst_qhash.cpp index ed21dc71b57..50b1248d91f 100644 --- a/tests/auto/corelib/tools/qhash/tst_qhash.cpp +++ b/tests/auto/corelib/tools/qhash/tst_qhash.cpp @@ -76,6 +76,8 @@ private slots: void reserveShared(); void reserveLessThanCurrentAmount(); + void reserveKeepCapacity_data(); + void reserveKeepCapacity(); void QTBUG98265(); @@ -2693,6 +2695,40 @@ void tst_QHash::reserveLessThanCurrentAmount() } } +void tst_QHash::reserveKeepCapacity_data() +{ + QTest::addColumn("requested"); + auto addRow = [](qsizetype requested) { + QTest::addRow("%td", ptrdiff_t(requested)) << requested; + }; + + QHash testHash = {{1, 1}}; + qsizetype minCapacity = testHash.capacity(); + addRow(minCapacity - 1); + addRow(minCapacity + 0); + addRow(minCapacity + 1); + addRow(2 * minCapacity - 1); + addRow(2 * minCapacity + 0); + addRow(2 * minCapacity + 1); +} + +void tst_QHash::reserveKeepCapacity() +{ + QFETCH(qsizetype, requested); + + QHash hash; + hash.reserve(requested); + qsizetype initialCapacity = hash.capacity(); + QCOMPARE_GE(initialCapacity, requested); + + // insert this many elements into the hash + for (qsizetype i = 0; i < requested; ++i) + hash.insert(i, i); + + // it mustn't have increased capacity after inserting the elements + QCOMPARE(hash.capacity(), initialCapacity); +} + void tst_QHash::QTBUG98265() { QMultiHash a; diff --git a/tests/auto/corelib/tools/qset/tst_qset.cpp b/tests/auto/corelib/tools/qset/tst_qset.cpp index 734d1d491be..391e00485c2 100644 --- a/tests/auto/corelib/tools/qset/tst_qset.cpp +++ b/tests/auto/corelib/tools/qset/tst_qset.cpp @@ -232,27 +232,39 @@ void tst_QSet::squeeze() set.squeeze(); QVERIFY(set.capacity() < 100); - for (int i = 0; i < 512; ++i) + for (int i = 0; i < 500; ++i) set.insert(i); - QVERIFY(set.capacity() == 512); + QCOMPARE(set.size(), 500); + + // squeezed capacity for 500 elements + qsizetype capacity = set.capacity(); // current implementation: 512 + QCOMPARE_GE(capacity, set.size()); set.reserve(50000); - QVERIFY(set.capacity() >= 50000); + QVERIFY(set.capacity() >= 50000); // current implementation: 65536 set.squeeze(); - QVERIFY(set.capacity() == 512); + QCOMPARE(set.capacity(), capacity); + // removing elements does not shed capacity set.remove(499); - QVERIFY(set.capacity() == 512); + QCOMPARE(set.capacity(), capacity); set.insert(499); - QVERIFY(set.capacity() == 512); + QCOMPARE(set.capacity(), capacity); - set.insert(1000); - QVERIFY(set.capacity() == 1024); + // grow it beyond the current capacity + for (int i = set.size(); i <= capacity; ++i) + set.insert(i); + QCOMPARE(set.size(), capacity + 1); + QCOMPARE_GT(set.capacity(), capacity + 1);// current implementation: 2 * capacity (1024) for (int i = 0; i < 500; ++i) set.remove(i); + + // removing elements does not shed capacity + QCOMPARE_GT(set.capacity(), capacity + 1); + set.squeeze(); QVERIFY(set.capacity() < 100); }