Lazily construct the QScriptActivationObject

We can store flags on the ReturnValueRegister entry in the stackframe
header (as native function don't use that)
Then when requesting an activation object we can lookup the flags to
know if we should create it.

This reduce a lot the cost of a native call.

Reviewed-by: Kent Hansen

6 files changed, 109 insertions, 40 deletions

diff --git a/src/script/api/qscriptcontext.cpp b/src/script/api/qscriptcontext.cpp
index 2d85450..f397ab5 100644
--- a/src/script/api/qscriptcontext.cpp
+++ b/src/script/api/qscriptcontext.cpp
@@ -329,18 +329,10 @@ bool QScriptContext::isCalledAsConstructor() const
 {
     JSC::CallFrame *frame = const_cast<JSC::ExecState*>(QScriptEnginePrivate::frameForContext(this));
 
-    //For native functions, look up for the QScriptActivationObject and its calledAsConstructor flag.
-    JSC::ScopeChainNode *node = frame->scopeChain();
-    JSC::ScopeChainIterator it(node);
-    for (it = node->begin(); it != node->end(); ++it) {
-        if ((*it) && (*it)->isVariableObject()) {
-            if ((*it)->inherits(&QScript::QScriptActivationObject::info)) {
-                return static_cast<QScript::QScriptActivationObject *>(*it)->d_ptr()->calledAsConstructor;
-            }
-            //not a native function
-            break;
-        }
-    }
+    //For native functions, look up flags.
+    uint flags = QScriptEnginePrivate::contextFlags(frame);
+    if (flags & QScriptEnginePrivate::NativeContext)
+        return flags & QScriptEnginePrivate::CalledAsConstructorContext;
 
     //Not a native function, try to look up in the bytecode if we where called from op_construct
     JSC::Instruction* returnPC = frame->returnPC();
@@ -422,17 +414,21 @@ void QScriptContext::setReturnValue(const QScriptValue &result)
 
   \sa argument(), argumentsObject()
 */
+
 QScriptValue QScriptContext::activationObject() const
 {
     JSC::CallFrame *frame = const_cast<JSC::ExecState*>(QScriptEnginePrivate::frameForContext(this));
-    // ### this is still a bit shaky
-    // if properties of the activation are accessed after this context is
-    // popped, we CRASH.
-    // Ideally we should be able to store the activation object in the callframe
-    // and JSC would clean it up for us.
     JSC::JSObject *result = 0;
-    // look in scope chain
-    {
+
+    uint flags = QScriptEnginePrivate::contextFlags(frame);
+    if ((flags & QScriptEnginePrivate::NativeContext) && !(flags & QScriptEnginePrivate::HasScopeContext)) {
+        //For native functions, lazily create it if needed
+        QScript::QScriptActivationObject *scope = new (frame) QScript::QScriptActivationObject(frame);
+        frame->setScopeChain(frame->scopeChain()->copy()->push(scope));
+        result = scope;
+        QScriptEnginePrivate::setContextFlags(frame, flags | QScriptEnginePrivate::HasScopeContext);
+    } else {
+        // look in scope chain
         JSC::ScopeChainNode *node = frame->scopeChain();
         JSC::ScopeChainIterator it(node);
         for (it = node->begin(); it != node->end(); ++it) {
@@ -443,14 +439,21 @@ QScriptValue QScriptContext::activationObject() const
         }
     }
     if (!result) {
-        JSC::CodeBlock *codeBlock = frame->codeBlock();
+        if (!parentContext())
+            return engine()->globalObject();
+
+        qWarning("QScriptContext::activationObject:  could not get activation object for frame");
+        return QScriptValue();
+        /*JSC::CodeBlock *codeBlock = frame->codeBlock();
         if (!codeBlock) {
-            // native function
+            // non-Qt native function 
+            Q_ASSERT(true); //### this should in theorry not happen
             result = new (frame)QScript::QScriptActivationObject(frame);
         } else {
+            // ### this is wrong
             JSC::FunctionBodyNode *body = static_cast<JSC::FunctionBodyNode*>(codeBlock->ownerNode());
             result = new (frame)JSC::JSActivation(frame, body);
-        }
+        }*/
     }
     return QScript::scriptEngineFromExec(frame)->scriptValueFromJSCValue(result);
 }
@@ -481,7 +484,15 @@ void QScriptContext::setActivationObject(const QScriptValue &activation)
         return;
     }
 
-    // replace the first activation object in the scope chain
+    uint flags = QScriptEnginePrivate::contextFlags(frame);
+    if ((flags & QScriptEnginePrivate::NativeContext) && !(flags & QScriptEnginePrivate::HasScopeContext)) {
+        //For native functions, we create a scope node
+        frame->setScopeChain(frame->scopeChain()->copy()->push(object));
+        QScriptEnginePrivate::setContextFlags(frame, flags | QScriptEnginePrivate::HasScopeContext);
+        return;
+    }
+
+    // else replace the first activation object in the scope chain
     JSC::ScopeChainNode *node = frame->scopeChain();
     while (node != 0) {
         if (node->object && node->object->isVariableObject()) {
@@ -630,6 +641,7 @@ QString QScriptContext::toString() const
 */
 QScriptValueList QScriptContext::scopeChain() const
 {
+    activationObject(); //ensure the creation of the normal scope for native context
     const JSC::CallFrame *frame = QScriptEnginePrivate::frameForContext(this);
     QScriptEnginePrivate *engine = QScript::scriptEngineFromExec(frame);
     QScriptValueList result;
@@ -652,6 +664,7 @@ QScriptValueList QScriptContext::scopeChain() const
 */
 void QScriptContext::pushScope(const QScriptValue &object)
 {
+    activationObject(); //ensure the creation of the normal scope for native context
     if (!object.isObject())
         return;
     else if (object.engine() != engine()) {
diff --git a/src/script/api/qscriptengine.cpp b/src/script/api/qscriptengine.cpp
index e8ea222..daf97d5 100644
--- a/src/script/api/qscriptengine.cpp
+++ b/src/script/api/qscriptengine.cpp
@@ -1114,6 +1114,30 @@ JSC::JSValue QScriptEnginePrivate::thisForContext(JSC::ExecState *frame)
     }
 }
 
+/*! \internal
+     For native context, we use the ReturnValueRegister entry in the stackframe header to store flags.
+     We can do that because this header is not used as the native function return their value thought C++
+
+     when setting flags, NativeContext should always be set
+
+     contextFlags returns 0 for non native context
+ */
+uint QScriptEnginePrivate::contextFlags(JSC::ExecState *exec)
+{
+    if (exec->codeBlock())
+        return 0; //js function doesn't have flags
+
+    return exec->returnValueRegister();
+}
+
+void QScriptEnginePrivate::setContextFlags(JSC::ExecState *exec, uint flags)
+{
+    Q_ASSERT(!exec->codeBlock());
+    quintptr flag_ptr = flags;
+    exec->registers()[JSC::RegisterFile::ReturnValueRegister] = JSC::JSValue(reinterpret_cast<JSC::JSObject*>(flag_ptr));
+}
+
+
 void QScriptEnginePrivate::mark()
 {
     if (!originalGlobalObject()->marked())
@@ -2126,6 +2150,7 @@ QScriptValue QScriptEngine::evaluate(const QString &program, const QString &file
     Q_D(QScriptEngine);
 
     JSC::JSLock lock(false); // ### hmmm
+    currentContext()->activationObject(); //force the creation of a context for native function;
 
     JSC::UString jscProgram = QScript::qtStringToJSCUString(program);
     JSC::UString jscFileName = QScript::qtStringToJSCUString(fileName);
@@ -2269,6 +2294,10 @@ JSC::CallFrame *QScriptEnginePrivate::pushContext(JSC::CallFrame *exec, const JS
         thisObject = new (exec) QScriptObject(structure);
     }
 
+    int flags = NativeContext;
+    if (calledAsConstructor)
+        flags |= CalledAsConstructorContext;
+
     JSC::CallFrame *newCallFrame = exec;
     if (callee == 0 || !(exec->callee() == callee && exec->returnPC() != 0)) {
         //We need to check if the Interpreter might have already created a frame for function called from JS.
@@ -2285,16 +2314,16 @@ JSC::CallFrame *QScriptEnginePrivate::pushContext(JSC::CallFrame *exec, const JS
         for (it = args.begin(); it != args.end(); ++it)
             newCallFrame[++dst] = *it;
         newCallFrame += argc + JSC::RegisterFile::CallFrameHeaderSize;
-        newCallFrame->init(0, /*vPC=*/0, exec->scopeChain(), exec, 0, argc, callee);
-    } else if (calledAsConstructor) {
-        //update the new created this
-        JSC::Register* thisRegister = newCallFrame->registers() - JSC::RegisterFile::CallFrameHeaderSize - newCallFrame->argumentCount();
-        *thisRegister = thisObject;
+        newCallFrame->init(0, /*vPC=*/0, exec->scopeChain(), exec, flags, argc, callee);
+    } else {
+        setContextFlags(newCallFrame, flags);
+        if (calledAsConstructor) {
+            //update the new created this
+            JSC::Register* thisRegister = newCallFrame->registers() - JSC::RegisterFile::CallFrameHeaderSize - newCallFrame->argumentCount();
+            *thisRegister = thisObject;
+        }
     }
     currentFrame = newCallFrame;
-    QScript::QScriptActivationObject *scope = new (newCallFrame) QScript::QScriptActivationObject(newCallFrame);
-    scope->d_ptr()->calledAsConstructor = calledAsConstructor;
-    newCallFrame->setScopeChain(newCallFrame->scopeChain()->copy()->push(scope));
     return newCallFrame;
 }
 
@@ -2311,7 +2340,7 @@ void QScriptEngine::popContext()
         agent()->contextPop();
     Q_D(QScriptEngine);
     if (d->currentFrame->returnPC() != 0 || d->currentFrame->codeBlock() != 0
-        || d->currentFrame->returnValueRegister() != 0 || !currentContext()->parentContext()) {
+        || !currentContext()->parentContext()) {
         qWarning("QScriptEngine::popContext() doesn't match with pushContext()");
         return;
     }
@@ -2324,17 +2353,18 @@ void QScriptEngine::popContext()
  */
 void QScriptEnginePrivate::popContext()
 {
+    bool hasScope = contextFlags(currentFrame) & HasScopeContext;
     if (currentFrame->returnPC() == 0) { //normal case
         JSC::RegisterFile &registerFile = currentFrame->interpreter()->registerFile();
         JSC::Register *const newEnd = currentFrame->registers() - JSC::RegisterFile::CallFrameHeaderSize - currentFrame->argumentCount();
-        currentFrame->scopeChain()->pop()->deref();
+        if (hasScope)
+            currentFrame->scopeChain()->pop()->deref();
         currentFrame = currentFrame->callerFrame();
         registerFile.shrink(newEnd);
-    } else { //the stack frame was created by the Interpreter, we don't need to rewind it.
+    } else if(hasScope) { //the stack frame was created by the Interpreter, we don't need to rewind it.
         currentFrame->setScopeChain(currentFrame->scopeChain()->pop());
         currentFrame->scopeChain()->deref();
     }
-
 }
 
 /*!
diff --git a/src/script/api/qscriptengine_p.h b/src/script/api/qscriptengine_p.h
index 86c7616..1f59d36 100644
--- a/src/script/api/qscriptengine_p.h
+++ b/src/script/api/qscriptengine_p.h
@@ -165,6 +165,15 @@ public:
     bool isCollecting() const;
     void collectGarbage();
 
+    //flags that we set on the return value register for native function. (ie when codeBlock is 0)
+    enum ContextFlags {
+        NativeContext = 1,
+        CalledAsConstructorContext = 2,
+        HasScopeContext = 4
+    };
+    static uint contextFlags(JSC::ExecState *);
+    static void setContextFlags(JSC::ExecState *, uint);
+
     QScript::TimeoutCheckerProxy *timeoutChecker() const;
 
 #ifndef QT_NO_QOBJECT
diff --git a/src/script/api/qscriptvalue.cpp b/src/script/api/qscriptvalue.cpp
index b02802c..4558097 100644
--- a/src/script/api/qscriptvalue.cpp
+++ b/src/script/api/qscriptvalue.cpp
@@ -2020,6 +2020,7 @@ QScriptValue QScriptValue::call(const QScriptValue &thisObject,
                  "a different engine");
         return QScriptValue();
     }
+    engine()->currentContext()->activationObject(); //force the creation of a context for native function;
 
     QScriptEnginePrivate *eng_p = QScriptEnginePrivate::get(d->engine);
     JSC::ExecState *exec = eng_p->currentFrame;
@@ -2095,6 +2096,7 @@ QScriptValue QScriptValue::call(const QScriptValue &thisObject,
                  "a different engine");
         return QScriptValue();
     }
+    engine()->currentContext()->activationObject(); //force the creation of a context for native function;
 
     QScriptEnginePrivate *eng_p = QScriptEnginePrivate::get(d->engine);
     JSC::ExecState *exec = eng_p->currentFrame;
@@ -2161,6 +2163,7 @@ QScriptValue QScriptValue::construct(const QScriptValueList &args)
     Q_D(const QScriptValue);
     if (!isFunction())
         return QScriptValue();
+    engine()->currentContext()->activationObject(); //force the creation of a context for native function;
     QScriptEnginePrivate *eng_p = QScriptEnginePrivate::get(d->engine);
     JSC::ExecState *exec = eng_p->currentFrame;
 
@@ -2172,6 +2175,7 @@ QScriptValue QScriptValue::construct(const QScriptValueList &args)
         else
             argsVector[i] = eng_p->scriptValueToJSCValue(args.at(i));
     }
+
     JSC::ArgList jscArgs(argsVector.data(), argsVector.size());
 
     JSC::JSValue callee = d->jscValue;
@@ -2208,6 +2212,7 @@ QScriptValue QScriptValue::construct(const QScriptValue &arguments)
     Q_D(QScriptValue);
     if (!isFunction())
         return QScriptValue();
+    engine()->currentContext()->activationObject(); //force the creation of a context for native function;
     QScriptEnginePrivate *eng_p = QScriptEnginePrivate::get(d->engine);
     JSC::ExecState *exec = eng_p->currentFrame;
 
diff --git a/src/script/bridge/qscriptactivationobject_p.h b/src/script/bridge/qscriptactivationobject_p.h
index eb60ae5..8c62b23 100644
--- a/src/script/bridge/qscriptactivationobject_p.h
+++ b/src/script/bridge/qscriptactivationobject_p.h
@@ -74,12 +74,9 @@ public:
 
     struct QScriptActivationObjectData : public JSVariableObjectData {
         QScriptActivationObjectData(JSC::Register* registers)
-            : JSVariableObjectData(&symbolTable, registers), calledAsConstructor(false)
+            : JSVariableObjectData(&symbolTable, registers)
         { }
         JSC::SymbolTable symbolTable;
-
-        //specifies if the context of this activation object is called as constructor
-        bool calledAsConstructor;
     };
 
     QScriptActivationObjectData *d_ptr() const { return static_cast<QScriptActivationObjectData *>(d); }
diff --git a/tests/auto/qscriptengine/tst_qscriptengine.cpp b/tests/auto/qscriptengine/tst_qscriptengine.cpp
index f15ebdf..821bc0c 100644
--- a/tests/auto/qscriptengine/tst_qscriptengine.cpp
+++ b/tests/auto/qscriptengine/tst_qscriptengine.cpp
@@ -806,10 +806,26 @@ void tst_QScriptEngine::newQMetaObject()
     QVERIFY(instance3.instanceOf(qclass));
     args.clear();
 
+    QPointer<QObject> qpointer1 = instance.toQObject();
+    QPointer<QObject> qpointer2 = instance2.toQObject();
+    QPointer<QObject> qpointer3 = instance3.toQObject();
+
+    QVERIFY(qpointer1);
+    QVERIFY(qpointer2);
+    QVERIFY(qpointer3);
+
     // verify that AutoOwnership is in effect
     instance = QScriptValue();
     eng.collectGarbage();
+
+    QEXPECT_FAIL("","collectGarbage not working", Continue);
+    QVERIFY(!qpointer1);
+    QVERIFY(qpointer2);
+    QEXPECT_FAIL("","collectGarbage not working", Continue);
+    QVERIFY(!qpointer3); // was child of instance
+
     QVERIFY(instance.toQObject() == 0);
+    QEXPECT_FAIL("","collectGarbage not working", Continue);
     QVERIFY(instance3.toQObject() == 0); // was child of instance
     QVERIFY(instance2.toQObject() != 0);
     instance2 = QScriptValue();
@@ -2060,7 +2076,6 @@ void tst_QScriptEngine::collectGarbage()
         QScriptValue v = eng.newQObject(ptr, QScriptEngine::ScriptOwnership);
     }
     eng.collectGarbage();
-    QEXPECT_FAIL("", "", Continue);
     QVERIFY(ptr == 0);
 }