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 <simon.hausmann@qt.io>

src/corelib/kernel/qmetaobject.cpp

@@ -1542,21 +1542,14 @@ bool QMetaObject::invokeMethodImpl(QObject *object, QtPrivate::QSlotObjectBase *
             return false;
         }
 
-        // args and typesCopy will be deallocated by ~QMetaCallEvent() using free()
-        void **args = static_cast<void **>(calloc(1, sizeof(void *)));
-        Q_CHECK_PTR(args);
-
-        int *types = static_cast<int *>(calloc(1, sizeof(int)));
-        Q_CHECK_PTR(types);
-
-        QCoreApplication::postEvent(object, new QMetaCallEvent(slot, 0, -1, 1, types, args));
+        QCoreApplication::postEvent(object, new QMetaCallEvent(slot, 0, -1, 1));
     } else if (type == Qt::BlockingQueuedConnection) {
 #if QT_CONFIG(thread)
         if (currentThread == objectThread)
             qWarning("QMetaObject::invokeMethod: Dead lock detected");
 
         QSemaphore semaphore;
-        QCoreApplication::postEvent(object, new QMetaCallEvent(slot, 0, -1, 0, 0, argv, &semaphore));
+        QCoreApplication::postEvent(object, new QMetaCallEvent(slot, 0, -1, argv, &semaphore));
         semaphore.acquire();
 #endif // QT_CONFIG(thread)
     } else {
@@ -2310,42 +2303,31 @@ bool QMetaMethod::invoke(QObject *object,
             return false;
         }
 
-        int nargs = 1; // include return type
-        void **args = (void **) malloc(paramCount * sizeof(void *));
-        Q_CHECK_PTR(args);
-        int *types = (int *) malloc(paramCount * sizeof(int));
-        Q_CHECK_PTR(types);
-        types[0] = 0; // return type
-        args[0] = 0;
+        QScopedPointer<QMetaCallEvent> event(new QMetaCallEvent(idx_offset, idx_relative, callFunction, 0, -1, paramCount));
+        int *types = event->types();
+        void **args = event->args();
 
+        int argIndex = 0;
         for (int i = 1; i < paramCount; ++i) {
             types[i] = QMetaType::type(typeNames[i]);
             if (types[i] == QMetaType::UnknownType && param[i]) {
                 // Try to register the type and try again before reporting an error.
-                int index = nargs - 1;
-                void *argv[] = { &types[i], &index };
+                void *argv[] = { &types[i], &argIndex };
                 QMetaObject::metacall(object, QMetaObject::RegisterMethodArgumentMetaType,
                                       idx_relative + idx_offset, argv);
                 if (types[i] == -1) {
                     qWarning("QMetaMethod::invoke: Unable to handle unregistered datatype '%s'",
                             typeNames[i]);
-                    for (int x = 1; x < i; ++x) {
-                        if (types[x] && args[x])
-                            QMetaType::destroy(types[x], args[x]);
-                    }
-                    free(types);
-                    free(args);
                     return false;
                 }
             }
             if (types[i] != QMetaType::UnknownType) {
                 args[i] = QMetaType::create(types[i], param[i]);
-                ++nargs;
+                ++argIndex;
             }
         }
 
-        QCoreApplication::postEvent(object, new QMetaCallEvent(idx_offset, idx_relative, callFunction,
-                                                        0, -1, nargs, types, args));
+        QCoreApplication::postEvent(object, event.take());
     } else { // blocking queued connection
 #if QT_CONFIG(thread)
         QThread *currentThread = QThread::currentThread();
@@ -2358,7 +2340,7 @@ bool QMetaMethod::invoke(QObject *object,
 
         QSemaphore semaphore;
         QCoreApplication::postEvent(object, new QMetaCallEvent(idx_offset, idx_relative, callFunction,
-                                                        0, -1, 0, 0, param, &semaphore));
+                                                        0, -1, param, &semaphore));
         semaphore.acquire();
 #endif // QT_CONFIG(thread)
     }

src/corelib/kernel/qobject.cpp

@@ -511,25 +511,107 @@ QAbstractMetaCallEvent::~QAbstractMetaCallEvent()
 /*!
     \internal
  */
-QMetaCallEvent::QMetaCallEvent(ushort method_offset, ushort method_relative, QObjectPrivate::StaticMetaCallFunction callFunction,
-                               const QObject *sender, int signalId,
-                               int nargs, int *types, 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)
-{ }
+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<void **>(memory);
+}
 
 /*!
     \internal
+
+    Only used by QtDeclarative - to be removed when migrated.
  */
-QMetaCallEvent::QMetaCallEvent(QtPrivate::QSlotObjectBase *slotO, const QObject *sender, int signalId,
-                               int nargs, int *types, 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))
+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_()
 {
-    if (slotObj_)
-        slotObj_->ref();
+    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),
+      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,
+                               void **args, QSemaphore *semaphore)
+    : QAbstractMetaCallEvent(sender, signalId, semaphore),
+      d({slotO, args, nullptr, 0, 0, ushort(-1)}),
+      prealloc_()
+{
+    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<void*>(d.args_) != reinterpret_cast<void*>(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();

src/corelib/kernel/qobject_p.h

@@ -509,29 +509,53 @@ private:
 class Q_CORE_EXPORT QMetaCallEvent : public QAbstractMetaCallEvent
 {
 public:
-    QMetaCallEvent(ushort method_offset, ushort method_relative, QObjectPrivate::StaticMetaCallFunction callFunction , const QObject *sender, int signalId,
-                   int nargs = 0, int *types = nullptr, void **args = nullptr, QSemaphore *semaphore = nullptr);
-    /*! \internal
-        \a signalId is in the signal index range (see QObjectPrivate::signalIndex()).
-    */
-    QMetaCallEvent(QtPrivate::QSlotObjectBase *slotObj, const QObject *sender, int signalId,
-                   int nargs = 0, int *types = nullptr, void **args = nullptr, QSemaphore *semaphore = nullptr);
+    // kept for compatibility until QtDeclarative has moved over
+    QMetaCallEvent(ushort method_offset, ushort method_relative,
+                   QObjectPrivate::StaticMetaCallFunction callFunction,
+                   const QObject *sender, int signalId,
+                   int nargs, int *types, void **args);
+
+    // blocking queued with semaphore - args always owned by caller
+    QMetaCallEvent(ushort method_offset, ushort method_relative,
+                   QObjectPrivate::StaticMetaCallFunction callFunction,
+                   const QObject *sender, int signalId,
+                   void **args, QSemaphore *semaphore);
+    QMetaCallEvent(QtPrivate::QSlotObjectBase *slotObj,
+                   const QObject *sender, int signalId,
+                   void **args, QSemaphore *semaphore);
+
+    // queued - args allocated by event, copied by caller
+    QMetaCallEvent(ushort method_offset, ushort method_relative,
+                   QObjectPrivate::StaticMetaCallFunction callFunction,
+                   const QObject *sender, int signalId,
+                   int nargs);
+    QMetaCallEvent(QtPrivate::QSlotObjectBase *slotObj,
+                   const QObject *sender, int signalId,
+                   int nargs);
 
     ~QMetaCallEvent() override;
 
-    inline int id() const { return method_offset_ + method_relative_; }
-    inline void **args() const { return args_; }
+    inline int id() const { return d.method_offset_ + d.method_relative_; }
+    inline const void * const* args() const { return d.args_; }
+    inline void ** args() { return d.args_; }
+    inline const int *types() const { return reinterpret_cast<int*>(d.args_ + d.nargs_); }
+    inline int *types() { return reinterpret_cast<int*>(d.args_ + d.nargs_); }
 
     virtual void placeMetaCall(QObject *object) override;
 
 private:
-    QtPrivate::QSlotObjectBase *slotObj_;
-    int nargs_;
-    int *types_;
-    void **args_;
-    QObjectPrivate::StaticMetaCallFunction callFunction_;
-    ushort method_offset_;
-    ushort method_relative_;
+    inline void allocArgs();
+
+    struct Data {
+        QtPrivate::QSlotObjectBase *slotObj_;
+        void **args_;
+        QObjectPrivate::StaticMetaCallFunction callFunction_;
+        int nargs_;
+        ushort method_offset_;
+        ushort method_relative_;
+    } d;
+    // preallocate enough space for three arguments
+    char prealloc_[3*(sizeof(void*) + sizeof(int))];
 };
 
 class QBoolBlocker

src/network/kernel/qhostinfo.cpp

@@ -140,17 +140,15 @@ void QHostInfoResult::postResultsReady(const QHostInfo &info)
     Q_CHECK_PTR(result);
 
     const int nargs = 2;
-    auto types = reinterpret_cast<int *>(malloc(nargs * sizeof(int)));
-    Q_CHECK_PTR(types);
+    auto metaCallEvent = new QMetaCallEvent(slotObj, nullptr, signal_index, nargs);
+    Q_CHECK_PTR(metaCallEvent);
+    void **args = metaCallEvent->args();
+    int *types = metaCallEvent->types();
     types[0] = QMetaType::type("void");
     types[1] = QMetaType::type("QHostInfo");
-    auto args = reinterpret_cast<void **>(malloc(nargs * sizeof(void *)));
-    Q_CHECK_PTR(args);
-    args[0] = 0;
+    args[0] = nullptr;
     args[1] = QMetaType::create(types[1], &info);
     Q_CHECK_PTR(args[1]);
-    auto metaCallEvent = new QMetaCallEvent(slotObj, nullptr, signal_index, nargs, types, args);
-    Q_CHECK_PTR(metaCallEvent);
     qApp->postEvent(result, metaCallEvent);
 }
 

tests/auto/corelib/kernel/qmetaobject/tst_qmetaobject.cpp

@@ -507,6 +507,7 @@ public slots:
     { QObject::moveToThread(t); }
 
     void slotWithUnregisteredParameterType(MyUnregisteredType);
+    void slotWithOneUnregisteredParameterType(QString a1, MyUnregisteredType a2);
 
     CountedStruct throwingSlot(const CountedStruct &, CountedStruct s2) {
 #ifndef QT_NO_EXCEPTIONS
@@ -604,6 +605,9 @@ void QtTestObject::testSender()
 void QtTestObject::slotWithUnregisteredParameterType(MyUnregisteredType)
 { slotResult = "slotWithUnregisteredReturnType"; }
 
+void QtTestObject::slotWithOneUnregisteredParameterType(QString a1, MyUnregisteredType)
+{ slotResult = "slotWithUnregisteredReturnType-" + a1; }
+
 void QtTestObject::staticFunction0()
 {
     staticResult = "staticFunction0";
@@ -885,6 +889,16 @@ void tst_QMetaObject::invokeQueuedMetaMember()
         QVERIFY(!QMetaObject::invokeMethod(&obj, "slotWithUnregisteredParameterType", Qt::QueuedConnection, Q_ARG(MyUnregisteredType, t)));
         QVERIFY(obj.slotResult.isEmpty());
     }
+
+    obj.slotResult.clear();
+    {
+        QString a1("Cannot happen");
+        MyUnregisteredType t;
+        QTest::ignoreMessage(QtWarningMsg, "QMetaMethod::invoke: Unable to handle unregistered datatype 'MyUnregisteredType'");
+        QVERIFY(!QMetaObject::invokeMethod(&obj, "slotWithOneUnregisteredParameterType", Qt::QueuedConnection,
+                                           Q_ARG(QString, a1), Q_ARG(MyUnregisteredType, t)));
+        QVERIFY(obj.slotResult.isEmpty());
+    }
 }
 
 void tst_QMetaObject::invokeQueuedPointer()