From 983a0bc5991259ee973e1c23135c50dc3758b646 Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Fri, 24 Mar 2023 23:46:46 +0200 Subject: [PATCH] Prefer QThreadPool over QtConcurrent where we don't care about result QtConcurrent::run()'s return type is marked with [[nodiscard]], which is not nice to code that just needs to move some tasks to a worker thread. On the other hand, QThreadPool satisfies our needs too and Qt doesn't need to construct futures that we won't use. One remark about the screenshot plugin: the task lambda is mutable so it cannot be represented using a std::function. --- src/effects/screenshot/CMakeLists.txt | 1 - .../screenshot/screenshotdbusinterface2.cpp | 137 ++++++++++-------- src/useractions.cpp | 5 +- .../plasmawindowmanagement_interface.cpp | 18 +-- src/wayland/tests/CMakeLists.txt | 4 +- src/wayland/tests/pasteclient.cpp | 5 +- src/wayland/tests/renderingservertest.cpp | 5 +- 7 files changed, 91 insertions(+), 84 deletions(-) diff --git a/src/effects/screenshot/CMakeLists.txt b/src/effects/screenshot/CMakeLists.txt index de3911882c..2de178798d 100644 --- a/src/effects/screenshot/CMakeLists.txt +++ b/src/effects/screenshot/CMakeLists.txt @@ -18,7 +18,6 @@ target_link_libraries(screenshot PRIVATE KF6::Service KF6::I18n - Qt::Concurrent Qt::DBus ) diff --git a/src/effects/screenshot/screenshotdbusinterface2.cpp b/src/effects/screenshot/screenshotdbusinterface2.cpp index 3c43f49b7d..0a34c1a209 100644 --- a/src/effects/screenshot/screenshotdbusinterface2.cpp +++ b/src/effects/screenshot/screenshotdbusinterface2.cpp @@ -16,7 +16,7 @@ #include #include -#include +#include #include #include @@ -27,6 +27,77 @@ namespace KWin { +class ScreenShotWriter2 : public QRunnable +{ +public: + ScreenShotWriter2(FileDescriptor &&fileDescriptor, const QImage &image) + : m_fileDescriptor(std::move(fileDescriptor)) + , m_image(image) + { + } + + void run() override + { + const int flags = fcntl(m_fileDescriptor.get(), F_GETFL, 0); + if (flags == -1) { + qCWarning(KWIN_SCREENSHOT) << "failed to get screenshot fd flags:" << strerror(errno); + return; + } + if (!(flags & O_NONBLOCK)) { + if (fcntl(m_fileDescriptor.get(), F_SETFL, flags | O_NONBLOCK) == -1) { + qCWarning(KWIN_SCREENSHOT) << "failed to make screenshot fd non blocking:" << strerror(errno); + return; + } + } + + QFile file; + if (!file.open(m_fileDescriptor.get(), QIODevice::WriteOnly)) { + qCWarning(KWIN_SCREENSHOT) << Q_FUNC_INFO << "failed to open pipe:" << file.errorString(); + return; + } + + const QByteArrayView buffer(m_image.constBits(), m_image.sizeInBytes()); + qint64 remainingSize = buffer.size(); + + pollfd pfds[1]; + pfds[0].fd = m_fileDescriptor.get(); + pfds[0].events = POLLOUT; + + while (true) { + const int ready = poll(pfds, 1, 60000); + if (ready < 0) { + if (errno != EINTR) { + qCWarning(KWIN_SCREENSHOT) << Q_FUNC_INFO << "poll() failed:" << strerror(errno); + return; + } + } else if (ready == 0) { + qCWarning(KWIN_SCREENSHOT) << Q_FUNC_INFO << "timed out writing to pipe"; + return; + } else if (!(pfds[0].revents & POLLOUT)) { + qCWarning(KWIN_SCREENSHOT) << Q_FUNC_INFO << "pipe is broken"; + return; + } else { + const char *chunk = buffer.constData() + (buffer.size() - remainingSize); + const qint64 writtenCount = file.write(chunk, remainingSize); + + if (writtenCount < 0) { + qCWarning(KWIN_SCREENSHOT) << Q_FUNC_INFO << "write() failed:" << file.errorString(); + return; + } + + remainingSize -= writtenCount; + if (writtenCount == 0 || remainingSize == 0) { + return; + } + } + } + } + +protected: + FileDescriptor m_fileDescriptor; + QImage m_image; +}; + static ScreenShotFlags screenShotFlagsFromOptions(const QVariantMap &options) { ScreenShotFlags flags = ScreenShotFlags(); @@ -49,62 +120,6 @@ static ScreenShotFlags screenShotFlagsFromOptions(const QVariantMap &options) return flags; } -static void writeBufferToPipe(FileDescriptor fileDescriptor, const QByteArray &buffer) -{ - const int flags = fcntl(fileDescriptor.get(), F_GETFL, 0); - if (flags == -1) { - qCWarning(KWIN_SCREENSHOT) << "failed to get screenshot fd flags:" << strerror(errno); - return; - } - if (!(flags & O_NONBLOCK)) { - if (fcntl(fileDescriptor.get(), F_SETFL, flags | O_NONBLOCK) == -1) { - qCWarning(KWIN_SCREENSHOT) << "failed to make screenshot fd non blocking:" << strerror(errno); - return; - } - } - - QFile file; - if (!file.open(fileDescriptor.get(), QIODevice::WriteOnly)) { - qCWarning(KWIN_SCREENSHOT) << Q_FUNC_INFO << "failed to open pipe:" << file.errorString(); - return; - } - - qint64 remainingSize = buffer.size(); - - pollfd pfds[1]; - pfds[0].fd = fileDescriptor.get(); - pfds[0].events = POLLOUT; - - while (true) { - const int ready = poll(pfds, 1, 60000); - if (ready < 0) { - if (errno != EINTR) { - qCWarning(KWIN_SCREENSHOT) << Q_FUNC_INFO << "poll() failed:" << strerror(errno); - return; - } - } else if (ready == 0) { - qCWarning(KWIN_SCREENSHOT) << Q_FUNC_INFO << "timed out writing to pipe"; - return; - } else if (!(pfds[0].revents & POLLOUT)) { - qCWarning(KWIN_SCREENSHOT) << Q_FUNC_INFO << "pipe is broken"; - return; - } else { - const char *chunk = buffer.constData() + (buffer.size() - remainingSize); - const qint64 writtenCount = file.write(chunk, remainingSize); - - if (writtenCount < 0) { - qCWarning(KWIN_SCREENSHOT) << Q_FUNC_INFO << "write() failed:" << file.errorString(); - return; - } - - remainingSize -= writtenCount; - if (writtenCount == 0 || remainingSize == 0) { - return; - } - } - } -} - static const QString s_dbusServiceName = QStringLiteral("org.kde.KWin.ScreenShot2"); static const QString s_dbusInterface = QStringLiteral("org.kde.KWin.ScreenShot2"); static const QString s_dbusObjectPath = QStringLiteral("/org/kde/KWin/ScreenShot2"); @@ -287,11 +302,9 @@ void ScreenShotSinkPipe2::flush(const QImage &image, const QVariantMap &attribut results.insert(QStringLiteral("scale"), double(image.devicePixelRatio())); QDBusConnection::sessionBus().send(m_replyMessage.createReply(results)); - QtConcurrent::run([fileDescriptor = std::move(m_fileDescriptor), image]() mutable { - const QByteArray buffer(reinterpret_cast(image.constBits()), - image.sizeInBytes()); - writeBufferToPipe(std::move(fileDescriptor), buffer); - }); + auto writer = new ScreenShotWriter2(std::move(m_fileDescriptor), image); + writer->setAutoDelete(true); + QThreadPool::globalInstance()->start(writer); } ScreenShotDBusInterface2::ScreenShotDBusInterface2(ScreenShotEffect *effect) diff --git a/src/useractions.cpp b/src/useractions.cpp index 122143083b..299c272706 100644 --- a/src/useractions.cpp +++ b/src/useractions.cpp @@ -48,7 +48,6 @@ #include #include #include -#include #include #include @@ -186,9 +185,7 @@ void UserActionsMenu::helperDialog(const QString &message, Window *window) if (window) { args << QStringLiteral("--embed") << QString::number(window->window()); } - QtConcurrent::run([args]() { - KProcess::startDetached(QStringLiteral("kdialog"), args); - }); + KProcess::startDetached(QStringLiteral("kdialog"), args); } QStringList configModules(bool controlCenter) diff --git a/src/wayland/plasmawindowmanagement_interface.cpp b/src/wayland/plasmawindowmanagement_interface.cpp index e9370347d2..d8e890aad1 100644 --- a/src/wayland/plasmawindowmanagement_interface.cpp +++ b/src/wayland/plasmawindowmanagement_interface.cpp @@ -14,9 +14,9 @@ #include #include #include +#include #include #include -#include #include @@ -466,15 +466,13 @@ void PlasmaWindowInterfacePrivate::setResourceName(const QString &resourceName) void PlasmaWindowInterfacePrivate::org_kde_plasma_window_get_icon(Resource *resource, int32_t fd) { - QtConcurrent::run( - [fd](const QIcon &icon) { - QFile file; - file.open(fd, QIODevice::WriteOnly, QFileDevice::AutoCloseHandle); - QDataStream ds(&file); - ds << icon; - file.close(); - }, - m_icon); + QThreadPool::globalInstance()->start([fd, icon = m_icon]() { + QFile file; + file.open(fd, QIODevice::WriteOnly, QFileDevice::AutoCloseHandle); + QDataStream ds(&file); + ds << icon; + file.close(); + }); } void PlasmaWindowInterfacePrivate::org_kde_plasma_window_request_enter_virtual_desktop(Resource *resource, const QString &id) diff --git a/src/wayland/tests/CMakeLists.txt b/src/wayland/tests/CMakeLists.txt index 5830801bae..0091aebd45 100644 --- a/src/wayland/tests/CMakeLists.txt +++ b/src/wayland/tests/CMakeLists.txt @@ -17,7 +17,7 @@ if (TARGET Qt::Widgets) fakeoutput.cpp ) add_executable(testRenderingServer ${testRenderingServer_SRCS}) - target_link_libraries(testRenderingServer kwin Qt::Concurrent Qt::Widgets) + target_link_libraries(testRenderingServer kwin Qt::Core Qt::Widgets) ecm_mark_as_test(testRenderingServer) endif() @@ -26,7 +26,7 @@ target_link_libraries(copyClient KF6::WaylandClient) ecm_mark_as_test(copyClient) add_executable(pasteClient pasteclient.cpp) -target_link_libraries(pasteClient Qt::Concurrent KF6::WaylandClient) +target_link_libraries(pasteClient Qt::Core KF6::WaylandClient) ecm_mark_as_test(pasteClient) add_executable(touchClientTest touchclienttest.cpp) diff --git a/src/wayland/tests/pasteclient.cpp b/src/wayland/tests/pasteclient.cpp index b844930b0b..788ea68e79 100644 --- a/src/wayland/tests/pasteclient.cpp +++ b/src/wayland/tests/pasteclient.cpp @@ -22,8 +22,7 @@ #include #include #include -#include -#include +#include // system #include @@ -137,7 +136,7 @@ void PasteClient::setupRegistry(Registry *registry) } dataOffer->receive((*it).name(), pipeFds[1]); close(pipeFds[1]); - QtConcurrent::run([pipeFds] { + QThreadPool::globalInstance()->start([pipeFds] { QFile readPipe; if (readPipe.open(pipeFds[0], QIODevice::ReadOnly)) { qDebug() << "Pasted: " << readPipe.readLine(); diff --git a/src/wayland/tests/renderingservertest.cpp b/src/wayland/tests/renderingservertest.cpp index d27d6a08e7..b1b0ce544e 100644 --- a/src/wayland/tests/renderingservertest.cpp +++ b/src/wayland/tests/renderingservertest.cpp @@ -18,11 +18,12 @@ #include #include #include +#include #include #include #include +#include #include -#include #include #include @@ -274,7 +275,7 @@ int main(int argc, char **argv) exit(1); } - QtConcurrent::run([pipe] { + QThreadPool::globalInstance()->start([pipe] { readDisplayFromPipe(pipe); }); }