From 13074a967f18ed348ab744f7ff831965607a6421 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tor=20Arne=20Vestb=C3=B8?= Date: Sun, 10 Dec 2023 15:25:44 +0100 Subject: [PATCH] Debounce QDeferredDeleteEvents in QObject::deleteLater() We used to look through the event queue in QCoreApplication::postEvent, and if we found an existing DeferredDelete event for the receiver we would compress the two events into one. This was changed in 99b89d30fa5484c5d1f3cbda828648c28af4fb7d, as the logic was causing O(n^2) for deleteLater, by using one of the bits in QObjectData to track whether the object had already been deleted. But it kept the logic for tracking this in QCoreApplication::postEvent, and QCoreApplication::compressEvent would still do the work of deleting the additional QDeferredDeleteEvents. To avoid the unnecessary heap allocation of the QDeferredDeleteEvents we can move the debouncing/compression to QObject::deleteLater(). We use the same mutex as in QCoreApplication::postEvent to guard concurrent access to deleteLaterCalled. A note has been added about the (preexisting) issue that the mutex is not sufficient to prevent data races, as the deleteLaterCalled flag is part of a bit-field, and we're not guarding any of our other accesses to other bits. As QDeferredDeleteEvents is private API, we can rely on no-one else posting it than QObject::deleteLater(), which should be the case now that tst_QApplication::sendPostedEvents() was fixed. The documentation has been clarified as well. It's safe to call deleteLater() more than once, but that's not _because_ other pending events for the object are cleared. The latter behavior is normal ~QObject() behavior. The documentation was probably written at a point we didn't do any event compression at all for QDeferredDeleteEvents. Task-number: QTBUG-120124 Task-number: QTBUG-119918 Change-Id: I2a733095b7cb066ba494b1335aa40200c749cb0c Reviewed-by: Thiago Macieira --- src/corelib/kernel/qcoreapplication.cpp | 14 -------------- src/corelib/kernel/qobject.cpp | 18 ++++++++++++++++++ .../kernel/qapplication/tst_qapplication.cpp | 3 +-- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/corelib/kernel/qcoreapplication.cpp b/src/corelib/kernel/qcoreapplication.cpp index 2752697dbae..b53f8eafba1 100644 --- a/src/corelib/kernel/qcoreapplication.cpp +++ b/src/corelib/kernel/qcoreapplication.cpp @@ -1677,9 +1677,6 @@ void QCoreApplication::postEvent(QObject *receiver, QEvent *event, int priority) return; } - if (event->type() == QEvent::DeferredDelete) - receiver->d_ptr->deleteLaterCalled = true; - if (event->type() == QEvent::DeferredDelete && data == QThreadData::current()) { // remember the current running eventloop for DeferredDelete // events posted in the receiver's thread. @@ -1750,17 +1747,6 @@ bool QCoreApplication::compressEvent(QEvent *event, QObject *receiver, QPostEven return false; } - if (event->type() == QEvent::DeferredDelete) { - if (receiver->d_ptr->deleteLaterCalled) { - // there was a previous DeferredDelete event, so we can drop the new one - delete event; - return true; - } - // deleteLaterCalled is set to true in postedEvents when queueing the very first - // deferred deletion event. - return false; - } - if (event->type() == QEvent::Quit && receiverPostedEvents > 0) { for (const QPostEvent &cur : std::as_const(*postedEvents)) { if (cur.receiver != receiver diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index d8183a9d1c7..0cee785f3b4 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -2425,6 +2425,24 @@ void QObject::deleteLater() if (qApp == this) qWarning("You are deferring the delete of QCoreApplication, this may not work as expected."); #endif + + { + // De-bounce QDeferredDeleteEvents. Use the post event list mutex + // to guard access to deleteLaterCalled, so we don't need a separate + // mutex in QObjectData. + auto locker = QCoreApplicationPrivate::lockThreadPostEventList(this); + + // FIXME: The deleteLaterCalled flag is part of a bit field, + // so we likely have data races here, even with the mutex above, + // as long as we're not guarding every access to the bit field. + + Q_D(QObject); + if (d->deleteLaterCalled) + return; + + d->deleteLaterCalled = true; + } + QCoreApplication::postEvent(this, new QDeferredDeleteEvent()); } diff --git a/tests/auto/widgets/kernel/qapplication/tst_qapplication.cpp b/tests/auto/widgets/kernel/qapplication/tst_qapplication.cpp index 914d1968969..e26152439a5 100644 --- a/tests/auto/widgets/kernel/qapplication/tst_qapplication.cpp +++ b/tests/auto/widgets/kernel/qapplication/tst_qapplication.cpp @@ -19,7 +19,6 @@ #if QT_CONFIG(process) # include #endif -#include #include #include @@ -1166,7 +1165,7 @@ void SendPostedEventsTester::doTest() QPointer p = this; QApplication::postEvent(this, new QEvent(QEvent::User)); // DeferredDelete should not be delivered until returning from this function - QApplication::postEvent(this, new QDeferredDeleteEvent()); + deleteLater(); QEventLoop eventLoop; QMetaObject::invokeMethod(&eventLoop, "quit", Qt::QueuedConnection);