JavaScriptCore:
authorggaren <ggaren@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Oct 2007 07:09:44 +0000 (07:09 +0000)
committerggaren <ggaren@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Oct 2007 07:09:44 +0000 (07:09 +0000)
        Reviewed by Maciej Stachowiak.

        Fixed http://bugs.webkit.org/show_bug.cgi?id=15683
        Re-order declaration initialization to avoid calling hasProperty inside
        VarDeclNode::processDeclaration

        .7% speedup on SunSpider.

        * kjs/function.h:
        * kjs/function.cpp: Merged parameter processing into FunctionBodyNode's
        other processing of declared symbols, so the order of execution could
        change.

        * kjs/nodes.cpp:
        (KJS::VarDeclNode::getDeclarations): Added special case for the
        "arguments" property name, explained in the comment.

        (KJS::VarDeclNode::processDeclaration): Removed call to hasProperty
        in the case of function code, since we know the declared symbol
        management will resolve conflicts between symbols. Yay!

        (KJS::VarDeclListNode::getDeclarations): Now that VarDeclNode's
        implementation of getDeclarations is non-trivial, we can't take a
        short-cut here any longer -- we need to put the VarDecl node on the
        stack so it gets processed normally.

        (KJS::FunctionBodyNode::processDeclarations): Changed the order of
        processing to enforce mutual exclusion rules.

        * kjs/nodes.h:
        (KJS::DeclarationStacks::DeclarationStacks): Structure includes an
        ExecState now, for fast access to the "arguments" property name.

LayoutTests:

        Layout tests for bugs that might result from changes like
        http://bugs.webkit.org/show_bug.cgi?id=15683

        * fast/js/vardecl-preserve-parameters-expected.txt: Added.
        * fast/js/vardecl-preserve-parameters.html: Added.
        * fast/js/vardecl-preserve-vardecl-expected.txt: Added.
        * fast/js/vardecl-preserve-vardecl.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@27025 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JavaScriptCore/ChangeLog
JavaScriptCore/kjs/function.cpp
JavaScriptCore/kjs/function.h
JavaScriptCore/kjs/nodes.cpp
JavaScriptCore/kjs/nodes.h
LayoutTests/ChangeLog
LayoutTests/fast/js/vardecl-preserve-parameters-expected.txt [new file with mode: 0644]
LayoutTests/fast/js/vardecl-preserve-parameters.html [new file with mode: 0644]
LayoutTests/fast/js/vardecl-preserve-vardecl-expected.txt [new file with mode: 0644]
LayoutTests/fast/js/vardecl-preserve-vardecl.html [new file with mode: 0644]

index 24794907e792f4951668356972b06a940b9d83b8..e0f72444d9d4b099fce093b1c2c8887cd46e427f 100644 (file)
@@ -1,3 +1,38 @@
+2007-10-25  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Maciej Stachowiak.
+        
+        Fixed http://bugs.webkit.org/show_bug.cgi?id=15683
+        Re-order declaration initialization to avoid calling hasProperty inside
+        VarDeclNode::processDeclaration
+        
+        .7% speedup on SunSpider.
+
+        * kjs/function.h:
+        * kjs/function.cpp: Merged parameter processing into FunctionBodyNode's
+        other processing of declared symbols, so the order of execution could 
+        change.
+
+        * kjs/nodes.cpp:
+        (KJS::VarDeclNode::getDeclarations): Added special case for the 
+        "arguments" property name, explained in the comment.
+
+        (KJS::VarDeclNode::processDeclaration): Removed call to hasProperty
+        in the case of function code, since we know the declared symbol
+        management will resolve conflicts between symbols. Yay!
+
+        (KJS::VarDeclListNode::getDeclarations): Now that VarDeclNode's 
+        implementation of getDeclarations is non-trivial, we can't take a 
+        short-cut here any longer -- we need to put the VarDecl node on the 
+        stack so it gets processed normally.
+
+        (KJS::FunctionBodyNode::processDeclarations): Changed the order of 
+        processing to enforce mutual exclusion rules.
+
+        * kjs/nodes.h:
+        (KJS::DeclarationStacks::DeclarationStacks): Structure includes an 
+        ExecState now, for fast access to the "arguments" property name.
+
 2007-10-24  Eric Seidel  <eric@webkit.org>
 
         Reviewed by Maciej.
index e2918b918406501bda298ca79d172418f5bc6dc0..67ad38775fc7e12a4440896ca6d9636241d03c05 100644 (file)
@@ -76,8 +76,6 @@ JSValue* FunctionImp::callAsFunction(ExecState* exec, JSObject* thisObj, const L
     newExec.setException(exec->exception());
   ctx.setExecState(&newExec);
 
-  passInParameters(&newExec, args);
-
   Debugger* dbg = exec->dynamicInterpreter()->debugger();
   int sid = -1;
   int lineno = -1;
@@ -126,19 +124,6 @@ JSValue* FunctionImp::callAsFunction(ExecState* exec, JSObject* thisObj, const L
     return jsUndefined();
 }
 
-// ECMA 10.1.3q
-inline void FunctionImp::passInParameters(ExecState* exec, const List& args)
-{
-    Vector<Identifier>& parameters = body->parameters();
-
-    JSObject* variable = exec->context()->variableObject();
-
-    size_t size = parameters.size();
-    for (size_t i = 0; i < size; ++i) {
-      variable->put(exec, parameters[i], args[i], DontDelete);
-    }
-}
-
 JSValue* FunctionImp::argumentsGetter(ExecState* exec, JSObject*, const Identifier& propertyName, const PropertySlot& slot)
 {
   FunctionImp* thisObj = static_cast<FunctionImp*>(slot.slotBase());
index 8c87d9681dd748f64c8f3736e861602f6cb42a85..f104b3c4ea860a3a4a842bd6782bd912a1c9972a 100644 (file)
@@ -107,8 +107,6 @@ namespace KJS {
     static JSValue* argumentsGetter(ExecState*, JSObject*, const Identifier&, const PropertySlot&);
     static JSValue* callerGetter(ExecState*, JSObject*, const Identifier&, const PropertySlot&);
     static JSValue* lengthGetter(ExecState*, JSObject*, const Identifier&, const PropertySlot&);
-
-    void passInParameters(ExecState*, const List&);
   };
 
   class IndexToNameMap {
index 7d0877b239483cc165beb438b475debdc09df62f..f8279ccd27e98aaeae464f5c20e9f45e0d31fb12 100644 (file)
@@ -1766,7 +1766,13 @@ VarDeclNode::VarDeclNode(const Identifier &id, AssignExprNode *in, Type t)
 }
 
 void VarDeclNode::getDeclarations(DeclarationStacks& stacks)
-{ 
+{
+    // The normal check to avoid overwriting pre-existing values with variable
+    // declarations doesn't work for the "arguments" property because we 
+    // instantiate it lazily. So we need to check here instead.
+    if (ident == stacks.exec->propertyNames().arguments)
+        return;
+
     stacks.varStack.append(this); 
 }
 
@@ -1844,18 +1850,19 @@ JSValue* VarDeclNode::evaluate(ExecState* exec)
 
 void VarDeclNode::processDeclaration(ExecState* exec)
 {
-  JSObject* variable = exec->context()->variableObject();
+  Context* context = exec->context();
+  JSObject* variable = context->variableObject();
 
-  // If a variable by this name already exists, don't clobber it -
-  // it might be a function parameter
-  if (!variable->hasProperty(exec, ident)) {
-    int flags = Internal;
-    if (exec->context()->codeType() != EvalCode)
-      flags |= DontDelete;
-    if (varType == VarDeclNode::Constant)
-      flags |= ReadOnly;
-    variable->put(exec, ident, jsUndefined(), flags);
-  }
+  if (context->codeType() != FunctionCode)
+    if (variable->hasProperty(exec, ident))
+        return;
+
+  int flags = Internal;
+  if (context->codeType() != EvalCode)
+    flags |= DontDelete;
+  if (varType == VarDeclNode::Constant)
+    flags |= ReadOnly;
+  variable->put(exec, ident, jsUndefined(), flags);
 }
 
 // ------------------------------ VarDeclListNode ------------------------------
@@ -1876,7 +1883,7 @@ void VarDeclListNode::getDeclarations(DeclarationStacks& stacks)
         ASSERT(next->mayHaveDeclarations());
         stacks.nodeStack.append(next.get()); 
     }
-    stacks.varStack.append(var.get()); 
+    stacks.nodeStack.append(var.get());
 }
 
 void VarDeclListNode::breakCycle() 
@@ -2595,14 +2602,14 @@ FunctionBodyNode::FunctionBodyNode(SourceElementsNode *s)
   setLoc(-1, -1);
 }
 
-void FunctionBodyNode::initializeDeclarationStacks()
+void FunctionBodyNode::initializeDeclarationStacks(ExecState* exec)
 {
     Node* node = source.get();
     if (!node)
         return;
 
     DeclarationStacks::NodeStack nodeStack;
-    DeclarationStacks stacks(nodeStack, m_varStack, m_functionStack);
+    DeclarationStacks stacks(exec, nodeStack, m_varStack, m_functionStack);
     
     while (true) {
         ASSERT(node->mayHaveDeclarations()); // Otherwise, we wasted time putting an irrelevant node on the stack.
@@ -2622,15 +2629,22 @@ void FunctionBodyNode::initializeDeclarationStacks()
 void FunctionBodyNode::processDeclarations(ExecState* exec)
 {
     if (!m_initializedDeclarationStacks)
-        initializeDeclarationStacks();
+        initializeDeclarationStacks(exec);
 
     size_t i, size;
 
-    for (i = 0, size = m_functionStack.size(); i < size; ++i)
-        m_functionStack[i]->processDeclaration(exec);
+    JSObject* variableObject = exec->context()->variableObject();
+    const List& args = *exec->context()->arguments();
 
+    // The order of additions to the variable object here implicitly enforces the mutual exclusion described in ECMA 10.1.3.
     for (i = 0, size = m_varStack.size(); i < size; ++i)
         m_varStack[i]->processDeclaration(exec);
+
+    for (i = 0, size = m_parameters.size(); i < size; ++i)
+        variableObject->put(exec, m_parameters[i], args[i], DontDelete);
+
+    for (i = 0, size = m_functionStack.size(); i < size; ++i)
+        m_functionStack[i]->processDeclaration(exec);
 }
 
 void FunctionBodyNode::addParam(const Identifier& ident)
index 4a74ecc1f5f6eac6dc0134581e71d25b1aa8c045..f4db38e366378348304c8d67383b907f466e3d11 100644 (file)
@@ -88,13 +88,15 @@ namespace KJS {
       typedef Vector<VarDeclNode*, 16> VarStack;
       typedef Vector<FuncDeclNode*, 16> FunctionStack;
       
-      DeclarationStacks(NodeStack& n, VarStack& v, FunctionStack& f)
-        : nodeStack(n)
+      DeclarationStacks(ExecState* e, NodeStack& n, VarStack& v, FunctionStack& f)
+        : exec(e)
+        , nodeStack(n)
         , varStack(v)
         , functionStack(f)
       {
       }
-        
+
+      ExecState* exec;
       NodeStack& nodeStack;
       VarStack& varStack; 
       FunctionStack& functionStack;
@@ -1243,12 +1245,13 @@ namespace KJS {
   private:
     UString m_sourceURL;
     int m_sourceId;
-    Vector<Identifier> m_parameters;
 
-    void initializeDeclarationStacks();
+    void initializeDeclarationStacks(ExecState*);
     bool m_initializedDeclarationStacks;
+
     DeclarationStacks::VarStack m_varStack;
     DeclarationStacks::FunctionStack m_functionStack;
+    Vector<Identifier> m_parameters;
   };
 
   class FuncExprNode : public Node {
index a3af7335fbf4dd0d39076ef16d3fbefb449376d0..2125135804cc9766e76bc5cb9b7328ebeca91bbb 100644 (file)
@@ -1,3 +1,13 @@
+2007-10-25  Geoffrey Garen  <ggaren@apple.com>
+
+        Layout tests for bugs that might result from changes like 
+        http://bugs.webkit.org/show_bug.cgi?id=15683
+
+        * fast/js/vardecl-preserve-parameters-expected.txt: Added.
+        * fast/js/vardecl-preserve-parameters.html: Added.
+        * fast/js/vardecl-preserve-vardecl-expected.txt: Added.
+        * fast/js/vardecl-preserve-vardecl.html: Added.
+
 2007-10-24  Justin Garcia  <justin.garcia@apple.com>
 
         Reviewed by Darin.
diff --git a/LayoutTests/fast/js/vardecl-preserve-parameters-expected.txt b/LayoutTests/fast/js/vardecl-preserve-parameters-expected.txt
new file mode 100644 (file)
index 0000000..83ab81d
--- /dev/null
@@ -0,0 +1,4 @@
+This test verifies var declarations inside a function don't stomp function arguments.
+
+PASS: x should be 1 and is.
+
diff --git a/LayoutTests/fast/js/vardecl-preserve-parameters.html b/LayoutTests/fast/js/vardecl-preserve-parameters.html
new file mode 100644 (file)
index 0000000..672999d
--- /dev/null
@@ -0,0 +1,23 @@
+<p>This test verifies var declarations inside a function don't stomp function arguments.</p>
+
+<pre id="console"></pre>
+
+<script>
+if (window.layoutTestController)
+    layoutTestController.dumpAsText();
+
+function log(s)
+{
+    document.getElementById("console").appendChild(document.createTextNode(s + "\n"));
+}
+
+function f(x)
+{
+    var x;
+    if (x == 1)
+        log("PASS: x should be 1 and is.");
+    else
+        log("FAIL: x should be 1 but instead is " + x + ".");
+}
+f(1);
+</script>
diff --git a/LayoutTests/fast/js/vardecl-preserve-vardecl-expected.txt b/LayoutTests/fast/js/vardecl-preserve-vardecl-expected.txt
new file mode 100644 (file)
index 0000000..399b74d
--- /dev/null
@@ -0,0 +1,5 @@
+This test verifies that var declarations in different evaluation units don't stomp each other.
+
+PASS: x should be 1 and is.
+PASS: y should be 1 and is.
+
diff --git a/LayoutTests/fast/js/vardecl-preserve-vardecl.html b/LayoutTests/fast/js/vardecl-preserve-vardecl.html
new file mode 100644 (file)
index 0000000..0a2d599
--- /dev/null
@@ -0,0 +1,32 @@
+<p>This test verifies that var declarations in different evaluation units don't stomp each other.</p>
+<pre id="console"></pre>
+
+<script>
+if (window.layoutTestController)
+    layoutTestController.dumpAsText();
+
+function log(s)
+{
+    document.getElementById("console").appendChild(document.createTextNode(s + "\n"));
+}
+</script>
+
+<script>
+var x = 1;
+y = 1;
+</script>
+
+<script>
+var x;
+var y;
+
+if (x == 1)
+    log("PASS: x should be 1 and is.");
+else
+    log("FAIL: x should be 1 but instead is " + x + ".");
+
+if (y == 1)
+    log("PASS: y should be 1 and is.");
+else
+    log("FAIL: y should be 1 but instead is " + y + ".");
+</script>