Reviewed by Darin.
authormjs <mjs@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Jul 2007 07:10:35 +0000 (07:10 +0000)
committermjs <mjs@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Jul 2007 07:10:35 +0000 (07:10 +0000)
        -fixed <rdar://problem/5353293> REGRESSION (r24287): 1% i-Bench JS slowdown from JavaScript compatibility fix (14719)
        http://bugs.webkit.org/show_bug.cgi?id=14719

        My fix for this actually resulted in JS iBench being 1% faster than before the regression
        and the Celtic Kane benchmark being 5% faster than before the regression.

        * kjs/nodes.cpp:
        (VarDeclNode::handleSlowCase): factored out the slow code path to be out of line.
        (VarDeclNode::evaluate): I did a couple of things:
        (1) Don't check if the variable is already declared by looking for the property in
        the variable object, that code path was dead code.
        (2) Special-case the common case where the top of the scope and the variable object
        are the same; in that case the variable must always be in the variable object.
        (3) Don't return a jsString() of the variable name, nothing uses the return value
        from this node types evaluate method.
        * kjs/nodes.h:

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

JavaScriptCore/ChangeLog
JavaScriptCore/kjs/nodes.cpp
JavaScriptCore/kjs/nodes.h

index 7a5718b..e60d5c7 100644 (file)
@@ -1,3 +1,24 @@
+2007-07-22  Maciej Stachowiak  <mjs@apple.com>
+
+        Reviewed by Darin.
+
+        -fixed <rdar://problem/5353293> REGRESSION (r24287): 1% i-Bench JS slowdown from JavaScript compatibility fix (14719)
+        http://bugs.webkit.org/show_bug.cgi?id=14719
+        
+        My fix for this actually resulted in JS iBench being 1% faster than before the regression
+        and the Celtic Kane benchmark being 5% faster than before the regression.
+        
+        * kjs/nodes.cpp:
+        (VarDeclNode::handleSlowCase): factored out the slow code path to be out of line.
+        (VarDeclNode::evaluate): I did a couple of things:
+        (1) Don't check if the variable is already declared by looking for the property in
+        the variable object, that code path was dead code.
+        (2) Special-case the common case where the top of the scope and the variable object
+        are the same; in that case the variable must always be in the variable object.
+        (3) Don't return a jsString() of the variable name, nothing uses the return value
+        from this node types evaluate method.
+        * kjs/nodes.h:
+
 2007-07-22  Darin Adler  <darin@apple.com>
 
         Reviewed by Kevin Decker.
index 6b488dc..eef4dea 100644 (file)
@@ -1587,70 +1587,67 @@ VarDeclNode::VarDeclNode(const Identifier &id, AssignExprNode *in, Type t)
 {
 }
 
-// ECMA 12.2
-JSValue *VarDeclNode::evaluate(ExecState *exec)
+JSValue* VarDeclNode::handleSlowCase(ExecState* exec, const ScopeChain& chain, JSValue* val)
 {
-  JSObject* variable = exec->context()->variableObject();
-
-  JSValue* val;
-  if (init) {
-      val = init->evaluate(exec);
-      KJS_CHECKEXCEPTIONVALUE
-      
-      if (variable->getDirect(ident) || ident == exec->propertyNames().arguments) {
-          const ScopeChain& chain = exec->context()->scopeChain();
-          ScopeChainIterator iter = chain.begin();
-          ScopeChainIterator end = chain.end();        
-
-          // we must always have something in the scope chain
-          ASSERT(iter != end);
-
-          PropertySlot slot;
-          JSObject* base;
-
-          do {
-              base = *iter;
-              if (base->getPropertySlot(exec, ident, slot))
-                  break;
+    ScopeChainIterator iter = chain.begin();
+    ScopeChainIterator end = chain.end();        
+    
+    // we must always have something in the scope chain
+    ASSERT(iter != end);
+    
+    PropertySlot slot;
+    JSObject* base;
+    
+    do {
+        base = *iter;
+        if (base->getPropertySlot(exec, ident, slot))
+            break;
+        
+        ++iter;
+    } while (iter != end);
+    
+    unsigned flags = 0;
+    base->getPropertyAttributes(ident, flags);
+    if (varType == VarDeclNode::Constant)
+        flags |= ReadOnly;
+    
+    base->put(exec, ident, val, flags);
+    return 0;
+}
 
-             ++iter;
-          } while (iter != end);
+// ECMA 12.2
+JSValue* VarDeclNode::evaluate(ExecState* exec)
+{
+    JSValue* val;
+    if (init) {
+        val = init->evaluate(exec);
+        KJS_CHECKEXCEPTIONVALUE
+            
+        const ScopeChain& chain = exec->context()->scopeChain();
+        JSObject* variableObject = exec->context()->variableObject();
 
-          unsigned flags = 0;
-          base->getPropertyAttributes(ident, flags);
-          if (varType == VarDeclNode::Constant)
-              flags |= ReadOnly;
+        ASSERT(!chain.isEmpty());
+        ASSERT(variableObject->getDirect(ident) || ident == exec->propertyNames().arguments);
 
-          base->put(exec, ident, val, flags);
-          return jsString(ident.ustring());
-      }
-    } else {
-      // already declared? - check with getDirect so you can override
-      // built-in properties of the global object with var declarations.
-      // Also check for 'arguments' property. The 'arguments' cannot be found with
-      // getDirect, because it's created lazily by
-      // ActivationImp::getOwnPropertySlot.
-      // Since variable declarations are always in function scope, 'variable'
-      // will always contain instance of ActivationImp and ActivationImp will
-      // always have 'arguments' property
-      if (variable->getDirect(ident) || ident == exec->propertyNames().arguments)
-          return 0;
-      val = jsUndefined();
-  }
+        // if the variable object is the top of the scope chain, then that must
+        // be where this variable is declared, processVarDecls would have put 
+        // it there. Don't search the scope chain, to optimize this very common case.
+        if (chain.top() != variableObject)
+            return handleSlowCase(exec, chain, val);
 
-#ifdef KJS_VERBOSE
-  printInfo(exec,(UString("new variable ")+ident.ustring()).cstring().c_str(),val);
-#endif
-  // We use Internal to bypass all checks in derived objects, e.g. so that
-  // "var location" creates a dynamic property instead of activating window.location.
-  int flags = Internal;
-  if (exec->context()->codeType() != EvalCode)
-    flags |= DontDelete;
-  if (varType == VarDeclNode::Constant)
-    flags |= ReadOnly;
-  variable->put(exec, ident, val, flags);
-
-  return jsString(ident.ustring());
+        unsigned flags = 0;
+        variableObject->getPropertyAttributes(ident, flags);
+        if (varType == VarDeclNode::Constant)
+            flags |= ReadOnly;
+        
+        variableObject->put(exec, ident, val, flags);
+    } 
+
+    // no caller of this function actually uses the return value. 
+    // FIXME: It would be better to change the inheritence hierarchy so this
+    // node doesn't even have an evaluate method, but instead a differently named
+    // one with a void return.
+    return 0;
 }
 
 void VarDeclNode::processVarDecls(ExecState *exec)
index af2894e..45fbe5b 100644 (file)
 #define KJS_FAST_CALL
 #endif
 
+#if COMPILER(GCC)
+#define KJS_NO_INLINE __attribute__((noinline))
+#else
+#define KJS_NO_INLINE
+#endif
+
 namespace KJS {
 
   class ProgramNode;
@@ -783,7 +789,7 @@ namespace KJS {
     RefPtr<Node> expr;
   };
 
-  class VarDeclNode : public Node {
+  class VarDeclNode: public Node {
   public:
     enum Type { Variable, Constant };
     VarDeclNode(const Identifier &id, AssignExprNode *in, Type t) KJS_FAST_CALL;
@@ -791,6 +797,7 @@ namespace KJS {
     virtual void processVarDecls(ExecState*) KJS_FAST_CALL;
     virtual void streamTo(SourceStream&) const KJS_FAST_CALL;
   private:
+    JSValue* handleSlowCase(ExecState*, const ScopeChain&, JSValue*) KJS_FAST_CALL KJS_NO_INLINE;
     Type varType;
     Identifier ident;
     RefPtr<AssignExprNode> init;