QmlDesigner: Simplify threading in image cache generator

Change-Id: Ib969e6dae268c4564239d11c761873092e2dbb17
Reviewed-by: Thomas Hartmann <thomas.hartmann@qt.io>
Reviewed-by: Tim Jenssen <tim.jenssen@qt.io>

src/plugins/qmldesigner/designercore/imagecache/imagecachegenerator.cpp

@@ -32,14 +32,23 @@
 
 namespace QmlDesigner {
 
+ImageCacheGenerator::ImageCacheGenerator(ImageCacheCollectorInterface &collector,
+                                         ImageCacheStorageInterface &storage)
+    : m_collector{collector}
+    , m_storage(storage)
+{
+    m_backgroundThread.reset(QThread::create([this]() { startGeneration(); }));
+    m_backgroundThread->start();
+}
+
 ImageCacheGenerator::~ImageCacheGenerator()
 {
-    std::lock_guard threadLock{*m_threadMutex.get()};
+    clean();
+    stopThread();
+    m_condition.notify_all();
 
     if (m_backgroundThread)
         m_backgroundThread->wait();
-
-    clean();
 }
 
 void ImageCacheGenerator::generateImage(Utils::SmallStringView name,
@@ -48,50 +57,32 @@ void ImageCacheGenerator::generateImage(Utils::SmallStringView name,
                                         AbortCallback &&abortCallback)
 {
     {
-        std::lock_guard lock{m_dataMutex};
+        std::lock_guard lock{m_mutex};
         m_tasks.emplace_back(name, timeStamp, std::move(captureCallback), std::move(abortCallback));
     }
 
-    startGenerationAsynchronously();
+    m_condition.notify_all();
 }
 
 void ImageCacheGenerator::clean()
 {
-    std::lock_guard dataLock{m_dataMutex};
+    std::lock_guard lock{m_mutex};
+    for (Task &task : m_tasks)
+        task.abortCallback();
     m_tasks.clear();
 }
 
-class ReleaseProcessing
+void ImageCacheGenerator::startGeneration()
 {
-public:
+    while (isRunning()) {
-    ReleaseProcessing(std::atomic_flag &processing)
+        waitForEntries();
-        : m_processing(processing)
-    {
-        m_processing.test_and_set(std::memory_order_acquire);
-    }
 
-    ~ReleaseProcessing() { m_processing.clear(std::memory_order_release); }
-
-private:
-    std::atomic_flag &m_processing;
-};
-
-void ImageCacheGenerator::startGeneration(std::shared_ptr<std::mutex> threadMutex)
-{
-    ReleaseProcessing guard(m_processing);
-
-    while (true) {
         Task task;
 
         {
-            std::unique_lock threadLock{*threadMutex.get(), std::defer_lock_t{}};
+            std::lock_guard lock{m_mutex};
 
-            if (!threadLock.try_lock())
+            if (m_finishing) {
-                return;
-
-            std::lock_guard dataLock{m_dataMutex};
-
-            if (m_tasks.empty()) {
                 m_storage.walCheckpointFull();
                 return;
             }
@@ -103,15 +94,7 @@ void ImageCacheGenerator::startGeneration(std::shared_ptr<std::mutex> threadMute
 
         m_collector.start(
             task.filePath,
-            [this, threadMutex, task](QImage &&image) {
+            [this, task](QImage &&image) {
-                std::unique_lock lock{*threadMutex.get(), std::defer_lock_t{}};
-
-                if (!lock.try_lock())
-                    return;
-
-                if (threadMutex.use_count() == 1)
-                    return;
-
                 if (image.isNull())
                     task.abortCallback();
                 else
@@ -119,41 +102,34 @@ void ImageCacheGenerator::startGeneration(std::shared_ptr<std::mutex> threadMute
 
                 m_storage.storeImage(std::move(task.filePath), task.timeStamp, image);
             },
-            [this, threadMutex, task] {
+            [this, task] {
-                std::unique_lock lock{*threadMutex.get(), std::defer_lock_t{}};
-
-                if (!lock.try_lock())
-                    return;
-
-                if (threadMutex.use_count() == 1)
-                    return;
-
                 task.abortCallback();
                 m_storage.storeImage(std::move(task.filePath), task.timeStamp, {});
             });
+
+        std::lock_guard lock{m_mutex};
+        if (m_tasks.empty())
+            m_storage.walCheckpointFull();
     }
 }
 
-void ImageCacheGenerator::startGenerationAsynchronously()
+void ImageCacheGenerator::waitForEntries()
 {
-    if (m_processing.test_and_set(std::memory_order_acquire))
+    std::unique_lock lock{m_mutex};
-        return;
+    if (m_tasks.empty())
+        m_condition.wait(lock, [&] { return m_tasks.size() || m_finishing; });
+}
 
-    std::unique_lock lock{*m_threadMutex.get(), std::defer_lock_t{}};
+void ImageCacheGenerator::stopThread()
+{
+    std::unique_lock lock{m_mutex};
+    m_finishing = true;
+}
 
-    if (!lock.try_lock())
+bool ImageCacheGenerator::isRunning()
-        return;
+{
-
+    std::unique_lock lock{m_mutex};
-    if (m_backgroundThread)
+    return !m_finishing;
-        m_backgroundThread->wait();
-
-    m_backgroundThread.reset(QThread::create(
-        [this](std::shared_ptr<std::mutex> threadMutex) { startGeneration(threadMutex); },
-        m_threadMutex));
-    m_backgroundThread->start();
-    //    m_backgroundThread = std::thread(
-    //        [this](std::shared_ptr<std::mutex> threadMutex) { startGeneration(threadMutex); },
-    //        m_threadMutex);
 }
 
 } // namespace QmlDesigner

src/plugins/qmldesigner/designercore/imagecache/imagecachegenerator.h

@@ -46,10 +46,7 @@ class ImageCacheStorageInterface;
 class ImageCacheGenerator final : public ImageCacheGeneratorInterface
 {
 public:
-    ImageCacheGenerator(ImageCacheCollectorInterface &collector, ImageCacheStorageInterface &storage)
+    ImageCacheGenerator(ImageCacheCollectorInterface &collector, ImageCacheStorageInterface &storage);
-        : m_collector{collector}
-        , m_storage(storage)
-    {}
 
     ~ImageCacheGenerator();
 
@@ -79,17 +76,21 @@ private:
         Sqlite::TimeStamp timeStamp;
     };
 
-    void startGeneration(std::shared_ptr<std::mutex> threadMutex);
+    void startGeneration();
-    void startGenerationAsynchronously();
+
+    void waitForEntries();
+    void stopThread();
+    bool isRunning();
 
+private:
 private:
     std::unique_ptr<QThread> m_backgroundThread;
-    std::mutex m_dataMutex;
+    mutable std::mutex m_mutex;
-    std::shared_ptr<std::mutex> m_threadMutex{std::make_shared<std::mutex>()};
+    std::condition_variable m_condition;
     std::vector<Task> m_tasks;
     ImageCacheCollectorInterface &m_collector;
     ImageCacheStorageInterface &m_storage;
-    std::atomic_flag m_processing = ATOMIC_FLAG_INIT;
+    bool m_finishing{false};
 };
 
 } // namespace QmlDesigner

tests/unit/unittest/imagecache-test.cpp

@@ -39,12 +39,12 @@ class ImageCache : public testing::Test
 protected:
     Notification notification;
     Notification waitInThread;
+    NiceMock<MockFunction<void()>> mockAbortCallback;
+    NiceMock<MockFunction<void(const QImage &image)>> mockCaptureCallback;
     NiceMock<MockImageCacheStorage> mockStorage;
     NiceMock<MockImageCacheGenerator> mockGenerator;
     NiceMock<MockTimeStampProvider> mockTimeStampProvider;
     QmlDesigner::ImageCache cache{mockStorage, mockGenerator, mockTimeStampProvider};
-    NiceMock<MockFunction<void()>> mockAbortCallback;
-    NiceMock<MockFunction<void(const QImage &image)>> mockCaptureCallback;
     QImage image1{10, 10, QImage::Format_ARGB32};
 };
 
@@ -260,14 +260,10 @@ TEST_F(ImageCache, RequestIconCallsAbortCallbackFromGenerator)
 
 TEST_F(ImageCache, CleanRemovesEntries)
 {
-    EXPECT_CALL(mockGenerator, generateImage(Eq("/path/to/Component1.qml"), _, _, _))
-        .WillRepeatedly([&](auto &&, auto, auto &&mockCaptureCallback, auto &&) {
-            mockCaptureCallback(QImage{});
-            waitInThread.wait();
-        });
     EXPECT_CALL(mockGenerator, generateImage(_, _, _, _))
         .WillRepeatedly([&](auto &&, auto, auto &&mockCaptureCallback, auto &&) {
             mockCaptureCallback(QImage{});
+            waitInThread.wait();
         });
     cache.requestIcon("/path/to/Component1.qml",
                       mockCaptureCallback.AsStdFunction(),
@@ -284,7 +280,7 @@ TEST_F(ImageCache, CleanRemovesEntries)
 
 TEST_F(ImageCache, CleanCallsAbort)
 {
-    ON_CALL(mockGenerator, generateImage(Eq("/path/to/Component1.qml"), _, _, _))
+    ON_CALL(mockGenerator, generateImage(_, _, _, _))
         .WillByDefault(
             [&](auto &&, auto, auto &&mockCaptureCallback, auto &&) { waitInThread.wait(); });
     cache.requestIcon("/path/to/Component1.qml",

tests/unit/unittest/imagecachegenerator-test.cpp

@@ -106,10 +106,22 @@ TEST_F(ImageCacheGenerator, DontCrashAtDestructingGenerator)
         captureCallback(QImage{image1});
     });
 
-    generator.generateImage("name", {}, imageCallbackMock.AsStdFunction(), {});
+    generator.generateImage("name",
-    generator.generateImage("name2", {}, imageCallbackMock.AsStdFunction(), {});
+                            {},
-    generator.generateImage("name3", {}, imageCallbackMock.AsStdFunction(), {});
+                            imageCallbackMock.AsStdFunction(),
-    generator.generateImage("name4", {}, imageCallbackMock.AsStdFunction(), {});
+                            abortCallbackMock.AsStdFunction());
+    generator.generateImage("name2",
+                            {},
+                            imageCallbackMock.AsStdFunction(),
+                            abortCallbackMock.AsStdFunction());
+    generator.generateImage("name3",
+                            {},
+                            imageCallbackMock.AsStdFunction(),
+                            abortCallbackMock.AsStdFunction());
+    generator.generateImage("name4",
+                            {},
+                            imageCallbackMock.AsStdFunction(),
+                            abortCallbackMock.AsStdFunction());
 }
 
 TEST_F(ImageCacheGenerator, StoreImage)
@@ -168,11 +180,10 @@ TEST_F(ImageCacheGenerator, StoreNullImageForAbortCallback)
 {
     ON_CALL(collectorMock, start(_, _, _)).WillByDefault([&](auto, auto, auto abortCallback) {
         abortCallback();
-        notification.notify();
     });
 
-    EXPECT_CALL(abortCallbackMock, Call()).WillOnce([&]() { notification.notify(); });
+    EXPECT_CALL(storageMock, storeImage(Eq("name"), Eq(Sqlite::TimeStamp{11}), Eq(QImage{})))
-    EXPECT_CALL(storageMock, storeImage(Eq("name"), Eq(Sqlite::TimeStamp{11}), Eq(QImage{})));
+        .WillOnce([&](auto, auto, auto) { notification.notify(); });
 
     generator.generateImage("name",
                             {11},
@@ -183,7 +194,6 @@ TEST_F(ImageCacheGenerator, StoreNullImageForAbortCallback)
 
 TEST_F(ImageCacheGenerator, AbortForEmptyImage)
 {
-    NiceMock<MockFunction<void()>> abortCallbackMock;
     ON_CALL(collectorMock, start(Eq("name"), _, _)).WillByDefault([&](auto, auto captureCallback, auto) {
         captureCallback(QImage{});
     });
@@ -216,7 +226,7 @@ TEST_F(ImageCacheGenerator, CallWalCheckpointFullIfQueueIsEmpty)
     notification.wait();
 }
 
-TEST_F(ImageCacheGenerator, Clean)
+TEST_F(ImageCacheGenerator, CleanIsCallingAbortCallback)
 {
     ON_CALL(collectorMock, start(_, _, _)).WillByDefault([&](auto, auto captureCallback, auto) {
         captureCallback({});
@@ -231,7 +241,7 @@ TEST_F(ImageCacheGenerator, Clean)
                             imageCallbackMock.AsStdFunction(),
                             abortCallbackMock.AsStdFunction());
 
-    EXPECT_CALL(imageCallbackMock, Call(_)).Times(0);
+    EXPECT_CALL(abortCallbackMock, Call()).Times(AtLeast(1));
 
     generator.clean();
     waitInThread.notify();