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>
This commit is contained in:
Marc Mutz 2017-11-21 10:26:35 +01:00
parent 1c0fcbc887
commit a0faf9e236
2 changed files with 10 additions and 5 deletions

View File

@ -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;
}
/*

View File

@ -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;