From fda8eb17417e92f8f80587937463be77899c7bed Mon Sep 17 00:00:00 2001
From: Marc Mutz <marc.mutz@qt.io>
Date: Wed, 25 May 2022 21:32:53 +0200
Subject: [PATCH] Optimize
 QThreadPrivate::addObjectWithPendingBindingStatusChange()

... and bindingStatus()

QBindingStatus* is the final state of the value chain in
m_statusOrPendingObjects, so we can use the Double-Checked Locking
Pattern to avoid locking the mutex when we already have a status - it
won't go away again, unlike the vector in the List state.

To enable the change, make the data member an atomic<>. All loads and
stores can continue to use memory_order::relaxed, except the loads of
a potential status, which have to acquire, and the store of the
status, which has to release. This creates the necessary
synchronizes-with relation. So even though we synchronize out of
middle of the mutex critical section in QThread::exec() this way,
there's no data race between QThread::exec() and a potential
bindingStatus() call.

Change-Id: I0e0b7bd305649fa5f56a0f8723fb75f2577b90dd
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
---
 src/corelib/thread/qthread.cpp |  8 +++++---
 src/corelib/thread/qthread_p.h | 15 +++++++++------
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/src/corelib/thread/qthread.cpp b/src/corelib/thread/qthread.cpp
index d067bd06d3..208420b8fe 100644
--- a/src/corelib/thread/qthread.cpp
+++ b/src/corelib/thread/qthread.cpp
@@ -519,7 +519,8 @@ void QtPrivate::BindingStatusOrList::setStatusAndClearList(QBindingStatus *statu
             QObjectPrivate::get(obj)->reinitBindingStorageAfterThreadMove();
         delete pendingObjects;
     }
-    data = encodeBindingStatus(status);
+    // synchronizes-with the load-acquire in bindingStatus():
+    data.store(encodeBindingStatus(status), std::memory_order_release);
 }
 
 /*!
@@ -568,14 +569,13 @@ int QThread::exec()
  */
 QBindingStatus *QtPrivate::BindingStatusOrList::addObjectUnlessAlreadyStatus(QObject *object)
 {
-
     if (auto status = bindingStatus())
         return status;
     List *objectList = list();
     if (!objectList) {
         objectList = new List();
         objectList->reserve(8);
-        data = encodeList(objectList);
+        data.store(encodeList(objectList), std::memory_order_relaxed);
     }
     objectList->push_back(object);
     return nullptr;
@@ -583,6 +583,8 @@ QBindingStatus *QtPrivate::BindingStatusOrList::addObjectUnlessAlreadyStatus(QOb
 
 QBindingStatus *QThreadPrivate::addObjectWithPendingBindingStatusChange(QObject *obj)
 {
+    if (auto status = m_statusOrPendingObjects.bindingStatus())
+        return status;
     QMutexLocker lock(&mutex);
     return m_statusOrPendingObjects.addObjectUnlessAlreadyStatus(obj);
 }
diff --git a/src/corelib/thread/qthread_p.h b/src/corelib/thread/qthread_p.h
index 79476a940f..7909f00e37 100644
--- a/src/corelib/thread/qthread_p.h
+++ b/src/corelib/thread/qthread_p.h
@@ -122,6 +122,7 @@ public:
         data(encodeBindingStatus(status)) {}
     explicit BindingStatusOrList(List *list) noexcept : data(encodeList(list)) {}
 
+    // requires external synchronization:
     QBindingStatus *addObjectUnlessAlreadyStatus(QObject *object);
     void setStatusAndClearList(QBindingStatus *status) noexcept;
 
@@ -133,17 +134,21 @@ public:
     static bool isList(quintptr data) noexcept { return data & 1; }
     static bool isNull(quintptr data) noexcept { return data == 0; }
 
+    // thread-safe:
     QBindingStatus *bindingStatus() const noexcept
     {
-        if (isBindingStatus(data))
-            return reinterpret_cast<QBindingStatus *>(data);
+        // synchronizes-with the store-release in setStatusAndClearList():
+        const auto d = data.load(std::memory_order_acquire);
+        if (isBindingStatus(d))
+            return reinterpret_cast<QBindingStatus *>(d);
         else
             return nullptr;
     }
 
+    // requires external synchronization:
     List *list() const noexcept
     {
-        return decodeList(data);
+        return decodeList(data.load(std::memory_order_relaxed));
     }
 
 private:
@@ -165,7 +170,7 @@ private:
         return quintptr(list) | 1;
     }
 
-    quintptr data;
+    std::atomic<quintptr> data;
 };
 
 } // namespace QtPrivate
@@ -241,8 +246,6 @@ public:
 
     QBindingStatus *bindingStatus()
     {
-
-        QMutexLocker lock(&mutex);
         return m_statusOrPendingObjects.bindingStatus();
     }