Skip to content

Commit

Permalink
QHash: fix small performance regression when detaching with resize
Browse files Browse the repository at this point in the history
There are two QHashPrivate::Data constructors (and two overloads of
::detached()). Initially the Data ctor that takes a new size always
assumed a resize was happening, and any place where there _may_ be a
resize we would usually detach then rehash.

In an effort to avoid the detach+rehash and instead call detach(d, size)
without the performance overhead of rehashing the call to
reallocationHelper() in this overload of the ctor was changed to verify
that a rehash was actually needed. This had the unfortunate side-effect
of making the compiler no longer inline the reallocationHelper()
function, and it no longer expanded the if-expression with the constant
so it was doing a tight copy-loop with a potential branch in the middle.

In this patch we revert that and make the bool a template argument to
highlight that it should be a constant for better performance (but also
leaving a comment.) Also mark it Q_ALWAYS_INLINE, it has two uses and
they both take advantage of the inlining + expanding the expression.

In theory this might have had an impact on QHash::reserve() calls,
though this code is only relevant when reserve() would cause growth so
the performance regression would hopefully be small compared to all the
other work that would also be needed.

Reverts 45c137d

Change-Id: I0d2076a9ded8ca816c54d6ce42d472a23bcbc9fd
Reviewed-by: Lars Knoll <[email protected]>
Reviewed-by: Thiago Macieira <[email protected]>
(cherry picked from commit 6c8b6ac)
Reviewed-by: Qt Cherry-pick Bot <[email protected]>
(cherry picked from commit d53feb3)
  • Loading branch information
Morten242 authored and Qt Cherry-pick Bot committed Dec 15, 2024
1 parent 909c3fa commit 6bf0b40
Showing 1 changed file with 8 additions and 4 deletions.
12 changes: 8 additions & 4 deletions src/corelib/tools/qhash.h
Original file line number Diff line number Diff line change
Expand Up @@ -558,15 +558,19 @@ struct Data
seed = QHashSeed::globalSeed();
}

void reallocationHelper(const Data &other, size_t nSpans, bool resized)
// The Resized parameter is a template param to make sure the compiler will get rid of the
// branch, for performance.
template <bool Resized>
Q_ALWAYS_INLINE
void reallocationHelper(const Data &other, size_t nSpans)
{
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);
Expand All @@ -578,14 +582,14 @@ struct Data
{
auto r = allocateSpans(numBuckets);
spans = r.spans;
reallocationHelper(other, r.nSpans, false);
reallocationHelper<false>(other, r.nSpans);
}
Data(const Data &other, size_t reserved) : size(other.size), seed(other.seed)
{
numBuckets = GrowthPolicy::bucketsForCapacity(qMax(size, reserved));
spans = allocateSpans(numBuckets).spans;
size_t otherNSpans = other.numBuckets >> SpanConstants::SpanShift;
reallocationHelper(other, otherNSpans, numBuckets != other.numBuckets);
reallocationHelper<true>(other, otherNSpans);
}

static Data *detached(Data *d)
Expand Down

0 comments on commit 6bf0b40

Please sign in to comment.