From 1cf09e5979a0c7f75effaa162130e8bd1d39ced1 Mon Sep 17 00:00:00 2001 From: Marco Bubke Date: Wed, 27 Jan 2021 14:04:12 +0100 Subject: [PATCH] QmlDesigner: Compress generator tasks A document change can generate new image request and if the are not in the database new task for the collector are generated. But if there are already tasks for an equal id they would generated twice. So the tasks are now merged. The auxilialry data is not merged because it is expected that it is not changing. Task-number: QDS-3388 Change-Id: Id1aceb564dd9dc7c5a1ea5ae4680d7ee36611cda Reviewed-by: Thomas Hartmann --- .../imagecache/imagecachegenerator.cpp | 55 ++++--- .../imagecache/imagecachegenerator.h | 11 +- .../unittest/imagecachegenerator-test.cpp | 134 +++++++++++++++++- 3 files changed, 175 insertions(+), 25 deletions(-) diff --git a/src/plugins/qmldesigner/designercore/imagecache/imagecachegenerator.cpp b/src/plugins/qmldesigner/designercore/imagecache/imagecachegenerator.cpp index ea0493176bf..13e1a141c78 100644 --- a/src/plugins/qmldesigner/designercore/imagecache/imagecachegenerator.cpp +++ b/src/plugins/qmldesigner/designercore/imagecache/imagecachegenerator.cpp @@ -56,22 +56,47 @@ void ImageCacheGenerator::generateImage(Utils::SmallStringView name, { { std::lock_guard lock{m_mutex}; - m_tasks.emplace_back(name, - extraId, - std::move(auxiliaryData), - timeStamp, - std::move(captureCallback), - std::move(abortCallback)); + + auto found = std::find_if(m_tasks.begin(), m_tasks.end(), [&](const Task &task) { + return task.filePath == name && task.extraId == extraId; + }); + + if (found != m_tasks.end()) { + found->timeStamp = timeStamp; + found->captureCallbacks.push_back(std::move(captureCallback)); + found->abortCallbacks.push_back(std::move(abortCallback)); + } else { + m_tasks.emplace_back(name, + extraId, + std::move(auxiliaryData), + timeStamp, + std::move(captureCallback), + std::move(abortCallback)); + } } m_condition.notify_all(); } +namespace { +Utils::PathString createId(Utils::SmallStringView name, Utils::SmallStringView extraId) +{ + return extraId.empty() ? Utils::PathString{name} : Utils::PathString{name, "+", extraId}; +} +template +void callCallbacks(const Callbacks &callbacks, Argument &&...arguments) +{ + for (auto &&callback : callbacks) + callback(std::forward(arguments)...); +} + +} // namespace + void ImageCacheGenerator::clean() { std::lock_guard lock{m_mutex}; for (Task &task : m_tasks) - task.abortCallback(); + callCallbacks(task.abortCallbacks); m_tasks.clear(); } @@ -83,12 +108,6 @@ void ImageCacheGenerator::waitForFinished() if (m_backgroundThread) m_backgroundThread->wait(); } -namespace { -Utils::PathString createId(Utils::SmallStringView name, Utils::SmallStringView extraId) -{ - return extraId.empty() ? Utils::PathString{name} : Utils::PathString{name, "+", extraId}; -} -} // namespace void ImageCacheGenerator::startGeneration() { @@ -105,9 +124,9 @@ void ImageCacheGenerator::startGeneration() return; } - task = std::move(m_tasks.back()); + task = std::move(m_tasks.front()); - m_tasks.pop_back(); + m_tasks.pop_front(); } m_collector.start( @@ -116,9 +135,9 @@ void ImageCacheGenerator::startGeneration() std::move(task.auxiliaryData), [this, task](QImage &&image, QImage &&smallImage) { if (image.isNull()) - task.abortCallback(); + callCallbacks(task.abortCallbacks); else - task.captureCallback(image, smallImage); + callCallbacks(task.captureCallbacks, image, smallImage); m_storage.storeImage(createId(task.filePath, task.extraId), task.timeStamp, @@ -126,7 +145,7 @@ void ImageCacheGenerator::startGeneration() smallImage); }, [this, task] { - task.abortCallback(); + callCallbacks(task.abortCallbacks); m_storage.storeImage(createId(task.filePath, task.extraId), task.timeStamp, {}, {}); }); diff --git a/src/plugins/qmldesigner/designercore/imagecache/imagecachegenerator.h b/src/plugins/qmldesigner/designercore/imagecache/imagecachegenerator.h index baa6831754e..78b7c96c937 100644 --- a/src/plugins/qmldesigner/designercore/imagecache/imagecachegenerator.h +++ b/src/plugins/qmldesigner/designercore/imagecache/imagecachegenerator.h @@ -32,6 +32,7 @@ #include +#include #include #include @@ -74,16 +75,16 @@ private: : filePath(filePath) , extraId(std::move(extraId)) , auxiliaryData(std::move(auxiliaryData)) - , captureCallback(std::move(captureCallback)) - , abortCallback(std::move(abortCallback)) + , captureCallbacks({std::move(captureCallback)}) + , abortCallbacks({std::move(abortCallback)}) , timeStamp(timeStamp) {} Utils::PathString filePath; Utils::SmallString extraId; ImageCache::AuxiliaryData auxiliaryData; - CaptureCallback captureCallback; - AbortCallback abortCallback; + std::vector captureCallbacks; + std::vector abortCallbacks; Sqlite::TimeStamp timeStamp; }; @@ -98,7 +99,7 @@ private: std::unique_ptr m_backgroundThread; mutable std::mutex m_mutex; std::condition_variable m_condition; - std::vector m_tasks; + std::deque m_tasks; ImageCacheCollectorInterface &m_collector; ImageCacheStorageInterface &m_storage; bool m_finishing{false}; diff --git a/tests/unit/unittest/imagecachegenerator-test.cpp b/tests/unit/unittest/imagecachegenerator-test.cpp index 9d83f9bbe9f..bcc4a5f47ad 100644 --- a/tests/unit/unittest/imagecachegenerator-test.cpp +++ b/tests/unit/unittest/imagecachegenerator-test.cpp @@ -78,11 +78,11 @@ TEST_F(ImageCacheGenerator, CallsCollectorWithCaptureCallback) TEST_F(ImageCacheGenerator, CallsCollectorOnlyIfNotProcessing) { - EXPECT_CALL(collectorMock, start(Eq("name"), _, _, _, _)) + EXPECT_CALL(collectorMock, start(AnyOf(Eq("name"), Eq("name2")), _, _, _, _)) .WillRepeatedly([&](auto, auto, auto, auto, auto) { notification.notify(); }); generator.generateImage("name", {}, {}, imageCallbackMock.AsStdFunction(), {}, {}); - generator.generateImage("name", {}, {}, imageCallbackMock.AsStdFunction(), {}, {}); + generator.generateImage("name2", {}, {}, imageCallbackMock.AsStdFunction(), {}, {}); notification.wait(2); } @@ -320,4 +320,134 @@ TEST_F(ImageCacheGenerator, CallsCollectorWithAuxiliaryData) notification.wait(); } +TEST_F(ImageCacheGenerator, MergeTasks) +{ + EXPECT_CALL(collectorMock, start(Eq("waitDummy"), _, _, _, _)) + .WillRepeatedly([&](auto, auto, auto, auto, auto) { waitInThread.wait(); }); + EXPECT_CALL(collectorMock, start(Eq("notificationDummy"), _, _, _, _)) + .WillRepeatedly([&](auto, auto, auto, auto, auto) { notification.notify(); }); + + EXPECT_CALL(collectorMock, start(Eq("name"), _, _, _, _)); + + generator.generateImage("waitDummy", {}, {}, {}, {}, {}); + generator.generateImage("name", {}, {}, {}, {}, {}); + generator.generateImage("name", {}, {}, {}, {}, {}); + generator.generateImage("notificationDummy", {}, {}, {}, {}, {}); + waitInThread.notify(); + notification.wait(); +} + +TEST_F(ImageCacheGenerator, DontMergeTasksWithDifferentId) +{ + EXPECT_CALL(collectorMock, start(Eq("waitDummy"), _, _, _, _)) + .WillRepeatedly([&](auto, auto, auto, auto, auto) { waitInThread.wait(); }); + + EXPECT_CALL(collectorMock, start(Eq("name"), _, _, _, _)) + .WillRepeatedly([&](auto, auto, auto, auto, auto) { notification.notify(); }); + EXPECT_CALL(collectorMock, start(Eq("name2"), _, _, _, _)) + .WillRepeatedly([&](auto, auto, auto, auto, auto) { notification.notify(); }); + + generator.generateImage("waitDummy", {}, {}, {}, {}, {}); + generator.generateImage("name", {}, {}, {}, {}, {}); + generator.generateImage("name2", {}, {}, {}, {}, {}); + waitInThread.notify(); + notification.wait(2); +} + +TEST_F(ImageCacheGenerator, MergeTasksWithSameExtraId) +{ + EXPECT_CALL(collectorMock, start(Eq("waitDummy"), _, _, _, _)) + .WillRepeatedly([&](auto, auto, auto, auto, auto) { waitInThread.wait(); }); + EXPECT_CALL(collectorMock, start(Eq("notificationDummy"), _, _, _, _)) + .WillRepeatedly([&](auto, auto, auto, auto, auto) { notification.notify(); }); + + EXPECT_CALL(collectorMock, start(Eq("name"), _, _, _, _)); + + generator.generateImage("waitDummy", {}, {}, {}, {}, {}); + generator.generateImage("name", "id1", {}, {}, {}, {}); + generator.generateImage("name", "id1", {}, {}, {}, {}); + waitInThread.notify(); + generator.generateImage("notificationDummy", {}, {}, {}, {}, {}); + notification.wait(); +} + +TEST_F(ImageCacheGenerator, DontMergeTasksWithDifferentExtraId) +{ + EXPECT_CALL(collectorMock, start(Eq("waitDummy"), _, _, _, _)) + .WillRepeatedly([&](auto, auto, auto, auto, auto) { waitInThread.wait(); }); + + EXPECT_CALL(collectorMock, start(Eq("name"), _, _, _, _)) + .Times(2) + .WillRepeatedly([&](auto, auto, auto, auto, auto) { notification.notify(); }); + + generator.generateImage("waitDummy", {}, {}, {}, {}, {}); + generator.generateImage("name", "id1", {}, {}, {}, {}); + generator.generateImage("name", "id2", {}, {}, {}, {}); + waitInThread.notify(); + notification.wait(2); +} + +TEST_F(ImageCacheGenerator, UseLastTimeStampIfTasksAreMerged) +{ + ON_CALL(collectorMock, start(Eq("waitDummy"), _, _, _, _)) + .WillByDefault([&](auto, auto, auto, auto, auto) { waitInThread.wait(); }); + ON_CALL(collectorMock, start(Eq("notificationDummy"), _, _, _, _)) + .WillByDefault([&](auto, auto, auto, auto, auto) { notification.notify(); }); + ON_CALL(collectorMock, start(Eq("name"), _, _, _, _)) + .WillByDefault([&](auto, auto, auto, auto, auto abortCallback) { abortCallback(); }); + + EXPECT_CALL(storageMock, storeImage(Eq("name"), _, _, _)); + + generator.generateImage("waitDummy", {}, {}, {}, {}, {}); + generator.generateImage("name", {}, {3}, {}, abortCallbackMock.AsStdFunction(), {}); + generator.generateImage("name", {}, {4}, {}, abortCallbackMock.AsStdFunction(), {}); + generator.generateImage("notificationDummy", {}, {}, {}, {}, {}); + waitInThread.notify(); + notification.wait(); +} + +TEST_F(ImageCacheGenerator, MergeCaptureCallbackIfTasksAreMerged) +{ + NiceMock> newerImageCallbackMock; + ON_CALL(collectorMock, start(Eq("waitDummy"), _, _, _, _)) + .WillByDefault([&](auto, auto, auto, auto, auto) { waitInThread.wait(); }); + ON_CALL(collectorMock, start(Eq("notificationDummy"), _, _, _, _)) + .WillByDefault([&](auto, auto, auto, auto, auto) { notification.notify(); }); + ON_CALL(collectorMock, start(Eq("name"), _, _, _, _)) + .WillByDefault([&](auto, auto, auto, auto imageCallback, auto) { + imageCallback(QImage{image1}, QImage{smallImage1}); + }); + + EXPECT_CALL(imageCallbackMock, Call(_, _)); + EXPECT_CALL(newerImageCallbackMock, Call(_, _)); + + generator.generateImage("waitDummy", {}, {}, {}, {}, {}); + generator.generateImage("name", {}, {}, imageCallbackMock.AsStdFunction(), {}, {}); + generator.generateImage("name", {}, {}, newerImageCallbackMock.AsStdFunction(), {}, {}); + generator.generateImage("notificationDummy", {}, {}, {}, {}, {}); + waitInThread.notify(); + notification.wait(); +} + +TEST_F(ImageCacheGenerator, MergeAbortCallbackIfTasksAreMerged) +{ + NiceMock> newerAbortCallbackMock; + ON_CALL(collectorMock, start(Eq("waitDummy"), _, _, _, _)) + .WillByDefault([&](auto, auto, auto, auto, auto) { waitInThread.wait(); }); + ON_CALL(collectorMock, start(Eq("notificationDummy"), _, _, _, _)) + .WillByDefault([&](auto, auto, auto, auto, auto) { notification.notify(); }); + ON_CALL(collectorMock, start(Eq("name"), _, _, _, _)) + .WillByDefault([&](auto, auto, auto, auto, auto abortCallback) { abortCallback(); }); + + EXPECT_CALL(abortCallbackMock, Call()); + EXPECT_CALL(newerAbortCallbackMock, Call()); + + generator.generateImage("waitDummy", {}, {}, {}, {}, {}); + generator.generateImage("name", {}, {}, {}, abortCallbackMock.AsStdFunction(), {}); + generator.generateImage("name", {}, {}, {}, newerAbortCallbackMock.AsStdFunction(), {}); + generator.generateImage("notificationDummy", {}, {}, {}, {}, {}); + waitInThread.notify(); + notification.wait(); +} + } // namespace