Evaluate bindings more intelligently during construction

Instead of just evaluating bindings in a fixed order, and possibly
having to evaluate a single binding multiple times, prior to reading
a property, we check if there are any bindings "pending" on it and
evaluate them then.  A pending binding is one that has been assigned
to the property, but not yet evaluated.

To minimize side effects we only do this for "safe" bindings.  A
safe binding is one that has no side effects, which we currently
define as not calling functions or otherwise assigning values during
its evaluation.  This isn't an entirely foolproof way to ensure that
the evaluation has no side effects, but it should be good enough.

Change-Id: I98aa76a95719e5d182e8941738d64f8d409f404a
Reviewed-by: Michael Brasser <michael.brasser@nokia.com>

5 files changed, 59 insertions, 40 deletions

diff --git a/src/qml/qml/v4/qv4bindings.cpp b/src/qml/qml/v4/qv4bindings.cpp
index 2ad69de055..4d6bf92567 100644
--- a/src/qml/qml/v4/qv4bindings.cpp
+++ b/src/qml/qml/v4/qv4bindings.cpp
@@ -448,6 +448,11 @@ void QV4Bindings::Binding::disconnect()
     }
 }
 
+void QV4Bindings::Binding::dump()
+{
+    qWarning() << parent->context()->url << instruction->line << instruction->column;
+}
+
 QV4Bindings::Subscription::Subscription()
     : m_bindings(0)
 {
@@ -884,23 +889,33 @@ inline quint32 QV4Bindings::toUint32(double n)
     MARK_REGISTER(reg); \
 }
 
-//TODO: avoid construction of name and name-based lookup
-#define INVALIDATION_CHECK(inv, obj, index) { \
-    if ((inv) != 0) { \
-        QQmlData *data = QQmlData::get((obj)); \
-        if (data && !data->propertyCache) { \
-            data->propertyCache = QQmlEnginePrivate::get(context->engine)->cache(object); \
-            if (data->propertyCache) data->propertyCache->addref(); \
-        } \
-        QQmlPropertyData *prop = (data && data->propertyCache) ? data->propertyCache->property((index)) : 0; \
-        if (prop && prop->isOverridden()) { \
-            int resolvedIndex = data->propertyCache->property(prop->name(obj), obj, context)->coreIndex; \
-            if ((int)index < resolvedIndex) { \
-                *(inv) = true; \
-                goto programExit; \
-            } \
-        } \
-    } \
+namespace {
+
+bool bindingInvalidated(bool *invalidated, QObject *obj, QQmlContextData *context, int index)
+{
+    if (invalidated != 0) {
+        if (QQmlData *data = QQmlData::get(obj, true)) {
+            if (!data->propertyCache) {
+                data->propertyCache = QQmlEnginePrivate::get(context->engine)->cache(obj);
+                if (data->propertyCache) data->propertyCache->addref();
+            }
+
+            if (QQmlPropertyData *prop = data->propertyCache ? data->propertyCache->property(index) : 0) {
+                if (prop->isOverridden()) {
+                    // TODO: avoid construction of name and name-based lookup
+                    int resolvedIndex = data->propertyCache->property(prop->name(obj), obj, context)->coreIndex;
+                    if (index < resolvedIndex) {
+                        *invalidated = true;
+                        return true;
+                    }
+                }
+            }
+        }
+    }
+
+    return false;
+}
+
 }
 
 #ifdef QML_THREADED_INTERPRETER
@@ -978,15 +993,6 @@ void QV4Bindings::run(int instrIndex, quint32 &executedBlocks,
         subscribeId(context, instr->subscribeop.index, instr->subscribeop.offset);
     QML_V4_END_INSTR(SubscribeId, subscribeop)
 
-    QML_V4_BEGIN_INSTR(Subscribe, subscribeop)
-    {
-        QObject *o = 0;
-        const Register &object = registers[instr->subscribeop.reg];
-        if (!object.isUndefined()) o = object.getQObject();
-        subscribe(o, instr->subscribeop.index, instr->subscribeop.offset, context->engine);
-    }
-    QML_V4_END_INSTR(Subscribe, subscribeop)
-
     QML_V4_BEGIN_INSTR(FetchAndSubscribe, fetchAndSubscribe)
     {
         Register &reg = registers[instr->fetchAndSubscribe.reg];
@@ -998,12 +1004,16 @@ void QV4Bindings::run(int instrIndex, quint32 &executedBlocks,
         if (!object) {
             THROW_EXCEPTION(instr->fetchAndSubscribe.exceptionId);
         } else {
-            INVALIDATION_CHECK(invalidated, object, instr->fetchAndSubscribe.property.coreIndex);
+            if (bindingInvalidated(invalidated, object, context, instr->fetchAndSubscribe.property.coreIndex))
+                goto programExit;
 
             const Register::Type valueType = (Register::Type)instr->fetchAndSubscribe.valueType;
             reg.init(valueType);
             if (instr->fetchAndSubscribe.valueType >= FirstCleanupType)
                 MARK_REGISTER(instr->fetchAndSubscribe.reg);
+
+            QQmlData::flushPendingBinding(object, instr->fetchAndSubscribe.property.coreIndex);
+
             QQmlAccessors *accessors = instr->fetchAndSubscribe.property.accessors;
             accessors->read(object, instr->fetchAndSubscribe.property.accessorData,
                             reg.typeDataPtr());
@@ -2313,12 +2323,16 @@ void QV4Bindings::run(int instrIndex, quint32 &executedBlocks,
         if (!object) {
             THROW_EXCEPTION(instr->fetch.exceptionId);
         } else {
-            INVALIDATION_CHECK(invalidated, object, instr->fetch.index);
+            if (bindingInvalidated(invalidated, object, context, instr->fetch.index))
+                goto programExit;
 
             const Register::Type valueType = (Register::Type)instr->fetch.valueType;
             reg.init(valueType);
             if (instr->fetch.valueType >= FirstCleanupType)
                 MARK_REGISTER(instr->fetch.reg);
+
+            QQmlData::flushPendingBinding(object, instr->fetch.index);
+
             void *argv[] = { reg.typeDataPtr(), 0 };
             QMetaObject::metacall(object, QMetaObject::ReadProperty, instr->fetch.index, argv);
             if (valueType == FloatType) {
@@ -2326,6 +2340,10 @@ void QV4Bindings::run(int instrIndex, quint32 &executedBlocks,
                 const double v = reg.getfloat();
                 reg.setnumber(v);
             }
+
+            if (instr->fetch.subIndex != static_cast<quint32>(-1))
+                subscribe(object, instr->fetch.subIndex, instr->fetch.subOffset, context->engine);
+
         }
     }
     QML_V4_END_INSTR(Fetch, fetch)
diff --git a/src/qml/qml/v4/qv4bindings_p.h b/src/qml/qml/v4/qv4bindings_p.h
index 4053c471fa..99918215b6 100644
--- a/src/qml/qml/v4/qv4bindings_p.h
+++ b/src/qml/qml/v4/qv4bindings_p.h
@@ -93,6 +93,8 @@ public:
 
         void disconnect();
 
+        void dump();
+
         struct Retarget {
             QObject *target;
             int targetProperty;
diff --git a/src/qml/qml/v4/qv4compiler.cpp b/src/qml/qml/v4/qv4compiler.cpp
index d5f44c4085..9c1053d01a 100644
--- a/src/qml/qml/v4/qv4compiler.cpp
+++ b/src/qml/qml/v4/qv4compiler.cpp
@@ -407,19 +407,20 @@ void QV4CompilerPrivate::visitName(IR::Name *e)
             fetch.property = *e->property;
             gen(fetch);
         } else {
-            if (blockNeedsSubscription(_subscribeName) && e->property->notifyIndex != -1) {
-                Instr::Subscribe sub;
-                sub.reg = currentReg;
-                sub.offset = subscriptionIndex(_subscribeName);
-                sub.index = e->property->notifyIndex;
-                gen(sub);
-            }
-
             Instr::Fetch fetch;
             fetch.reg = currentReg;
             fetch.index = e->property->coreIndex;
             fetch.exceptionId = exceptionId(e->line, e->column);
             fetch.valueType = regType;
+
+            if (blockNeedsSubscription(_subscribeName) && e->property->notifyIndex != -1) {
+                fetch.subOffset = subscriptionIndex(_subscribeName);
+                fetch.subIndex = e->property->notifyIndex;
+            } else {
+                fetch.subOffset = static_cast<quint16>(-1);
+                fetch.subIndex = static_cast<quint32>(-1);
+            }
+
             gen(fetch);
         }
 
diff --git a/src/qml/qml/v4/qv4instruction.cpp b/src/qml/qml/v4/qv4instruction.cpp
index 252c9e9a7a..d550450160 100644
--- a/src/qml/qml/v4/qv4instruction.cpp
+++ b/src/qml/qml/v4/qv4instruction.cpp
@@ -96,9 +96,6 @@ void Bytecode::dump(const V4Instr *i, int address) const
     case V4Instr::BindingId:
         INSTR_DUMP << i->id.line << ':' << i->id.column << ':';
         break;
-    case V4Instr::Subscribe:
-        INSTR_DUMP << '\t' << "Subscribe" << "\t\t" << "Object_Reg(" << i->subscribeop.reg << ") Notify_Signal(" << i->subscribeop.index << ") -> Subscribe_Slot(" << i->subscribeop.offset << ')';
-        break;
     case V4Instr::SubscribeId:
         INSTR_DUMP << '\t' << "SubscribeId" << "\t\t" << "Id_Offset(" << i->subscribeop.index << ") -> Subscribe_Slot(" << i->subscribeop.offset << ')';
         break;
diff --git a/src/qml/qml/v4/qv4instruction_p.h b/src/qml/qml/v4/qv4instruction_p.h
index 34d483b079..a918992b9d 100644
--- a/src/qml/qml/v4/qv4instruction_p.h
+++ b/src/qml/qml/v4/qv4instruction_p.h
@@ -67,7 +67,6 @@ QT_BEGIN_NAMESPACE
 #define FOR_EACH_V4_INSTR(F) \
     F(Noop, common) \
     F(BindingId, id) \
-    F(Subscribe, subscribeop) \
     F(SubscribeId, subscribeop) \
     F(FetchAndSubscribe, fetchAndSubscribe) \
     F(LoadId, load) \
@@ -291,6 +290,8 @@ union Q_AUTOTEST_EXPORT V4Instr {
         quint8 exceptionId;
         quint8 valueType;
         quint32 index;
+        quint16 subOffset;
+        quint32 subIndex;
     };
 
     struct instr_copy {