From cb7c9672527ebd29b925bbc384e92ad2fc785d75 Mon Sep 17 00:00:00 2001 From: Igor Kushnir Date: Tue, 12 Nov 2019 12:27:14 +0200 Subject: [PATCH] Add WorkerThread class and use it in ComicFlow In a later commit WorkerThread should also replace classes similar to ImageLoader: PageLoader, ImageLoaderGL and ImageLoaderByteArrayGL. Bugs fixed: 1. Eliminated a data race between ImageLoader::run() and ComicFlow::updateImageData()->ImageLoader::result(). Specifically when ImageLoader::busy() returns false, then ImageLoader::run() sets ImageLoader::working to true, loads the image and starts assigning it to ImageLoader::img, while ImageLoader::result() is accessed without locking from updateImageData(). Making ImageLoader::working atomic is clearly insufficient to eliminate this data race. The fix is to set 'working' to true immediately and synchronously as soon as a new task is assigned to the worker. 2. Replaced thread termination with graceful thread exit. ComicFlow destructor called QThread::terminate(), using which is discouraged by Qt documentation. The application exited without errors in Release mode. In Debug mode, however, it received the SIG32 signal on exit and printed the following warning - "QWaitCondition: mutex destroy failure: Device or resource busy". The loop in WorkerThread::run() is no longer endless. The worker thread properly ends and is joined in WorkerThread destructor. Design decisions: 1. WorkerThread could emit a signal when it completes a task. Thus updateTimer could be removed from ComicFlow and GoToFlow. However, there is no obvious way to use this new signal in the two GL classes. Also I don't know whether updateTimer is just an inefficient polling substitute for notification or an intentional animation mechanism. 2. The index variable is no longer stored in the worker class, but in ComicFlow directly. Thing is, this data member was never actually accessed by the worker, but ComicFlow went so far as to lock worker's mutex to "protect" access to the index. 3. The common ImageLoader implementation turned out to be very general. So I converted it into the WorkerThread class template that is not restricted to producing QImage results and can be reused elsewhere. 4. I used standard classes (such as std::thread) instead of their Qt equivalents (e.g. QThread) because they are more thoroughly documented. The standard classes should also be more efficient as they were more carefully designed and provide much fewer unnecessary features. 5. Release-Acquire ordering is safe for the WorkerThread::working use case and is more efficient than the std::atomic-default Sequentially-consistent ordering. 6. condition.notify_one() is called while the mutex is unlocked to improve performance. This is safe in both cases: a) if the worker thread exits due to a spurious wakeup just before the condition.notify_one() call in WorkerThread destructor, so much the better; b) if a spurious wakeup lets the worker thread finish the task and start waiting on the condition again just before the condition.notify_one() call in WorkerThread::performTask(), the second waking will be ignored by the worker thread as 'working' and 'abort' will be false then. --- YACReaderLibrary/YACReaderLibrary.pro | 2 + YACReaderLibrary/comic_flow.cpp | 129 +++----------------------- YACReaderLibrary/comic_flow.h | 50 ++-------- common/release_acquire_atomic.h | 28 ++++++ common/worker_thread.h | 91 ++++++++++++++++++ 5 files changed, 145 insertions(+), 155 deletions(-) create mode 100644 common/release_acquire_atomic.h create mode 100644 common/worker_thread.h diff --git a/YACReaderLibrary/YACReaderLibrary.pro b/YACReaderLibrary/YACReaderLibrary.pro index 26fd2e50..2e6d6888 100644 --- a/YACReaderLibrary/YACReaderLibrary.pro +++ b/YACReaderLibrary/YACReaderLibrary.pro @@ -107,6 +107,8 @@ HEADERS += comic_flow.h \ ../common/comic.h \ ../common/bookmarks.h \ ../common/pictureflow.h \ + ../common/release_acquire_atomic.h \ + ../common/worker_thread.h \ ../common/custom_widgets.h \ ../common/qnaturalsorting.h \ ../common/yacreader_global.h \ diff --git a/YACReaderLibrary/comic_flow.cpp b/YACReaderLibrary/comic_flow.cpp index 1bd5f32a..dd0b4402 100644 --- a/YACReaderLibrary/comic_flow.cpp +++ b/YACReaderLibrary/comic_flow.cpp @@ -1,21 +1,17 @@ #include "comic_flow.h" -#include "qnaturalsorting.h" +#include "worker_thread.h" #include "yacreader_global.h" -#include - -#include -#include #include ComicFlow::ComicFlow(QWidget *parent, FlowType flowType) - : YACReaderFlow(parent, flowType) + : YACReaderFlow(parent, flowType), worker(new WorkerThread) { + resetWorkerIndex(); updateTimer = new QTimer; connect(updateTimer, SIGNAL(timeout()), this, SLOT(updateImageData())); - worker = new ImageLoader; connect(this, SIGNAL(centerIndexChanged(int)), this, SLOT(preload())); connect(this, SIGNAL(centerIndexChangedSilent(int)), this, SLOT(preload())); @@ -24,8 +20,6 @@ ComicFlow::ComicFlow(QWidget *parent, FlowType flowType) ComicFlow::~ComicFlow() { - worker->terminate(); - delete worker; delete updateTimer; } @@ -33,7 +27,6 @@ void ComicFlow::setImagePaths(const QStringList &paths) { clear(); - //imagePath = path; imageFiles = paths; imagesLoaded.clear(); imagesLoaded.fill(false, imageFiles.size()); @@ -54,7 +47,8 @@ void ComicFlow::setImagePaths(const QStringList &paths) } setCenterIndex(0); - worker->reset(); + + resetWorkerIndex(); preload(); } @@ -71,10 +65,11 @@ void ComicFlow::updateImageData() return; // set image of last one - int idx = worker->index(); - if (idx >= 0 && !worker->result().isNull()) { - if (!imagesSetted[idx]) { - setSlide(idx, worker->result()); + const int idx = workerIndex; + if (idx >= 0) { + const QImage result = worker->extractResult(); + if (!result.isNull() && !imagesSetted[idx]) { + setSlide(idx, result); imagesSetted[idx] = true; numImagesLoaded++; imagesLoaded[idx] = true; @@ -99,7 +94,8 @@ void ComicFlow::updateImageData() // schedule thumbnail generation QString fname = imageFiles[i]; - worker->generate(i, fname, slideSize()); + workerIndex = i; + worker->performTask([fname] { return QImage { fname }; }); return; } } @@ -124,10 +120,6 @@ void ComicFlow::wheelEvent(QWheelEvent *event) void ComicFlow::removeSlide(int cover) { - worker->lock(); - - worker->reset(); - imageFiles.removeAt(cover); if (imagesLoaded[cover]) numImagesLoaded--; @@ -135,16 +127,13 @@ void ComicFlow::removeSlide(int cover) imagesSetted.remove(cover); YACReaderFlow::removeSlide(cover); - worker->unlock(); + resetWorkerIndex(); preload(); } void ComicFlow::resortCovers(QList newOrder) { - worker->lock(); - worker->reset(); - YACReaderFlow::resortCovers(newOrder); QStringList imageFilesNew; @@ -160,95 +149,5 @@ void ComicFlow::resortCovers(QList newOrder) imagesLoaded = imagesLoadedNew; imagesSetted = imagesSettedNew; - worker->unlock(); -} -//----------------------------------------------------------------------------- -//ImageLoader -//----------------------------------------------------------------------------- -static QImage loadImage(const QString &fileName) -{ - QImage image; - bool result = image.load(fileName); - - if (!result) - return QImage(); - - return image; -} - -ImageLoader::ImageLoader() - : QThread(), restart(false), working(false), idx(-1) -{ -} - -ImageLoader::~ImageLoader() -{ - mutex.lock(); - condition.wakeOne(); - mutex.unlock(); - wait(); -} - -bool ImageLoader::busy() const -{ - return isRunning() ? working : false; -} - -void ImageLoader::generate(int index, const QString &fileName, QSize size) -{ - mutex.lock(); - this->idx = index; - this->fileName = fileName; - this->size = size; - this->img = QImage(); - mutex.unlock(); - - if (!isRunning()) - start(); - else { - // already running, wake up whenever ready - restart = true; - condition.wakeOne(); - } -} - -void ImageLoader::lock() -{ - mutex.lock(); -} - -void ImageLoader::unlock() -{ - mutex.unlock(); -} - -void ImageLoader::run() -{ - for (;;) { - // copy necessary data - mutex.lock(); - this->working = true; - QString fileName = this->fileName; - mutex.unlock(); - - QImage image = loadImage(fileName); - - // let everyone knows it is ready - mutex.lock(); - this->working = false; - this->img = image; - mutex.unlock(); - - // put to sleep - mutex.lock(); - if (!this->restart) - condition.wait(&mutex); - restart = false; - mutex.unlock(); - } -} - -QImage ImageLoader::result() -{ - return img; + resetWorkerIndex(); } diff --git a/YACReaderLibrary/comic_flow.h b/YACReaderLibrary/comic_flow.h index 78b91582..eff0b433 100644 --- a/YACReaderLibrary/comic_flow.h +++ b/YACReaderLibrary/comic_flow.h @@ -4,21 +4,21 @@ #include "yacreader_flow.h" #include -#include -#include #include -#include -#include #include #include -class ImageLoader; +#include + +template +class WorkerThread; + class ComicFlow : public YACReaderFlow { Q_OBJECT public: ComicFlow(QWidget *parent = nullptr, FlowType flowType = CoverFlowLike); - virtual ~ComicFlow(); + ~ComicFlow() override; void setImagePaths(const QStringList &paths); //bool eventFilter(QObject *target, QEvent *event); @@ -31,46 +31,16 @@ private slots: void updateImageData(); private: - //QString imagePath; + void resetWorkerIndex() { workerIndex = -1; } + QStringList imageFiles; QVector imagesLoaded; QVector imagesSetted; int numImagesLoaded; + int workerIndex; QTimer *updateTimer; - ImageLoader *worker; + std::unique_ptr> worker; virtual void wheelEvent(QWheelEvent *event); }; -//----------------------------------------------------------------------------- -// Source code of ImageLoader class was modified from http://code.google.com/p/photoflow/ -//------------------------------------------------------------------------------ -class ImageLoader : public QThread -{ -public: - ImageLoader(); - ~ImageLoader() override; - // returns FALSE if worker is still busy and can't take the task - bool busy() const; - void generate(int index, const QString &fileName, QSize size); - void reset() { idx = -1; }; - int index() const { return idx; }; - void lock(); - void unlock(); - QImage result(); - -protected: - void run() override; - -private: - QMutex mutex; - QWaitCondition condition; - - bool restart; - bool working; - int idx; - QString fileName; - QSize size; - QImage img; -}; - #endif diff --git a/common/release_acquire_atomic.h b/common/release_acquire_atomic.h new file mode 100644 index 00000000..4255cea2 --- /dev/null +++ b/common/release_acquire_atomic.h @@ -0,0 +1,28 @@ +#ifndef RELEASE_ACQUIRE_ATOMIC_H +#define RELEASE_ACQUIRE_ATOMIC_H + +#include + +template +class ReleaseAcquireAtomic +{ +public: + constexpr ReleaseAcquireAtomic(T desired) noexcept + : value(desired) {} + + T operator=(T desired) noexcept + { + value.store(desired, std::memory_order_release); + return desired; + } + + operator T() const noexcept + { + return value.load(std::memory_order_acquire); + } + +private: + std::atomic value; +}; + +#endif // RELEASE_ACQUIRE_ATOMIC_H diff --git a/common/worker_thread.h b/common/worker_thread.h new file mode 100644 index 00000000..eaad8047 --- /dev/null +++ b/common/worker_thread.h @@ -0,0 +1,91 @@ +#ifndef WORKER_THREAD_H +#define WORKER_THREAD_H + +#include "release_acquire_atomic.h" + +#include +#include +#include +#include +#include +#include + +//! Usage: +//! 1. call performTask(); +//! 2. wait until busy() returns false; +//! 3. (optionally) call extractResult(); +//! 4. return to step 1 (assign another task). +//! You may invoke busy() and the destructor at any moment. +template +class WorkerThread +{ +public: + WorkerThread() = default; + ~WorkerThread(); + + using Task = std::function; + void performTask(Task newTask); + + //! @return true if the last assigned task is not done yet. + bool busy() const { return working; } + Result extractResult() { return std::move(result); } + +private: + void run(); + + Task task; + Result result; + + ReleaseAcquireAtomic working { false }; + bool abort { false }; + std::mutex mutex; + std::condition_variable condition; + std::thread thread; +}; + +template +WorkerThread::~WorkerThread() +{ + { + std::lock_guard lock(mutex); + abort = true; + } + condition.notify_one(); + if (thread.joinable()) + thread.join(); +} + +template +void WorkerThread::performTask(Task newTask) +{ + assert(!working && "Don't interrupt my work!"); + assert(newTask && "The task may not be empty."); + task = std::move(newTask); + + if (thread.joinable()) { + { + std::lock_guard lock(mutex); + working = true; + } + condition.notify_one(); + } else { + working = true; + thread = std::thread(&WorkerThread::run, this); + } +} + +template +void WorkerThread::run() +{ + while (true) { + result = task(); + working = false; + + std::unique_lock lock(mutex); + condition.wait(lock, [this] { return working || abort; }); + if (abort) + break; + } +} + +#endif // WORKER_THREAD_H