From ce59ccb3341223cda4f7ef2469d86326c6a116e8 Mon Sep 17 00:00:00 2001 From: Sona Kurazyan Date: Wed, 6 Oct 2021 10:04:54 +0200 Subject: [PATCH] Optimize QPromise destructor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unify cancel and finish in QPromise destructor in a single call. This saves us one extra mutex lock and atomic state change. Task-number: QTBUG-84977 Change-Id: Iac06302c39a2863008b27325fcf6792d4f58c8ae Reviewed-by: Mårten Nordheim --- src/corelib/thread/qfutureinterface.cpp | 32 +++++++++++++--- src/corelib/thread/qfutureinterface.h | 5 +++ src/corelib/thread/qpromise.h | 6 +-- .../corelib/thread/qfuture/tst_qfuture.cpp | 34 +++++++++++++++++ .../qfuturewatcher/tst_qfuturewatcher.cpp | 38 +++++++++++++++++++ 5 files changed, 106 insertions(+), 9 deletions(-) diff --git a/src/corelib/thread/qfutureinterface.cpp b/src/corelib/thread/qfutureinterface.cpp index b34385897c..e640114bb0 100644 --- a/src/corelib/thread/qfutureinterface.cpp +++ b/src/corelib/thread/qfutureinterface.cpp @@ -110,14 +110,36 @@ static inline int switch_from_to(QAtomicInt &a, int from, int to) void QFutureInterfaceBase::cancel() { - QMutexLocker locker(&d->m_mutex); - if (d->state.loadRelaxed() & Canceled) - return; + cancel(CancelMode::CancelOnly); +} + +void QFutureInterfaceBase::cancel(QFutureInterfaceBase::CancelMode mode) +{ + QMutexLocker locker(&d->m_mutex); + + const auto oldState = d->state.loadRelaxed(); + + switch (mode) { + case CancelMode::CancelAndFinish: + if ((oldState & Finished) && (oldState & Canceled)) + return; + switch_from_to(d->state, suspendingOrSuspended | Running, Canceled | Finished); + break; + case CancelMode::CancelOnly: + if (oldState & Canceled) + return; + switch_from_to(d->state, suspendingOrSuspended, Canceled); + break; + } - switch_from_to(d->state, suspendingOrSuspended, Canceled); d->waitCondition.wakeAll(); d->pausedWaitCondition.wakeAll(); - d->sendCallOut(QFutureCallOutEvent(QFutureCallOutEvent::Canceled)); + + if (!(oldState & Canceled)) + d->sendCallOut(QFutureCallOutEvent(QFutureCallOutEvent::Canceled)); + if (mode == CancelMode::CancelAndFinish && !(oldState & Finished)) + d->sendCallOut(QFutureCallOutEvent(QFutureCallOutEvent::Finished)); + d->isValid = false; } diff --git a/src/corelib/thread/qfutureinterface.h b/src/corelib/thread/qfutureinterface.h index ecc31d6ec3..e813031d59 100644 --- a/src/corelib/thread/qfutureinterface.h +++ b/src/corelib/thread/qfutureinterface.h @@ -154,6 +154,8 @@ public: int loadState() const; void cancel(); + void cancelAndFinish() { cancel(CancelMode::CancelAndFinish); } + void setSuspended(bool suspend); void toggleSuspended(); void reportSuspended() const; @@ -216,6 +218,9 @@ protected: bool launchAsync() const; bool isRunningOrPending() const; + + enum class CancelMode { CancelOnly, CancelAndFinish }; + void cancel(CancelMode mode); }; inline void swap(QFutureInterfaceBase &lhs, QFutureInterfaceBase &rhs) noexcept diff --git a/src/corelib/thread/qpromise.h b/src/corelib/thread/qpromise.h index a1ac8eaf27..3cecbd8306 100644 --- a/src/corelib/thread/qpromise.h +++ b/src/corelib/thread/qpromise.h @@ -70,10 +70,8 @@ public: return; // Otherwise, if computation is not finished at this point, cancel // potential waits - if (!(state & QFutureInterfaceBase::State::Finished)) { - d.cancel(); - finish(); // required to finalize the state - } + if (!(state & QFutureInterfaceBase::State::Finished)) + d.cancelAndFinish(); // cancel and finalize the state } // Core QPromise APIs diff --git a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp index 7703261832..a6be2e9727 100644 --- a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp +++ b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp @@ -107,6 +107,7 @@ private slots: void futureInterface(); void refcounting(); void cancel(); + void cancelAndFinish(); void statePropagation(); void multipleResults(); void indexedResults(); @@ -865,6 +866,39 @@ void tst_QFuture::cancel() } } +void tst_QFuture::cancelAndFinish() +{ + { + QFutureInterface fi; + + fi.reportStarted(); + fi.cancelAndFinish(); + + QVERIFY(fi.isStarted()); + QVERIFY(!fi.isRunning()); + QVERIFY(!fi.isSuspended()); + QVERIFY(!fi.isSuspending()); + QVERIFY(fi.isCanceled()); + QVERIFY(fi.isFinished()); + } + + // The same with suspended state + { + QFutureInterface fi; + + fi.reportStarted(); + fi.setSuspended(true); + fi.cancelAndFinish(); + + QVERIFY(fi.isStarted()); + QVERIFY(!fi.isRunning()); + QVERIFY(!fi.isSuspended()); + QVERIFY(!fi.isSuspending()); + QVERIFY(fi.isCanceled()); + QVERIFY(fi.isFinished()); + } +} + void tst_QFuture::statePropagation() { QFuture f1; diff --git a/tests/auto/corelib/thread/qfuturewatcher/tst_qfuturewatcher.cpp b/tests/auto/corelib/thread/qfuturewatcher/tst_qfuturewatcher.cpp index 33ecfb9cab..34068f4f51 100644 --- a/tests/auto/corelib/thread/qfuturewatcher/tst_qfuturewatcher.cpp +++ b/tests/auto/corelib/thread/qfuturewatcher/tst_qfuturewatcher.cpp @@ -46,6 +46,8 @@ private slots: void startFinish(); void progressValueChanged(); void canceled(); + void cancelAndFinish_data(); + void cancelAndFinish(); void resultAt(); void resultReadyAt(); void futureSignals(); @@ -229,6 +231,42 @@ void tst_QFutureWatcher::canceled() future.waitForFinished(); } +void tst_QFutureWatcher::cancelAndFinish_data() +{ + QTest::addColumn("isCanceled"); + QTest::addColumn("isFinished"); + + QTest::addRow("running") << false << false; + QTest::addRow("canceled") << true << false; + QTest::addRow("finished") << false << true; + QTest::addRow("canceledAndFinished") << true << true; +} + +void tst_QFutureWatcher::cancelAndFinish() +{ + QFETCH(bool, isCanceled); + QFETCH(bool, isFinished); + + QFutureInterface fi; + QFutureWatcher futureWatcher; + QSignalSpy finishedSpy(&futureWatcher, &QFutureWatcher::finished); + QSignalSpy canceledSpy(&futureWatcher, &QFutureWatcher::canceled); + futureWatcher.setFuture(fi.future()); + + fi.reportStarted(); + + if (isCanceled) + fi.cancel(); + if (isFinished) + fi.reportFinished(); + + fi.cancelAndFinish(); + + // The signals should be emitted only once + QTRY_COMPARE(canceledSpy.count(), 1); + QTRY_COMPARE(finishedSpy.count(), 1); +} + class IntTask : public RunFunctionTaskBase { public: