From 319c4786036b5f45fc95c683cef5cf5ba2ce2a6d Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Fri, 23 Aug 2019 15:05:32 +0200 Subject: [PATCH] QReadWriteLock: replace (QWaitCondition, QMutex) with std::(condition_variable, mutex) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It turns out that QWaitCondition is a std::condition_variable_any. The _any variant works with any mutex type, but requires a native mutex for the native condition variable. So, QWaitCondition and std::condition_variable_any both require two different mutexes: the one the user passes in, and an internal one. std::condition_variable, however, only works with std::mutex, and since both are backed by the native API, condition_variable can use the mutex passed in by the user instead of having to use an internal one. So, port from 2 × QWaitCondition + QMutex (2 × native cond + 2 × native mutex + Qt mutex) to std::condition_variable + std::mutex (2 × native cond + native mutex), shaving the overhead of two additional mutexes (one Qt, one native) as well as the memory allocation performed by QWaitCondition (for its Private). Speeds up the writeOnly case by ~1/8th: PASS : tst_QReadWriteLock::writeOnly(QReadWriteLock) RESULT : tst_QReadWriteLock::writeOnly():"QReadWriteLock": - 39,703 msecs per iteration (total: 39,703, iterations: 1) + 34,950 msecs per iteration (total: 34,950, iterations: 1) Change-Id: I196cb13a27242fc1cb99723dfab5b2e5f8522143 Reviewed-by: Edward Welbourne Reviewed-by: Mårten Nordheim Reviewed-by: Olivier Goffart (Woboq GmbH) Reviewed-by: Volker Hilsheimer --- src/corelib/thread/qreadwritelock.cpp | 37 +++++++++++++++------------ src/corelib/thread/qreadwritelock_p.h | 14 +++++----- 2 files changed, 29 insertions(+), 22 deletions(-) diff --git a/src/corelib/thread/qreadwritelock.cpp b/src/corelib/thread/qreadwritelock.cpp index 14654986a0c..5aba05c1b97 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 /* @@ -65,6 +67,9 @@ QT_BEGIN_NAMESPACE */ namespace { + +using ms = std::chrono::milliseconds; + enum { StateMask = 0x3, StateLockedForRead = 0x1, @@ -274,7 +279,7 @@ bool QReadWriteLock::tryLockForRead(int timeout) d = d_ptr.loadAcquire(); continue; } - return d->lockForRead(timeout); + return d->lockForRead(lock, timeout); } } @@ -378,7 +383,7 @@ bool QReadWriteLock::tryLockForWrite(int timeout) d = d_ptr.loadAcquire(); continue; } - return d->lockForWrite(timeout); + return d->lockForWrite(lock, timeout); } } @@ -461,9 +466,9 @@ QReadWriteLock::StateForWaitCondition QReadWriteLock::stateForWaitCondition() co } -bool QReadWriteLockPrivate::lockForRead(int timeout) +bool QReadWriteLockPrivate::lockForRead(std::unique_lock &lock, int timeout) { - Q_ASSERT(!mutex.tryLock()); // mutex must be locked when entering this function + Q_ASSERT(!mutex.try_lock()); // mutex must be locked when entering this function QElapsedTimer t; if (timeout > 0) @@ -477,10 +482,10 @@ bool QReadWriteLockPrivate::lockForRead(int timeout) if (elapsed > timeout) return false; waitingReaders++; - readerCond.wait(&mutex, timeout - elapsed); + readerCond.wait_for(lock, ms{timeout - elapsed}); } else { waitingReaders++; - readerCond.wait(&mutex); + readerCond.wait(lock); } waitingReaders--; } @@ -489,9 +494,9 @@ bool QReadWriteLockPrivate::lockForRead(int timeout) return true; } -bool QReadWriteLockPrivate::lockForWrite(int timeout) +bool QReadWriteLockPrivate::lockForWrite(std::unique_lock &lock, int timeout) { - Q_ASSERT(!mutex.tryLock()); // mutex must be locked when entering this function + Q_ASSERT(!mutex.try_lock()); // mutex must be locked when entering this function QElapsedTimer t; if (timeout > 0) @@ -506,15 +511,15 @@ bool QReadWriteLockPrivate::lockForWrite(int timeout) if (waitingReaders && !waitingWriters && !writerCount) { // We timed out and now there is no more writers or waiting writers, but some // readers were queueud (probably because of us). Wake the waiting readers. - readerCond.wakeAll(); + readerCond.notify_all(); } return false; } waitingWriters++; - writerCond.wait(&mutex, timeout - elapsed); + writerCond.wait_for(lock, ms{timeout - elapsed}); } else { waitingWriters++; - writerCond.wait(&mutex); + writerCond.wait(lock); } waitingWriters--; } @@ -527,11 +532,11 @@ bool QReadWriteLockPrivate::lockForWrite(int timeout) void QReadWriteLockPrivate::unlock() { - Q_ASSERT(!mutex.tryLock()); // mutex must be locked when entering this function + Q_ASSERT(!mutex.try_lock()); // mutex must be locked when entering this function if (waitingWriters) - writerCond.wakeOne(); + writerCond.notify_one(); else if (waitingReaders) - readerCond.wakeAll(); + readerCond.notify_all(); } bool QReadWriteLockPrivate::recursiveLockForRead(int timeout) @@ -547,7 +552,7 @@ bool QReadWriteLockPrivate::recursiveLockForRead(int timeout) return true; } - if (!lockForRead(timeout)) + if (!lockForRead(lock, timeout)) return false; currentReaders.insert(self, 1); @@ -565,7 +570,7 @@ bool QReadWriteLockPrivate::recursiveLockForWrite(int timeout) return true; } - if (!lockForWrite(timeout)) + if (!lockForWrite(lock, timeout)) return false; currentWriter = self; diff --git a/src/corelib/thread/qreadwritelock_p.h b/src/corelib/thread/qreadwritelock_p.h index a4d002b7f2a..b2e782f9eeb 100644 --- a/src/corelib/thread/qreadwritelock_p.h +++ b/src/corelib/thread/qreadwritelock_p.h @@ -54,7 +54,9 @@ #include #include -#include + +#include +#include QT_REQUIRE_CONFIG(thread); @@ -66,9 +68,9 @@ public: explicit QReadWriteLockPrivate(bool isRecursive = false) : recursive(isRecursive) {} - QMutex mutex; - QWaitCondition writerCond; - QWaitCondition readerCond; + std::mutex mutex; + std::condition_variable writerCond; + std::condition_variable readerCond; int readerCount = 0; int writerCount = 0; int waitingReaders = 0; @@ -76,8 +78,8 @@ public: const bool recursive; //Called with the mutex locked - bool lockForWrite(int timeout); - bool lockForRead(int timeout); + bool lockForWrite(std::unique_lock &lock, int timeout); + bool lockForRead(std::unique_lock &lock, int timeout); void unlock(); //memory management