summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorMarc Mutz <marc.mutz@kdab.com>2017-11-21 10:26:35 +0100
committerMarc Mutz <marc.mutz@kdab.com>2017-11-30 20:16:21 +0000
commita0faf9e23666d4aa26a93d6e9ebf420e71d5e6c2 (patch)
tree41804cf205c92b7514cabf651c383caf5b73a6cc
parent1c0fcbc887459d8963088309e83303eb1a7d2db0 (diff)
Optimize QThread::isInterruptionRequested()
To signal a thread to cancel, nothing more than a std::atomic_flag is needed, but the implementation actually used mutexes, and weird run-state introspection, so we can't just swap it out for a std::atomic_flag. Instead, we retain the principal logic, however weird it is, and just optimize the common case where isInterruptionRequested() is called from the secondary thread, repeatedly. We add a fast-path that just checks that d->interruptionRequested is not set. That requires nothing more than a relaxed atomic load, because there's no new value read that could be used as a signal to the secondary thread that some condition changed. "What signal?", you may ask. Well, one can think of users doing this: void cancel() { m_why = tr("&Canceled"); requestIterruption(); } void run() override { while (!isInterruptionRequested()) { doWork(); } emit progress(100, 100, m_why); } We need to keep this code working, at least until Qt 6. But the code can already now only rely on synchronization if isInterruptionRequested() returns true. If it returns false, then requestInterruption() has not been called, yet, and any modifications done prior to the requestInterruption() call are not visible in the secondary thead. So we still lock the mutex, and in general don't change the semantics of the functions, except that we don't lock the mutex in the case where the flag wasn't set in the first place. This makes calling isInterruptionRequested() as cheap as it can get, assuming a lock-free implementation, of course. I opted to use a std::atomic<bool> instead of QAtomicInt, as the latter does not have loadRelaxed()/storeRelaxed(), and because it future-proofs the code. Change-Id: I67faf36b8de73d2723f9cdd66c416010d0873d98 Reviewed-by: Thiago Macieira <thiago.macieira@intel.com> Reviewed-by: Olivier Goffart (Woboq GmbH) <ogoffart@woboq.com>
-rw-r--r--src/corelib/thread/qthread.cpp12
-rw-r--r--src/corelib/thread/qthread_p.h3
2 files changed, 10 insertions, 5 deletions
diff --git a/src/corelib/thread/qthread.cpp b/src/corelib/thread/qthread.cpp
index 20b1f01173..adff853669 100644
--- a/src/corelib/thread/qthread.cpp
+++ b/src/corelib/thread/qthread.cpp
@@ -849,10 +849,12 @@ void QThread::requestInterruption()
return;
}
Q_D(QThread);
+ // ### Qt 6: use std::atomic_flag, and document that
+ // requestInterruption/isInterruptionRequested do not synchronize with each other
QMutexLocker locker(&d->mutex);
if (!d->running || d->finished || d->isInFinish)
return;
- d->interruptionRequested = true;
+ d->interruptionRequested.store(true, std::memory_order_relaxed);
}
/*!
@@ -881,10 +883,12 @@ void QThread::requestInterruption()
bool QThread::isInterruptionRequested() const
{
Q_D(const QThread);
- QMutexLocker locker(&d->mutex);
- if (!d->running || d->finished || d->isInFinish)
+ // fast path: check that the flag is not set:
+ if (!d->interruptionRequested.load(std::memory_order_relaxed))
return false;
- return d->interruptionRequested;
+ // slow path: if the flag is set, take into account run status:
+ QMutexLocker locker(&d->mutex);
+ return d->running && !d->finished && !d->isInFinish;
}
/*
diff --git a/src/corelib/thread/qthread_p.h b/src/corelib/thread/qthread_p.h
index 1d38eb0ebf..baeefd87ff 100644
--- a/src/corelib/thread/qthread_p.h
+++ b/src/corelib/thread/qthread_p.h
@@ -63,6 +63,7 @@
#include "private/qobject_p.h"
#include <algorithm>
+#include <atomic>
#ifdef Q_OS_WINRT
namespace ABI {
@@ -165,7 +166,7 @@ public:
bool running;
bool finished;
bool isInFinish; //when in QThreadPrivate::finish
- bool interruptionRequested;
+ std::atomic<bool> interruptionRequested;
bool exited;
int returnCode;