From b7d073e9905bf9812ba96cecdcf6871a95517d30 Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Fri, 26 Jul 2019 16:17:53 +0200 Subject: Refactor memory allocation for arguments of QMetaCallEvents There are two cases: In a BlockingQueuedConnection, QMetaCallEvent doesn't allocate memory and instead passes already existing pointers through. A QSemaphore is used to serialize data access between threads. So the constructor taking a QSemaphore can be simplified to only accept an existing arg array. In a QueuedConnection, QMetaCallEvent needs to make deep copies of the arguments, and memory needs to be allocated based on the number of arguments. The previous code put the burden of memory allocation on the code generating the event, while the memory was free'd by ~QMetaCallEvent. Instead, make it QMetaCallEvent's responsibility to allocate and free the memory as needed, and adjust the code generating QMetaCallEvents. We can allocate the memory for types and pointers to arguments in a single block, starting with the space for the array of void*, followed by the space for the array of integers to avoid byte alignment issues. By pre-allocating the space that's needed by three arguments, we can avoid all mallocs for the majority of QMetaCallEvents. Until this change has propagated through qt5.git, we need to keep the old API that is still used by QtDeclarative around. Once QtDeclarative has migrated to the new API, it can be removed. Change-Id: Id7359ffc14897237ea9672dabae9ef199a821907 Reviewed-by: Simon Hausmann --- src/corelib/kernel/qobject.cpp | 159 +++++++++++++++++++++++++++++++---------- 1 file changed, 121 insertions(+), 38 deletions(-) (limited to 'src/corelib/kernel/qobject.cpp') diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index d364ae1087..048a47881f 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -511,25 +511,107 @@ QAbstractMetaCallEvent::~QAbstractMetaCallEvent() /*! \internal */ -QMetaCallEvent::QMetaCallEvent(ushort method_offset, ushort method_relative, QObjectPrivate::StaticMetaCallFunction callFunction, +inline void QMetaCallEvent::allocArgs() +{ + if (!d.nargs_) + return; + + constexpr size_t each = sizeof(void*) + sizeof(int); + void *const memory = d.nargs_ * each > sizeof(prealloc_) ? + calloc(d.nargs_, each) : prealloc_; + + Q_CHECK_PTR(memory); + d.args_ = static_cast(memory); +} + +/*! + \internal + + Only used by QtDeclarative - to be removed when migrated. + */ +QMetaCallEvent::QMetaCallEvent(ushort method_offset, ushort method_relative, + QObjectPrivate::StaticMetaCallFunction callFunction, const QObject *sender, int signalId, - int nargs, int *types, void **args, QSemaphore *semaphore) + int nargs, int *types_, void **args_) + : QAbstractMetaCallEvent(sender, signalId), + d({nullptr, nullptr, callFunction, nargs, method_offset, method_relative}), + prealloc_() +{ + allocArgs(); + for (int arg = 0; arg < nargs; ++arg) { + types()[arg] = types_[arg]; + args()[arg] = args_[arg]; + } + free(types_); + free(args_); +} + +/*! + \internal + + Used for blocking queued connections, just passes \a args through without + allocating any memory. + */ +QMetaCallEvent::QMetaCallEvent(ushort method_offset, ushort method_relative, + QObjectPrivate::StaticMetaCallFunction callFunction, + const QObject *sender, int signalId, + void **args, QSemaphore *semaphore) : QAbstractMetaCallEvent(sender, signalId, semaphore), - slotObj_(nullptr), nargs_(nargs), types_(types), args_(args), - callFunction_(callFunction), method_offset_(method_offset), method_relative_(method_relative) -{ } + d({nullptr, args, callFunction, 0, method_offset, method_relative}), + prealloc_() +{ +} /*! \internal + + Used for blocking queued connections, just passes \a args through without + allocating any memory. */ -QMetaCallEvent::QMetaCallEvent(QtPrivate::QSlotObjectBase *slotO, const QObject *sender, int signalId, - int nargs, int *types, void **args, QSemaphore *semaphore) +QMetaCallEvent::QMetaCallEvent(QtPrivate::QSlotObjectBase *slotO, + const QObject *sender, int signalId, + void **args, QSemaphore *semaphore) : QAbstractMetaCallEvent(sender, signalId, semaphore), - slotObj_(slotO), nargs_(nargs), types_(types), args_(args), - callFunction_(nullptr), method_offset_(0), method_relative_(ushort(-1)) + d({slotO, args, nullptr, 0, 0, ushort(-1)}), + prealloc_() { - if (slotObj_) - slotObj_->ref(); + if (d.slotObj_) + d.slotObj_->ref(); +} + +/*! + \internal + + Allocates memory for \a nargs; code creating an event needs to initialize + the void* and int arrays by accessing \a args() and \a types(), respectively. + */ +QMetaCallEvent::QMetaCallEvent(ushort method_offset, ushort method_relative, + QObjectPrivate::StaticMetaCallFunction callFunction, + const QObject *sender, int signalId, + int nargs) + : QAbstractMetaCallEvent(sender, signalId), + d({nullptr, nullptr, callFunction, nargs, method_offset, method_relative}), + prealloc_() +{ + allocArgs(); +} + +/*! + \internal + + Allocates memory for \a nargs; code creating an event needs to initialize + the void* and int arrays by accessing \a args() and \a types(), respectively. + */ +QMetaCallEvent::QMetaCallEvent(QtPrivate::QSlotObjectBase *slotO, + const QObject *sender, int signalId, + int nargs) + : QAbstractMetaCallEvent(sender, signalId), + d({slotO, nullptr, nullptr, nargs, 0, ushort(-1)}), + prealloc_() +{ + if (d.slotObj_) + d.slotObj_->ref(); + allocArgs(); } /*! @@ -537,16 +619,17 @@ QMetaCallEvent::QMetaCallEvent(QtPrivate::QSlotObjectBase *slotO, const QObject */ QMetaCallEvent::~QMetaCallEvent() { - if (types_) { - for (int i = 0; i < nargs_; ++i) { - if (types_[i] && args_[i]) - QMetaType::destroy(types_[i], args_[i]); + if (d.nargs_) { + int *typeIDs = types(); + for (int i = 0; i < d.nargs_; ++i) { + if (typeIDs[i] && d.args_[i]) + QMetaType::destroy(typeIDs[i], d.args_[i]); } - free(types_); - free(args_); + if (reinterpret_cast(d.args_) != reinterpret_cast(prealloc_)) + free(d.args_); } - if (slotObj_) - slotObj_->destroyIfLastRef(); + if (d.slotObj_) + d.slotObj_->destroyIfLastRef(); } /*! @@ -554,12 +637,13 @@ QMetaCallEvent::~QMetaCallEvent() */ void QMetaCallEvent::placeMetaCall(QObject *object) { - if (slotObj_) { - slotObj_->call(object, args_); - } else if (callFunction_ && method_offset_ <= object->metaObject()->methodOffset()) { - callFunction_(object, QMetaObject::InvokeMetaMethod, method_relative_, args_); + if (d.slotObj_) { + d.slotObj_->call(object, d.args_); + } else if (d.callFunction_ && d.method_offset_ <= object->metaObject()->methodOffset()) { + d.callFunction_(object, QMetaObject::InvokeMetaMethod, d.method_relative_, d.args_); } else { - QMetaObject::metacall(object, QMetaObject::InvokeMetaMethod, method_offset_ + method_relative_, args_); + QMetaObject::metacall(object, QMetaObject::InvokeMetaMethod, + d.method_offset_ + d.method_relative_, d.args_); } } @@ -3643,12 +3727,16 @@ static void queued_activate(QObject *sender, int signal, QObjectPrivate::Connect int nargs = 1; // include return type while (argumentTypes[nargs-1]) ++nargs; - int *types = (int *) malloc(nargs*sizeof(int)); - Q_CHECK_PTR(types); - void **args = (void **) malloc(nargs*sizeof(void *)); - Q_CHECK_PTR(args); + + QMetaCallEvent *ev = c->isSlotObject ? + new QMetaCallEvent(c->slotObj, sender, signal, nargs) : + new QMetaCallEvent(c->method_offset, c->method_relative, c->callFunction, sender, signal, nargs); + + void **args = ev->args(); + int *types = ev->types(); + types[0] = 0; // return type - args[0] = 0; // return value + args[0] = nullptr; // return value if (nargs > 1) { for (int n = 1; n < nargs; ++n) @@ -3662,16 +3750,10 @@ static void queued_activate(QObject *sender, int signal, QObjectPrivate::Connect if (!c->receiver.loadRelaxed()) { // the connection has been disconnected before we got the lock locker.unlock(); - for (int n = 1; n < nargs; ++n) - QMetaType::destroy(types[n], args[n]); - free(types); - free(args); + delete ev; return; } - QMetaCallEvent *ev = c->isSlotObject ? - new QMetaCallEvent(c->slotObj, sender, signal, nargs, types, args) : - new QMetaCallEvent(c->method_offset, c->method_relative, c->callFunction, sender, signal, nargs, types, args); QCoreApplication::postEvent(c->receiver.loadRelaxed(), ev); } @@ -3772,8 +3854,9 @@ void doActivate(QObject *sender, int signal_index, void **argv) if (!c->receiver.loadAcquire()) continue; QMetaCallEvent *ev = c->isSlotObject ? - new QMetaCallEvent(c->slotObj, sender, signal_index, 0, 0, argv, &semaphore) : - new QMetaCallEvent(c->method_offset, c->method_relative, c->callFunction, sender, signal_index, 0, 0, argv, &semaphore); + new QMetaCallEvent(c->slotObj, sender, signal_index, argv, &semaphore) : + new QMetaCallEvent(c->method_offset, c->method_relative, c->callFunction, + sender, signal_index, argv, &semaphore); QCoreApplication::postEvent(receiver, ev); } semaphore.acquire(); -- cgit v1.2.3 From b68dbdd0094a546c49c5771173ff40577b3c02b2 Mon Sep 17 00:00:00 2001 From: Lars Knoll Date: Fri, 18 Jan 2019 10:59:04 +0100 Subject: Fix return value The method actually returns a boolean. Change-Id: I5887ad23e19be9a9c87c7858d81891378fd23cc9 Reviewed-by: Thiago Macieira Reviewed-by: Olivier Goffart (Woboq GmbH) --- src/corelib/kernel/qobject.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/corelib/kernel/qobject.cpp') diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index 048a47881f..82fe546715 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -5179,7 +5179,7 @@ bool QObject::disconnectImpl(const QObject *sender, void **signal, const QObject } if (!senderMetaObject) { qWarning("QObject::disconnect: signal not found in %s", sender->metaObject()->className()); - return QMetaObject::Connection(0); + return false; } signal_index += QMetaObjectPrivate::signalOffset(senderMetaObject); } -- cgit v1.2.3 From e9eddfd85628f0ec672895652c67443caa160b7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Fri, 16 Aug 2019 15:49:42 +0200 Subject: Fix rare double-free in QObject machinery As exposed by tst_QObjectRace::destroyRace we would sometimes end up with a double-free when destroying a QSlotObject in multi-threaded scenarios. One free would be done in ~QObject as the receiver was being destroyed while the other free was done when deleting a QMetaCallEvent object after we realized it was not needed because the receiver was destroyed. Since we can be in a separate thread from the receiver we should lock before referencing the connection object. Amends b7d073e9905bf9812ba96cecdcf6871a95517d30. Change-Id: Icb53862dc880ae9a4e5581a1a9ee693573f7d9c7 Reviewed-by: Volker Hilsheimer --- src/corelib/kernel/qobject.cpp | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) (limited to 'src/corelib/kernel/qobject.cpp') diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index 82fe546715..4a26e9cdc0 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -3728,6 +3728,15 @@ static void queued_activate(QObject *sender, int signal, QObjectPrivate::Connect while (argumentTypes[nargs-1]) ++nargs; + QBasicMutexLocker locker(signalSlotLock(c->receiver.loadRelaxed())); + if (!c->receiver.loadRelaxed()) { + // the connection has been disconnected before we got the lock + return; + } + if (c->isSlotObject) + c->slotObj->ref(); + locker.unlock(); + QMetaCallEvent *ev = c->isSlotObject ? new QMetaCallEvent(c->slotObj, sender, signal, nargs) : new QMetaCallEvent(c->method_offset, c->method_relative, c->callFunction, sender, signal, nargs); @@ -3746,9 +3755,11 @@ static void queued_activate(QObject *sender, int signal, QObjectPrivate::Connect args[n] = QMetaType::create(types[n], argv[n]); } - QBasicMutexLocker locker(signalSlotLock(c->receiver.loadRelaxed())); + locker.relock(); + if (c->isSlotObject) + c->slotObj->destroyIfLastRef(); if (!c->receiver.loadRelaxed()) { - // the connection has been disconnected before we got the lock + // the connection has been disconnected while we were unlocked locker.unlock(); delete ev; return; -- cgit v1.2.3 From d97009a9f147af7f9785afb53df49e874aadd969 Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Sun, 11 Aug 2019 08:59:07 +0200 Subject: Remove obsolete API after qtdeclarative migrated This is a follow-up to commit b7d073e9905bf9812ba96cecdcf6871a95517d30, which refactored memory allocation of QMetaCallEvent. Change-Id: I363256c80ee941b545e6f9c659c65556fff5c907 Reviewed-by: Simon Hausmann --- src/corelib/kernel/qobject.cpp | 22 ---------------------- 1 file changed, 22 deletions(-) (limited to 'src/corelib/kernel/qobject.cpp') diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index 4a26e9cdc0..9251a3c05f 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -524,28 +524,6 @@ inline void QMetaCallEvent::allocArgs() d.args_ = static_cast(memory); } -/*! - \internal - - Only used by QtDeclarative - to be removed when migrated. - */ -QMetaCallEvent::QMetaCallEvent(ushort method_offset, ushort method_relative, - QObjectPrivate::StaticMetaCallFunction callFunction, - const QObject *sender, int signalId, - int nargs, int *types_, void **args_) - : QAbstractMetaCallEvent(sender, signalId), - d({nullptr, nullptr, callFunction, nargs, method_offset, method_relative}), - prealloc_() -{ - allocArgs(); - for (int arg = 0; arg < nargs; ++arg) { - types()[arg] = types_[arg]; - args()[arg] = args_[arg]; - } - free(types_); - free(args_); -} - /*! \internal -- cgit v1.2.3