From 6460c3c33d8f880c50e2b529827437e442d05bd3 Mon Sep 17 00:00:00 2001 From: Sona Kurazyan Date: Mon, 7 Jun 2021 20:15:39 +0200 Subject: QFuture: put the result store and the exception store in a union QFuture doesn't need both at the same time, calling QFuture::result(s) either returns a result or throws an exception. Store result and exception stores in a union, to reduce the memory. Also added a note for making the ResultStoreBase destructor non-virtual in Qt 7. Task-number: QTBUG-92045 Change-Id: I7f0ac03804d19cc67c1a1466c7a1365219768a14 Reviewed-by: Andrei Golubev Reviewed-by: Fabian Kosmale --- src/corelib/thread/qexception.cpp | 6 +++ src/corelib/thread/qexception.h | 2 + src/corelib/thread/qfuture_impl.h | 12 +++--- src/corelib/thread/qfutureinterface.cpp | 73 +++++++++++++++++++++++---------- src/corelib/thread/qfutureinterface.h | 34 ++++++++++++++- src/corelib/thread/qfutureinterface_p.h | 21 ++++++++-- src/corelib/thread/qresultstore.h | 1 + 7 files changed, 118 insertions(+), 31 deletions(-) (limited to 'src/corelib') diff --git a/src/corelib/thread/qexception.cpp b/src/corelib/thread/qexception.cpp index c9b7fb6cf6..8967e9db1c 100644 --- a/src/corelib/thread/qexception.cpp +++ b/src/corelib/thread/qexception.cpp @@ -252,6 +252,12 @@ void ExceptionStore::throwPossibleException() std::rethrow_exception(exceptionHolder); } +void ExceptionStore::rethrowException() const +{ + Q_ASSERT(hasException()); + std::rethrow_exception(exceptionHolder); +} + } // namespace QtPrivate #endif //Q_CLANG_QDOC diff --git a/src/corelib/thread/qexception.h b/src/corelib/thread/qexception.h index 986eb43e1b..1c69287615 100644 --- a/src/corelib/thread/qexception.h +++ b/src/corelib/thread/qexception.h @@ -96,6 +96,7 @@ public: bool hasException() const; std::exception_ptr exception() const; void throwPossibleException(); + Q_NORETURN void rethrowException() const; std::exception_ptr exceptionHolder; }; @@ -110,6 +111,7 @@ class Q_CORE_EXPORT ExceptionStore public: ExceptionStore() { } inline void throwPossibleException() {} + inline void rethrowException() const { } }; } // namespace QtPrivate diff --git a/src/corelib/thread/qfuture_impl.h b/src/corelib/thread/qfuture_impl.h index e455a74793..511caafdf7 100644 --- a/src/corelib/thread/qfuture_impl.h +++ b/src/corelib/thread/qfuture_impl.h @@ -427,7 +427,7 @@ bool Continuation::execute() if (parentFuture.isCanceled()) { #ifndef QT_NO_EXCEPTIONS - if (parentFuture.d.exceptionStore().hasException()) { + if (parentFuture.d.hasException()) { // If the continuation doesn't take a QFuture argument, propagate the exception // to the caller, by reporting it. If the continuation takes a QFuture argument, // the user may want to catch the exception inside the continuation, to not @@ -663,7 +663,7 @@ void FailureHandler::run() promise.reportStarted(); - if (parentFuture.d.exceptionStore().hasException()) { + if (parentFuture.d.hasException()) { using ArgType = typename QtPrivate::ArgResolver::First; if constexpr (std::is_void_v) { handleAllExceptions(); @@ -681,7 +681,8 @@ template void FailureHandler::handleException() { try { - parentFuture.d.exceptionStore().throwPossibleException(); + Q_ASSERT(parentFuture.d.hasException()); + parentFuture.d.exceptionStore().rethrowException(); } catch (const ArgType &e) { try { // Handle exceptions matching with the handler's argument type @@ -707,7 +708,8 @@ template void FailureHandler::handleAllExceptions() { try { - parentFuture.d.exceptionStore().throwPossibleException(); + Q_ASSERT(parentFuture.d.hasException()); + parentFuture.d.exceptionStore().rethrowException(); } catch (...) { try { QtPrivate::fulfillPromise(promise, std::forward(handler)); @@ -766,7 +768,7 @@ public: if (parentFuture.isCanceled()) { #ifndef QT_NO_EXCEPTIONS - if (parentFuture.d.exceptionStore().hasException()) { + if (parentFuture.d.hasException()) { // Propagate the exception to the result future promise.reportException(parentFuture.d.exceptionStore().exception()); } else { diff --git a/src/corelib/thread/qfutureinterface.cpp b/src/corelib/thread/qfutureinterface.cpp index 9bb7d505e2..35883d71e1 100644 --- a/src/corelib/thread/qfutureinterface.cpp +++ b/src/corelib/thread/qfutureinterface.cpp @@ -361,7 +361,8 @@ void QFutureInterfaceBase::reportException(const std::exception_ptr &exception) if (d->state.loadRelaxed() & (Canceled|Finished)) return; - d->m_exceptionStore.setException(exception); + d->hasException = true; + d->data.setException(exception); switch_on(d->state, Canceled); d->waitCondition.wakeAll(); d->pausedWaitCondition.wakeAll(); @@ -406,7 +407,8 @@ int QFutureInterfaceBase::loadState() const void QFutureInterfaceBase::waitForResult(int resultIndex) { - d->m_exceptionStore.throwPossibleException(); + if (d->hasException) + d->data.m_exceptionStore.rethrowException(); QMutexLocker lock(&d->m_mutex); if (!isRunningOrPending()) @@ -423,7 +425,8 @@ void QFutureInterfaceBase::waitForResult(int resultIndex) while (isRunningOrPending() && !d->internal_isResultReadyAt(waitIndex)) d->waitCondition.wait(&d->m_mutex); - d->m_exceptionStore.throwPossibleException(); + if (d->hasException) + d->data.m_exceptionStore.rethrowException(); } void QFutureInterfaceBase::waitForFinished() @@ -441,7 +444,8 @@ void QFutureInterfaceBase::waitForFinished() d->waitCondition.wait(&d->m_mutex); } - d->m_exceptionStore.throwPossibleException(); + if (d->hasException) + d->data.m_exceptionStore.rethrowException(); } void QFutureInterfaceBase::reportResultsReady(int beginIndex, int endIndex) @@ -488,7 +492,8 @@ QThreadPool *QFutureInterfaceBase::threadPool() const void QFutureInterfaceBase::setFilterMode(bool enable) { QMutexLocker locker(&d->m_mutex); - resultStoreBase().setFilterMode(enable); + if (!hasException()) + resultStoreBase().setFilterMode(enable); } /*! @@ -558,19 +563,27 @@ QMutex &QFutureInterfaceBase::mutex() const return d->m_mutex; } +bool QFutureInterfaceBase::hasException() const +{ + return d->hasException; +} + QtPrivate::ExceptionStore &QFutureInterfaceBase::exceptionStore() { - return d->m_exceptionStore; + Q_ASSERT(d->hasException); + return d->data.m_exceptionStore; } QtPrivate::ResultStoreBase &QFutureInterfaceBase::resultStoreBase() { - return d->m_results; + Q_ASSERT(!d->hasException); + return d->data.m_results; } const QtPrivate::ResultStoreBase &QFutureInterfaceBase::resultStoreBase() const { - return d->m_results; + Q_ASSERT(!d->hasException); + return d->data.m_results; } QFutureInterfaceBase &QFutureInterfaceBase::operator=(const QFutureInterfaceBase &other) @@ -609,7 +622,8 @@ void QFutureInterfaceBase::reset() void QFutureInterfaceBase::rethrowPossibleException() { - exceptionStore().throwPossibleException(); + if (hasException()) + exceptionStore().rethrowException(); } QFutureInterfaceBasePrivate::QFutureInterfaceBasePrivate(QFutureInterfaceBase::State initialState) @@ -618,25 +632,38 @@ QFutureInterfaceBasePrivate::QFutureInterfaceBasePrivate(QFutureInterfaceBase::S progressTime.invalidate(); } +QFutureInterfaceBasePrivate::~QFutureInterfaceBasePrivate() +{ + if (hasException) + data.m_exceptionStore.~ExceptionStore(); + else + data.m_results.~ResultStoreBase(); +} + int QFutureInterfaceBasePrivate::internal_resultCount() const { - return m_results.count(); // ### subtract canceled results. + return hasException ? 0 : data.m_results.count(); // ### subtract canceled results. } bool QFutureInterfaceBasePrivate::internal_isResultReadyAt(int index) const { - return (m_results.contains(index)); + return hasException ? false : (data.m_results.contains(index)); } bool QFutureInterfaceBasePrivate::internal_waitForNextResult() { - if (m_results.hasNextResult()) + if (hasException) + return false; + + if (data.m_results.hasNextResult()) return true; - while ((state.loadRelaxed() & QFutureInterfaceBase::Running) && m_results.hasNextResult() == false) + while ((state.loadRelaxed() & QFutureInterfaceBase::Running) + && data.m_results.hasNextResult() == false) waitCondition.wait(&m_mutex); - return !(state.loadRelaxed() & QFutureInterfaceBase::Canceled) && m_results.hasNextResult(); + return !(state.loadRelaxed() & QFutureInterfaceBase::Canceled) + && data.m_results.hasNextResult(); } bool QFutureInterfaceBasePrivate::internal_updateProgress(int progress, @@ -714,14 +741,16 @@ void QFutureInterfaceBasePrivate::connectOutputInterface(QFutureCallOutInterface m_progressText)); } - QtPrivate::ResultIteratorBase it = m_results.begin(); - while (it != m_results.end()) { - const int begin = it.resultIndex(); - const int end = begin + it.batchSize(); - interface->postCallOutEvent(QFutureCallOutEvent(QFutureCallOutEvent::ResultsReady, - begin, - end)); - it.batchedAdvance(); + if (!hasException) { + QtPrivate::ResultIteratorBase it = data.m_results.begin(); + while (it != data.m_results.end()) { + const int begin = it.resultIndex(); + const int end = begin + it.batchSize(); + interface->postCallOutEvent(QFutureCallOutEvent(QFutureCallOutEvent::ResultsReady, + begin, + end)); + it.batchedAdvance(); + } } if (currentState & QFutureInterfaceBase::Suspended) diff --git a/src/corelib/thread/qfutureinterface.h b/src/corelib/thread/qfutureinterface.h index 3a6fe86d76..34c060aa06 100644 --- a/src/corelib/thread/qfutureinterface.h +++ b/src/corelib/thread/qfutureinterface.h @@ -167,6 +167,7 @@ public: void suspendIfRequested(); QMutex &mutex() const; + bool hasException() const; QtPrivate::ExceptionStore &exceptionStore(); QtPrivate::ResultStoreBase &resultStoreBase(); const QtPrivate::ResultStoreBase &resultStoreBase() const; @@ -247,7 +248,7 @@ public: ~QFutureInterface() { - if (!derefT()) + if (!derefT() && !hasException()) resultStoreBase().template clear(); } @@ -277,6 +278,25 @@ public: // TODO: Enable and make it return a QList, when QList is fixed to support move-only types std::vector takeResults(); #endif + +#ifndef QT_NO_EXCEPTIONS + void reportException(const std::exception_ptr &e) + { + if (hasException()) + return; + + resultStoreBase().template clear(); + QFutureInterfaceBase::reportException(e); + } + void reportException(const QException &e) + { + if (hasException()) + return; + + resultStoreBase().template clear(); + QFutureInterfaceBase::reportException(e); + } +#endif }; template @@ -286,6 +306,7 @@ inline bool QFutureInterface::reportResult(const T *result, int index) if (this->queryState(Canceled) || this->queryState(Finished)) return false; + Q_ASSERT(!hasException()); QtPrivate::ResultStoreBase &store = resultStoreBase(); const int resultCountBefore = store.count(); @@ -307,6 +328,7 @@ bool QFutureInterface::reportAndMoveResult(T &&result, int index) if (queryState(Canceled) || queryState(Finished)) return false; + Q_ASSERT(!hasException()); QtPrivate::ResultStoreBase &store = resultStoreBase(); const int oldResultCount = store.count(); @@ -336,6 +358,7 @@ inline bool QFutureInterface::reportResults(const QList &_results, int beg if (this->queryState(Canceled) || this->queryState(Finished)) return false; + Q_ASSERT(!hasException()); auto &store = resultStoreBase(); const int resultCountBefore = store.count(); @@ -363,6 +386,8 @@ inline bool QFutureInterface::reportFinished(const T *result) template inline const T &QFutureInterface::resultReference(int index) const { + Q_ASSERT(!hasException()); + QMutexLocker locker{&mutex()}; return resultStoreBase().resultAt(index).template value(); } @@ -370,6 +395,8 @@ inline const T &QFutureInterface::resultReference(int index) const template inline const T *QFutureInterface::resultPointer(int index) const { + Q_ASSERT(!hasException()); + QMutexLocker locker{&mutex()}; return resultStoreBase().resultAt(index).template pointer(); } @@ -405,6 +432,8 @@ T QFutureInterface::takeResult() // not to mess with other unready results. waitForResult(-1); + Q_ASSERT(!hasException()); + const QMutexLocker locker{&mutex()}; QtPrivate::ResultIteratorBase position = resultStoreBase().resultAt(0); T ret(std::move_if_noexcept(position.value())); @@ -421,6 +450,9 @@ std::vector QFutureInterface::takeResults() Q_ASSERT(isValid()); waitForResult(-1); + + Q_ASSERT(!hasException()); + std::vector res; res.reserve(resultCount()); diff --git a/src/corelib/thread/qfutureinterface_p.h b/src/corelib/thread/qfutureinterface_p.h index 44c3a48d2a..a717a1143f 100644 --- a/src/corelib/thread/qfutureinterface_p.h +++ b/src/corelib/thread/qfutureinterface_p.h @@ -134,6 +134,7 @@ class QFutureInterfaceBasePrivate { public: QFutureInterfaceBasePrivate(QFutureInterfaceBase::State initialState); + ~QFutureInterfaceBasePrivate(); // When the last QFuture reference is removed, we need to make // sure that data stored in the ResultStore is cleaned out. @@ -167,9 +168,22 @@ public: QElapsedTimer progressTime; QWaitCondition waitCondition; QWaitCondition pausedWaitCondition; - // ### TODO: put m_results and m_exceptionStore into a union (see QTBUG-92045) - QtPrivate::ResultStoreBase m_results; - QtPrivate::ExceptionStore m_exceptionStore; + + union Data { + QtPrivate::ResultStoreBase m_results; + QtPrivate::ExceptionStore m_exceptionStore; + + void setException(const std::exception_ptr &e) + { + m_results.~ResultStoreBase(); + new (&m_exceptionStore) QtPrivate::ExceptionStore(); + m_exceptionStore.setException(e); + } + + ~Data() { } + }; + Data data = { QtPrivate::ResultStoreBase() }; + QString m_progressText; QRunnable *runnable = nullptr; QThreadPool *m_pool = nullptr; @@ -185,6 +199,7 @@ public: bool manualProgress = false; // only accessed from executing thread bool launchAsync = false; bool isValid = false; + bool hasException = false; inline QThreadPool *pool() const { return m_pool ? m_pool : QThreadPool::globalInstance(); } diff --git a/src/corelib/thread/qresultstore.h b/src/corelib/thread/qresultstore.h index 0a1382fd79..fb9151fcc6 100644 --- a/src/corelib/thread/qresultstore.h +++ b/src/corelib/thread/qresultstore.h @@ -136,6 +136,7 @@ public: ResultIteratorBase resultAt(int index) const; bool contains(int index) const; int count() const; + // ### Qt 7: 'virtual' isn't required, can be removed virtual ~ResultStoreBase(); protected: -- cgit v1.2.3