QObjectWrapper: Try harder to avoid creating wrappers if we can

When searching a QObject for properties, try to find QObject properties first,
and then bail if we don't find a match and the object is not already wrapped.

It makes no sense to wrap the object just to check the (empty) JS side
props, which is often the case with QmlContextWrapper's scopeObject and
contextObject. This can especially be seen when looking at QML's model
activity, as they set a context object in order to hijack lookup of
model data & index.

This can be seen in a test like the following:

    Repeater {
        model: 10
        delegate: Text {
            text: index
        }
    }

Before this patch, the 'index' access would previously trigger wrapping
of the QQmlDelegateModelItem. The same applies to the 'model' prop etc.

Noteworthy improvements of a full qmlbench run that aren't part of the "usual"
statistical noise are generally around the 5% mark.

Outside performance, the advantages are also clear on memory. Examining
for instance delegates_flipable.qml with QV4_MM_STATS=1 on my MBP:

Before:
     Freed JS type: QmlContext (5000 instances)
     Freed JS type: QObjectWrapper (5000 instances)
     Freed JS type: QmlContextWrapper (5000 instances)
     Freed JS type: QQuickItemWrapper (5000 instances)

After:
     Freed JS type: QmlContext (5000 instances)
     Freed JS type: QmlContextWrapper (5000 instances)
     Freed JS type: QQuickItemWrapper (5000 instances)

... which is what we expect, no more QObjectWrappers here, a lot less garbage
to clear up.

Timewise, while the measured mark/sweep times are not stable, there is a
proportional decrease of about 40% in time for both mark and sweep phases in
the GC (slightly more on TX1 than desktop, curiously).

Finally, GC's memory use is quite stable for each round of the test, and in this
case, we see a decrease of 378kb used at the time of each GC.

Before:
     Allocated 2515968 bytes in 39 chunks
     Used memory before GC: 2489824
     Used memory after GC : 889856
     Freed up bytes       : 1599968

After:
     Allocated 2128896 bytes in 33 chunks
     Used memory before GC: 2098304
     Used memory after GC : 818304
     Freed up bytes       : 1280000

As a final note, while this provides an improvement to many of qmlbench's
benchmarks, as they make heavy use of Repeater, these benefits should map quite
well to real world applications as well: anything that uses item views will
likely see a significant reduction to the amount of GC'd objects.

In the future, it would be nice to have tests for the amount of garbage
created by various activities, but that is an item for another day.

Change-Id: Idd14180e689235eea8883d68487570b0e14d47e6
Reviewed-by: Michael Brasser <michael.brasser@live.com>
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>

2 files changed, 100 insertions, 57 deletions

diff --git a/src/qml/jsruntime/qv4qobjectwrapper.cpp b/src/qml/jsruntime/qv4qobjectwrapper.cpp
index 1dd90995d3..d86a51e3a4 100644
--- a/src/qml/jsruntime/qv4qobjectwrapper.cpp
+++ b/src/qml/jsruntime/qv4qobjectwrapper.cpp
@@ -202,19 +202,61 @@ void QObjectWrapper::initializeBindings(ExecutionEngine *engine)
 
 QQmlPropertyData *QObjectWrapper::findProperty(ExecutionEngine *engine, QQmlContextData *qmlContext, String *name, RevisionMode revisionMode, QQmlPropertyData *local) const
 {
+    QObject *o = d()->object();
+    return findProperty(engine, o, qmlContext, name, revisionMode, local);
+}
+
+QQmlPropertyData *QObjectWrapper::findProperty(ExecutionEngine *engine, QObject *o, QQmlContextData *qmlContext, String *name, RevisionMode revisionMode, QQmlPropertyData *local)
+{
     Q_UNUSED(revisionMode);
 
-    QQmlData *ddata = QQmlData::get(d()->object(), false);
-    if (!ddata)
-        return 0;
+    QQmlData *ddata = QQmlData::get(o, false);
     QQmlPropertyData *result = 0;
     if (ddata && ddata->propertyCache)
-        result = ddata->propertyCache->property(name, d()->object(), qmlContext);
+        result = ddata->propertyCache->property(name, o, qmlContext);
     else
-        result = QQmlPropertyCache::property(engine->jsEngine(), d()->object(), name, qmlContext, *local);
+        result = QQmlPropertyCache::property(engine->jsEngine(), o, name, qmlContext, *local);
     return result;
 }
 
+ReturnedValue QObjectWrapper::getProperty(ExecutionEngine *engine, QObject *object, QQmlPropertyData *property, bool captureRequired)
+{
+    QQmlData::flushPendingBinding(object, QQmlPropertyIndex(property->coreIndex()));
+
+    if (property->isFunction() && !property->isVarProperty()) {
+        if (property->isVMEFunction()) {
+            QQmlVMEMetaObject *vmemo = QQmlVMEMetaObject::get(object);
+            Q_ASSERT(vmemo);
+            return vmemo->vmeMethod(property->coreIndex());
+        } else if (property->isV4Function()) {
+            Scope scope(engine);
+            ScopedContext global(scope, engine->qmlContext());
+            if (!global)
+                global = engine->rootContext();
+            return QV4::QObjectMethod::create(global, object, property->coreIndex());
+        } else if (property->isSignalHandler()) {
+            QmlSignalHandler::initProto(engine);
+            return engine->memoryManager->allocObject<QV4::QmlSignalHandler>(object, property->coreIndex())->asReturnedValue();
+        } else {
+            ExecutionContext *global = engine->rootContext();
+            return QV4::QObjectMethod::create(global, object, property->coreIndex());
+        }
+    }
+
+    QQmlEnginePrivate *ep = engine->qmlEngine() ? QQmlEnginePrivate::get(engine->qmlEngine()) : 0;
+
+    if (captureRequired && ep && ep->propertyCapture && !property->isConstant())
+        ep->propertyCapture->captureProperty(object, property->coreIndex(), property->notifyIndex());
+
+    if (property->isVarProperty()) {
+        QQmlVMEMetaObject *vmemo = QQmlVMEMetaObject::get(object);
+        Q_ASSERT(vmemo);
+        return vmemo->vmeProperty(property->coreIndex());
+    } else {
+        return loadProperty(engine, object, *property);
+    }
+}
+
 ReturnedValue QObjectWrapper::getQmlProperty(QQmlContextData *qmlContext, String *name, QObjectWrapper::RevisionMode revisionMode,
                                              bool *hasProperty, bool includeImports) const
 {
@@ -279,42 +321,19 @@ ReturnedValue QObjectWrapper::getQmlProperty(QQmlContextData *qmlContext, String
     return getProperty(v4, d()->object(), result);
 }
 
-ReturnedValue QObjectWrapper::getProperty(ExecutionEngine *engine, QObject *object, QQmlPropertyData *property, bool captureRequired)
+ReturnedValue QObjectWrapper::getProperty(ExecutionEngine *engine, QObject *object, int propertyIndex, bool captureRequired)
 {
-    QQmlData::flushPendingBinding(object, QQmlPropertyIndex(property->coreIndex()));
-
-    if (property->isFunction() && !property->isVarProperty()) {
-        if (property->isVMEFunction()) {
-            QQmlVMEMetaObject *vmemo = QQmlVMEMetaObject::get(object);
-            Q_ASSERT(vmemo);
-            return vmemo->vmeMethod(property->coreIndex());
-        } else if (property->isV4Function()) {
-            Scope scope(engine);
-            ScopedContext global(scope, engine->qmlContext());
-            if (!global)
-                global = engine->rootContext();
-            return QV4::QObjectMethod::create(global, object, property->coreIndex());
-        } else if (property->isSignalHandler()) {
-            QmlSignalHandler::initProto(engine);
-            return engine->memoryManager->allocObject<QV4::QmlSignalHandler>(object, property->coreIndex())->asReturnedValue();
-        } else {
-            ExecutionContext *global = engine->rootContext();
-            return QV4::QObjectMethod::create(global, object, property->coreIndex());
-        }
-    }
-
-    QQmlEnginePrivate *ep = engine->qmlEngine() ? QQmlEnginePrivate::get(engine->qmlEngine()) : 0;
-
-    if (captureRequired && ep && ep->propertyCapture && !property->isConstant())
-        ep->propertyCapture->captureProperty(object, property->coreIndex(), property->notifyIndex());
+    if (QQmlData::wasDeleted(object))
+        return QV4::Encode::null();
+    QQmlData *ddata = QQmlData::get(object, /*create*/false);
+    if (!ddata)
+        return QV4::Encode::undefined();
 
-    if (property->isVarProperty()) {
-        QQmlVMEMetaObject *vmemo = QQmlVMEMetaObject::get(object);
-        Q_ASSERT(vmemo);
-        return vmemo->vmeProperty(property->coreIndex());
-    } else {
-        return loadProperty(engine, object, *property);
-    }
+    QQmlPropertyCache *cache = ddata->propertyCache;
+    Q_ASSERT(cache);
+    QQmlPropertyData *property = cache->property(propertyIndex);
+    Q_ASSERT(property); // We resolved this property earlier, so it better exist!
+    return getProperty(engine, object, property, captureRequired);
 }
 
 ReturnedValue QObjectWrapper::getQmlProperty(QV4::ExecutionEngine *engine, QQmlContextData *qmlContext, QObject *object, String *name, QObjectWrapper::RevisionMode revisionMode, bool *hasProperty)
@@ -325,12 +344,49 @@ ReturnedValue QObjectWrapper::getQmlProperty(QV4::ExecutionEngine *engine, QQmlC
         return QV4::Encode::null();
     }
 
-    if (!QQmlData::get(object, true)) {
+    if (name->equals(engine->id_destroy()) || name->equals(engine->id_toString())) {
+        int index = name->equals(engine->id_destroy()) ? QV4::QObjectMethod::DestroyMethod : QV4::QObjectMethod::ToStringMethod;
         if (hasProperty)
-            *hasProperty = false;
-        return QV4::Encode::null();
+            *hasProperty = true;
+        ExecutionContext *global = engine->rootContext();
+        return QV4::QObjectMethod::create(global, object, index);
     }
 
+    QQmlData *ddata = QQmlData::get(object, false);
+    QQmlPropertyData local;
+    QQmlPropertyData *result = findProperty(engine, object, qmlContext, name, revisionMode, &local);
+
+    if (result) {
+        if (revisionMode == QV4::QObjectWrapper::CheckRevision && result->hasRevision()) {
+            if (ddata && ddata->propertyCache && !ddata->propertyCache->isAllowedInRevision(result)) {
+                if (hasProperty)
+                    *hasProperty = false;
+                return QV4::Encode::undefined();
+            }
+        }
+
+        if (hasProperty)
+            *hasProperty = true;
+
+        return getProperty(engine, object, result);
+    } else {
+        // Check if this object is already wrapped.
+        if (!ddata || (ddata->jsWrapper.isUndefined() &&
+                        (ddata->jsEngineId == 0 || // Nobody owns the QObject
+                          !ddata->hasTaintedV4Object))) { // Someone else has used the QObject, but it isn't tainted
+
+            // Not wrapped. Last chance: try query QObjectWrapper's prototype.
+            // If it can't handle this, then there is no point
+            // to wrap the QObject just to look at an empty set of JS props.
+            QV4::Object *proto = QObjectWrapper::defaultPrototype(engine);
+            return proto->get(name, hasProperty);
+        }
+    }
+
+    // If we get here, we must already be wrapped (which implies a ddata).
+    // There's no point wrapping again, as there wouldn't be any new props.
+    Q_ASSERT(ddata);
+
     QV4::Scope scope(engine);
     QV4::Scoped<QObjectWrapper> wrapper(scope, wrap(engine, object));
     if (!wrapper) {
@@ -341,6 +397,7 @@ ReturnedValue QObjectWrapper::getQmlProperty(QV4::ExecutionEngine *engine, QQmlC
     return wrapper->getQmlProperty(qmlContext, name, revisionMode, hasProperty);
 }
 
+
 bool QObjectWrapper::setQmlProperty(ExecutionEngine *engine, QQmlContextData *qmlContext, QObject *object, String *name,
                                     QObjectWrapper::RevisionMode revisionMode, const Value &value)
 {
@@ -554,21 +611,6 @@ void QObjectWrapper::markWrapper(QObject *object, MarkStack *markStack)
         markStack->engine->m_multiplyWrappedQObjects->mark(object, markStack);
 }
 
-ReturnedValue QObjectWrapper::getProperty(ExecutionEngine *engine, QObject *object, int propertyIndex, bool captureRequired)
-{
-    if (QQmlData::wasDeleted(object))
-        return QV4::Encode::null();
-    QQmlData *ddata = QQmlData::get(object, /*create*/false);
-    if (!ddata)
-        return QV4::Encode::undefined();
-
-    QQmlPropertyCache *cache = ddata->propertyCache;
-    Q_ASSERT(cache);
-    QQmlPropertyData *property = cache->property(propertyIndex);
-    Q_ASSERT(property); // We resolved this property earlier, so it better exist!
-    return getProperty(engine, object, property, captureRequired);
-}
-
 void QObjectWrapper::setProperty(ExecutionEngine *engine, int propertyIndex, const Value &value)
 {
     setProperty(engine, d()->object(), propertyIndex, value);
diff --git a/src/qml/jsruntime/qv4qobjectwrapper_p.h b/src/qml/jsruntime/qv4qobjectwrapper_p.h
index 55700d17c1..018e444f7c 100644
--- a/src/qml/jsruntime/qv4qobjectwrapper_p.h
+++ b/src/qml/jsruntime/qv4qobjectwrapper_p.h
@@ -189,6 +189,7 @@ protected:
 
     static ReturnedValue create(ExecutionEngine *engine, QObject *object);
 
+    static QQmlPropertyData *findProperty(ExecutionEngine *engine, QObject *o, QQmlContextData *qmlContext, String *name, RevisionMode revisionMode, QQmlPropertyData *local);
     QQmlPropertyData *findProperty(ExecutionEngine *engine, QQmlContextData *qmlContext, String *name, RevisionMode revisionMode, QQmlPropertyData *local) const;
 
     static ReturnedValue get(const Managed *m, String *name, bool *hasProperty);