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 <andrei.golubev@qt.io>
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

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

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

src/corelib/thread/qfuture_impl.h

@@ -427,7 +427,7 @@ bool Continuation<Function, ResultType, ParentResultType>::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<Function, ResultType>::run()
 
     promise.reportStarted();
 
-    if (parentFuture.d.exceptionStore().hasException()) {
+    if (parentFuture.d.hasException()) {
         using ArgType = typename QtPrivate::ArgResolver<Function>::First;
         if constexpr (std::is_void_v<ArgType>) {
             handleAllExceptions();
@@ -681,7 +681,8 @@ template<class ArgType>
 void FailureHandler<Function, ResultType>::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<class Function, class ResultType>
 void FailureHandler<Function, ResultType>::handleAllExceptions()
 {
     try {
-        parentFuture.d.exceptionStore().throwPossibleException();
+        Q_ASSERT(parentFuture.d.hasException());
+        parentFuture.d.exceptionStore().rethrowException();
     } catch (...) {
         try {
             QtPrivate::fulfillPromise(promise, std::forward<Function>(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 {

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)

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<T>();
     }
 
@@ -277,6 +278,25 @@ public:
     // TODO: Enable and make it return a QList, when QList is fixed to support move-only types
     std::vector<T> takeResults();
 #endif
+
+#ifndef QT_NO_EXCEPTIONS
+    void reportException(const std::exception_ptr &e)
+    {
+        if (hasException())
+            return;
+
+        resultStoreBase().template clear<T>();
+        QFutureInterfaceBase::reportException(e);
+    }
+    void reportException(const QException &e)
+    {
+        if (hasException())
+            return;
+
+        resultStoreBase().template clear<T>();
+        QFutureInterfaceBase::reportException(e);
+    }
+#endif
 };
 
 template <typename T>
@@ -286,6 +306,7 @@ inline bool QFutureInterface<T>::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<T>::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<T>::reportResults(const QList<T> &_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<T>::reportFinished(const T *result)
 template <typename T>
 inline const T &QFutureInterface<T>::resultReference(int index) const
 {
+    Q_ASSERT(!hasException());
+
     QMutexLocker<QMutex> locker{&mutex()};
     return resultStoreBase().resultAt(index).template value<T>();
 }
@@ -370,6 +395,8 @@ inline const T &QFutureInterface<T>::resultReference(int index) const
 template <typename T>
 inline const T *QFutureInterface<T>::resultPointer(int index) const
 {
+    Q_ASSERT(!hasException());
+
     QMutexLocker<QMutex> locker{&mutex()};
     return resultStoreBase().resultAt(index).template pointer<T>();
 }
@@ -405,6 +432,8 @@ T QFutureInterface<T>::takeResult()
     // not to mess with other unready results.
     waitForResult(-1);
 
+    Q_ASSERT(!hasException());
+
     const QMutexLocker<QMutex> locker{&mutex()};
     QtPrivate::ResultIteratorBase position = resultStoreBase().resultAt(0);
     T ret(std::move_if_noexcept(position.value<T>()));
@@ -421,6 +450,9 @@ std::vector<T> QFutureInterface<T>::takeResults()
     Q_ASSERT(isValid());
 
     waitForResult(-1);
+
+    Q_ASSERT(!hasException());
+
     std::vector<T> res;
     res.reserve(resultCount());
 

src/corelib/thread/qfutureinterface_p.h

@@ -134,6 +134,7 @@ class QFutureInterfaceBasePrivate
 {
 public:
     QFutureInterfaceBasePrivate(QFutureInterfaceBase::State initialState);
+    ~QFutureInterfaceBasePrivate();
 
     // When the last QFuture<T> 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(); }

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: