From 7c9afa8d00cc2bbc1b102eef6e76c23e07c7833f Mon Sep 17 00:00:00 2001 From: Ivan Solovev Date: Fri, 1 Apr 2022 10:37:45 +0200 Subject: [PATCH] Q[Multi]Hash: fix squeeze() When calling QHash::reserve(), or when creating the internal QHashPrivate::Data structure, the value 0 for the size parameter is reserved for performing the squeeze operation. However commit 8a984ab772dd194e39094e728b869e65912912a7 broke it, by using the 0 value in QHashPrivate::Data constructors as a mark that no resizing needs to be done. This patch reverts the problematic commit (also applying some later fixes to the code), and adds the missing tests for Q[Multi]Hash::squeeze(). Pick-to: 6.3 6.2 Change-Id: Id644df7b2beb008e6a37b2c89b709adfbd893e25 Reviewed-by: Thiago Macieira --- src/corelib/tools/qhash.h | 44 +++++---- tests/auto/corelib/tools/qhash/tst_qhash.cpp | 95 +++++++++++++++++++- 2 files changed, 123 insertions(+), 16 deletions(-) diff --git a/src/corelib/tools/qhash.h b/src/corelib/tools/qhash.h index 6a545988d4..07e2f2d355 100644 --- a/src/corelib/tools/qhash.h +++ b/src/corelib/tools/qhash.h @@ -565,7 +565,6 @@ struct Data } }; - Data(size_t reserve = 0) { numBuckets = GrowthPolicy::bucketsForCapacity(reserve); @@ -573,25 +572,16 @@ struct Data spans = new Span[nSpans]; seed = QHashSeed::globalSeed(); } - Data(const Data &other, size_t reserved = 0) - : size(other.size), - numBuckets(other.numBuckets), - seed(other.seed) - { - if (reserved) - numBuckets = GrowthPolicy::bucketsForCapacity(qMax(size, reserved)); - bool resized = numBuckets != other.numBuckets; - size_t nSpans = numBuckets >> SpanConstants::SpanShift; - spans = new Span[nSpans]; - size_t otherNSpans = other.numBuckets >> SpanConstants::SpanShift; - for (size_t s = 0; s < otherNSpans; ++s) { + void reallocationHelper(const Data &other, size_t nSpans, bool resized) + { + for (size_t s = 0; s < nSpans; ++s) { const Span &span = other.spans[s]; for (size_t index = 0; index < SpanConstants::NEntries; ++index) { if (!span.hasNode(index)) continue; const Node &n = span.at(index); - auto it = resized ? findBucket(n.key) : Bucket{ spans + s, index }; + auto it = resized ? findBucket(n.key) : Bucket { spans + s, index }; Q_ASSERT(it.isUnused()); Node *newNode = it.insert(); new (newNode) Node(n); @@ -599,7 +589,31 @@ struct Data } } - static Data *detached(Data *d, size_t size = 0) + Data(const Data &other) : size(other.size), numBuckets(other.numBuckets), seed(other.seed) + { + size_t nSpans = numBuckets >> SpanConstants::SpanShift; + spans = new Span[nSpans]; + reallocationHelper(other, nSpans, false); + } + Data(const Data &other, size_t reserved) : size(other.size), seed(other.seed) + { + numBuckets = GrowthPolicy::bucketsForCapacity(qMax(size, reserved)); + size_t nSpans = numBuckets >> SpanConstants::SpanShift; + spans = new Span[nSpans]; + size_t otherNSpans = other.numBuckets >> SpanConstants::SpanShift; + reallocationHelper(other, otherNSpans, true); + } + + static Data *detached(Data *d) + { + if (!d) + return new Data; + Data *dd = new Data(*d); + if (!d->ref.deref()) + delete d; + return dd; + } + static Data *detached(Data *d, size_t size) { if (!d) return new Data(size); diff --git a/tests/auto/corelib/tools/qhash/tst_qhash.cpp b/tests/auto/corelib/tools/qhash/tst_qhash.cpp index 6f59fe4436..994963781c 100644 --- a/tests/auto/corelib/tools/qhash/tst_qhash.cpp +++ b/tests/auto/corelib/tools/qhash/tst_qhash.cpp @@ -105,6 +105,9 @@ private slots: void detachAndReferences(); void lookupUsingKeyIterator(); + + void squeeze(); + void squeezeShared(); }; struct IdentityTracker { @@ -2694,7 +2697,7 @@ void tst_QHash::reserveLessThanCurrentAmount() // Make sure that hash still has all elements for (int i = 0; i < 1000; ++i) - QCOMPARE(hash.values(i), QList() << i * 10 + 1 << i * 10); + QCOMPARE(hash.values(i), QList({ i * 10 + 1, i * 10 })); } } @@ -2774,5 +2777,95 @@ void tst_QHash::lookupUsingKeyIterator() QVERIFY(!hash[*it].isEmpty()); } +void tst_QHash::squeeze() +{ + { + QHash hash; + hash.reserve(1000); + for (int i = 0; i < 10; ++i) + hash.insert(i, i * 10); + QVERIFY(hash.isDetached()); + const size_t buckets = hash.bucket_count(); + const qsizetype size = hash.size(); + + hash.squeeze(); + + QVERIFY(hash.bucket_count() < buckets); + QCOMPARE(hash.size(), size); + for (int i = 0; i < size; ++i) + QCOMPARE(hash.value(i), i * 10); + } + { + QMultiHash hash; + hash.reserve(1000); + for (int i = 0; i < 10; ++i) { + hash.insert(i, i * 10); + hash.insert(i, i * 10 + 1); + } + QVERIFY(hash.isDetached()); + const size_t buckets = hash.bucket_count(); + const qsizetype size = hash.size(); + + hash.squeeze(); + + QVERIFY(hash.bucket_count() < buckets); + QCOMPARE(hash.size(), size); + for (int i = 0; i < (size / 2); ++i) + QCOMPARE(hash.values(i), QList({ i * 10 + 1, i * 10 })); + } +} + +void tst_QHash::squeezeShared() +{ + { + QHash hash; + hash.reserve(1000); + for (int i = 0; i < 10; ++i) + hash.insert(i, i * 10); + + QHash other = hash; + + // Check that when squeezing a hash with shared d_ptr, the number of + // buckets actually decreases. + QVERIFY(!other.isDetached()); + const size_t buckets = other.bucket_count(); + const qsizetype size = other.size(); + + other.squeeze(); + + QCOMPARE(hash.bucket_count(), buckets); + QVERIFY(other.bucket_count() < buckets); + + QCOMPARE(other.size(), size); + for (int i = 0; i < size; ++i) + QCOMPARE(other.value(i), i * 10); + } + { + QMultiHash hash; + hash.reserve(1000); + for (int i = 0; i < 10; ++i) { + hash.insert(i, i * 10); + hash.insert(i, i * 10 + 1); + } + + QMultiHash other = hash; + + // Check that when squeezing a hash with shared d_ptr, the number of + // buckets actually decreases. + QVERIFY(!other.isDetached()); + const size_t buckets = other.bucket_count(); + const qsizetype size = other.size(); + + other.squeeze(); + + QCOMPARE(hash.bucket_count(), buckets); + QVERIFY(other.bucket_count() < buckets); + + QCOMPARE(other.size(), size); + for (int i = 0; i < (size / 2); ++i) + QCOMPARE(other.values(i), QList({ i * 10 + 1, i * 10 })); + } +} + QTEST_APPLESS_MAIN(tst_QHash) #include "tst_qhash.moc"