From 0b3f4f8f5aada44f7e5b95cd72fd45f9e2198a09 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Thu, 16 Feb 2017 12:09:17 +0100 Subject: [PATCH] QReadWriteLock: replace a QHash with a QVarLengthArray<., 16> The QHash was used to track the recursion level of concurrent readers. But it makes no sense to optimize this data structure for O(1) lookup speed, since once you go beyond a few threads, a mutex-based solution falls apart, anyway. So use an unordered QVarLengthArray with preallocated capacity 16 instead. Lookup and erasure are now O(N), but tracking the first 16 threads that concurrently lock this shared mutex for reading no longer allocates memory (except for the Private class that contains the data structure). Results on my machine (recursive only): thread count: 16 ********* Start testing of tst_QReadWriteLock ********* Config: Using QtTest library 6.3.0, Qt 6.3.0 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 11.2.1 20211101), ubuntu 20.04 [...] PASS : tst_QReadWriteLock::uncontended(QReadWriteLock, read, recursive: 1) RESULT : tst_QReadWriteLock::uncontended():"QReadWriteLock, read, recursive: 1": - 51 msecs per iteration (total: 51, iterations: 1) + 38 msecs per iteration (total: 77, iterations: 2) PASS : tst_QReadWriteLock::uncontended(QReadWriteLock, write, recursive: 1) RESULT : tst_QReadWriteLock::uncontended():"QReadWriteLock, write, recursive: 1": - 31 msecs per iteration (total: 62, iterations: 2) + 29 msecs per iteration (total: 58, iterations: 2) PASS : tst_QReadWriteLock::uncontended(QReadWriteLock, read, recursive: 2) RESULT : tst_QReadWriteLock::uncontended():"QReadWriteLock, read, recursive: 2": - 89 msecs per iteration (total: 89, iterations: 1) + 75 msecs per iteration (total: 75, iterations: 1) PASS : tst_QReadWriteLock::uncontended(QReadWriteLock, write, recursive: 2) RESULT : tst_QReadWriteLock::uncontended():"QReadWriteLock, write, recursive: 2": - 62 msecs per iteration (total: 62, iterations: 1) + 56 msecs per iteration (total: 56, iterations: 1) PASS : tst_QReadWriteLock::uncontended(QReadWriteLock, read, recursive: 32) RESULT : tst_QReadWriteLock::uncontended():"QReadWriteLock, read, recursive: 32": - 1,357 msecs per iteration (total: 1,357, iterations: 1) + 1,154 msecs per iteration (total: 1,154, iterations: 1) PASS : tst_QReadWriteLock::uncontended(QReadWriteLock, write, recursive: 32) RESULT : tst_QReadWriteLock::uncontended():"QReadWriteLock, write, recursive: 32": - 1,067 msecs per iteration (total: 1,067, iterations: 1) + 984 msecs per iteration (total: 984, iterations: 1) [...] PASS : tst_QReadWriteLock::readOnly(QReadWriteLock, recursive: 1) RESULT : tst_QReadWriteLock::readOnly():"QReadWriteLock, recursive: 1": - 11,561 msecs per iteration (total: 11,561, iterations: 1) + 6,704 msecs per iteration (total: 6,704, iterations: 1) PASS : tst_QReadWriteLock::readOnly(QReadWriteLock, recursive: 2) RESULT : tst_QReadWriteLock::readOnly():"QReadWriteLock, recursive: 2": - 16,173 msecs per iteration (total: 16,173, iterations: 1) + 13,053 msecs per iteration (total: 13,053, iterations: 1) PASS : tst_QReadWriteLock::readOnly(QReadWriteLock, recursive: 32) RESULT : tst_QReadWriteLock::readOnly():"QReadWriteLock, recursive: 32": - 178,597 msecs per iteration (total: 178,597, iterations: 1) + 146,008 msecs per iteration (total: 146,008, iterations: 1) [...] PASS : tst_QReadWriteLock::writeOnly(QReadWriteLock, recursive: 1) RESULT : tst_QReadWriteLock::writeOnly():"QReadWriteLock, recursive: 1": - 65,165 msecs per iteration (total: 65,165, iterations: 1) + 64,503 msecs per iteration (total: 64,503, iterations: 1) PASS : tst_QReadWriteLock::writeOnly(QReadWriteLock, recursive: 2) RESULT : tst_QReadWriteLock::writeOnly():"QReadWriteLock, recursive: 2": - 70,665 msecs per iteration (total: 70,665, iterations: 1) + 69,812 msecs per iteration (total: 69,812, iterations: 1) PASS : tst_QReadWriteLock::writeOnly(QReadWriteLock, recursive: 32) RESULT : tst_QReadWriteLock::writeOnly():"QReadWriteLock, recursive: 32": - 50,811 msecs per iteration (total: 50,811, iterations: 1) + 57,659 msecs per iteration (total: 57,659, iterations: 1) Recursive mode is really, really expensive, even with this patch applied. Change-Id: I36a164cf09462b69dce7e553f96afcebb49e3dbf Reviewed-by: Thiago Macieira --- src/corelib/thread/qreadwritelock.cpp | 20 +++++++++++++++----- src/corelib/thread/qreadwritelock_p.h | 11 +++++++++-- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/src/corelib/thread/qreadwritelock.cpp b/src/corelib/thread/qreadwritelock.cpp index 894e8524943..f22f3dab409 100644 --- a/src/corelib/thread/qreadwritelock.cpp +++ b/src/corelib/thread/qreadwritelock.cpp @@ -50,6 +50,8 @@ #include "private/qfreelist_p.h" #include "private/qlocking_p.h" +#include + QT_BEGIN_NAMESPACE /* @@ -534,6 +536,11 @@ void QReadWriteLockPrivate::unlock() readerCond.wakeAll(); } +static auto handleEquals(Qt::HANDLE handle) +{ + return [handle](QReadWriteLockPrivate::Reader reader) { return reader.handle == handle; }; +} + bool QReadWriteLockPrivate::recursiveLockForRead(int timeout) { Q_ASSERT(recursive); @@ -541,16 +548,18 @@ bool QReadWriteLockPrivate::recursiveLockForRead(int timeout) Qt::HANDLE self = QThread::currentThreadId(); - auto it = currentReaders.find(self); + auto it = std::find_if(currentReaders.begin(), currentReaders.end(), + handleEquals(self)); if (it != currentReaders.end()) { - ++it.value(); + ++it->recursionLevel; return true; } if (!lockForRead(timeout)) return false; - currentReaders.insert(self, 1); + Reader r = {self, 1}; + currentReaders.append(std::move(r)); return true; } @@ -583,12 +592,13 @@ void QReadWriteLockPrivate::recursiveUnlock() return; currentWriter = nullptr; } else { - auto it = currentReaders.find(self); + auto it = std::find_if(currentReaders.begin(), currentReaders.end(), + handleEquals(self)); if (it == currentReaders.end()) { qWarning("QReadWriteLock::unlock: unlocking from a thread that did not lock"); return; } else { - if (--it.value() <= 0) { + if (--it->recursionLevel <= 0) { currentReaders.erase(it); readerCount--; } diff --git a/src/corelib/thread/qreadwritelock_p.h b/src/corelib/thread/qreadwritelock_p.h index a4d002b7f2a..1adbce6658c 100644 --- a/src/corelib/thread/qreadwritelock_p.h +++ b/src/corelib/thread/qreadwritelock_p.h @@ -53,7 +53,7 @@ // #include -#include +#include #include QT_REQUIRE_CONFIG(thread); @@ -87,13 +87,20 @@ public: // Recusive mutex handling Qt::HANDLE currentWriter = {}; - QHash currentReaders; + + struct Reader { + Qt::HANDLE handle; + int recursionLevel; + }; + + QVarLengthArray currentReaders; // called with the mutex unlocked bool recursiveLockForWrite(int timeout); bool recursiveLockForRead(int timeout); void recursiveUnlock(); }; +Q_DECLARE_TYPEINFO(QReadWriteLockPrivate::Reader, Q_PRIMITIVE_TYPE); QT_END_NAMESPACE