Bug 19269 – speed up SunSpider by eliminating the toObject call for most get/put/delete

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Darin Adler]

I have a design that combines the getOwnPropertySlot virtual function call with the toObject function. It's going to give us a 1% or 2% speedup.

[Darin Adler]

Created attachment 21365 [details]
patch

[Darin Adler]

Comment on attachment 21365 [details]
patch

Oops, broken!

[Darin Adler]

Created attachment 21366 [details]
patch

[Darin Adler]

Created attachment 21367 [details]
earlier try -- this made things slower because I went too far in some way

[Darin Adler]

Removes most of the legacy code (not all) and leaves things 1.025x as fast. I'm excited about getting this in, but maybe not until after the Acid3 effort is done?

[Darin Adler]

Note. This patch will also resolve bug 19286.

[Geoffrey Garen]

Comment on attachment 21366 [details]
patch

+          speed up SunSpider by eliminating the toObject call for most get/put/delete

You ended up leaving out delete.

+        The getOwnPropertySlot virtual function now takes care of the toObject call
+        for get. Similarly, the put and deleteProperty functions do the same for those
+        operations. To do this, the virtual functions were moved from the JSObject class

deleteProperty? o'rly?

+        * kjs/lexer.cpp:
+        (KJS::Lexer::shift): Changed shift to just do a single character, to unroll
+        the loop and especially to make the one character case faster.
+        (KJS::Lexer::setCode): Call shift multiple times instead of passing a number.
+        (KJS::Lexer::lex): Ditto.
+        (KJS::Lexer::matchPunctuator): Ditto. Also removed unneeded elses after returns.
+        (KJS::Lexer::scanRegExp): Ditto.
+        * kjs/lexer.h: Removed the count argument from shift.

Separate patch for this stuff, please.

+        (KJS::JSCell::getOwnPropertySlot): Added. Calls toObject and then sets the slot.
+        This function has to always return true, because the caller can't walk the prototype
+        chain. Because of that, we do a getPropertySlot, not getOwnPropertySlot, which works
+        for the caller. This is private, only called by getOwnPropertySlotInternal.

I think you meant something other than "getOwnPropertySlotInternal."

Index: JavaScriptCore/kjs/grammar.y
===================================================================
--- JavaScriptCore/kjs/grammar.y	(revision 34151)
+++ JavaScriptCore/kjs/grammar.y	(working copy)
@@ -1094,34 +1094,11 @@ SourceElement:
 
 static AddNode* makeAddNode(ExpressionNode* left, ExpressionNode* right)
 {
-    JSType t1 = left->expectedReturnType();
-    JSType t2 = right->expectedReturnType();
-
-    if (t1 == NumberType && t2 == NumberType)
-        return new AddNumbersNode(left, right);
-    if (t1 == StringType && t2 == StringType)
-        return new AddStringsNode(left, right);
-    if (t1 == StringType)
-        return new AddStringLeftNode(left, right);
-    if (t2 == StringType)
-        return new AddStringRightNode(left, right);
     return new AddNode(left, right);
 }
 
 static LessNode* makeLessNode(ExpressionNode* left, ExpressionNode* right)
 {
-    JSType t1 = left->expectedReturnType();
-    JSType t2 = right->expectedReturnType();
-    
-    if (t1 == StringType && t2 == StringType)
-        return new LessStringsNode(left, right);
-
-    // There are certainly more efficient ways to do this type check if necessary
-    if (t1 == NumberType || t1 == BooleanType || t1 == UndefinedType || t1 == NullType ||
-        t2 == NumberType || t2 == BooleanType || t2 == UndefinedType || t2 == NullType)
-        return new LessNumbersNode(left, right);
-
-    // Neither is certain to be a number, nor were both certain to be strings, so we use the default (slow) implementation.
     return new LessNode(left, right);
 }

Is this change related?
 
Index: JavaScriptCore/kjs/value.cpp
===================================================================
--- JavaScriptCore/kjs/value.cpp	(revision 34151)
+++ JavaScriptCore/kjs/value.cpp	(working copy)
@@ -217,6 +217,59 @@ ConstructType JSCell::getConstructData(C
     return ConstructTypeNone;
 }
 
+bool JSCell::getOwnPropertySlot(ExecState* exec, const Identifier& identifier, PropertySlot& slot)
+{
+    // This is not a general purpose implementation of getOwnPropertySlot.
+    // It should only be called by JSValue::get.
+    // It calls getPropertySlot, not getOwnPropertySlot.
+    JSObject* object = toObject(exec);
+    if (UNLIKELY(exec->hadException())) {
+        slot.setUndefined();
+        return true;
+    }
+    slot.setBase(object);
+    if (!object->getPropertySlot(exec, identifier, slot))
+        slot.setUndefined();
+    return true;
+}
+
+bool JSCell::getOwnPropertySlot(ExecState* exec, unsigned identifier, PropertySlot& slot)
+{
+    // This is not a general purpose implementation of getOwnPropertySlot.
+    // It should only be called by JSValue::get.
+    // It calls getPropertySlot, not getOwnPropertySlot.
+    JSObject* object = toObject(exec);
+    if (UNLIKELY(exec->hadException())) {
+        slot.setUndefined();
+        return true;
+    }
+    slot.setBase(object);
+    if (!object->getPropertySlot(exec, identifier, slot))
+        slot.setUndefined();
+    return true;
+}
+
+void JSCell::put(ExecState* exec, const Identifier& identifier, JSValue* value)
+{
+    JSObject* object = toObject(exec);
+    if (UNLIKELY(exec->hadException()))
+        return;
+    object->put(exec, identifier, value);
+}
+
+void JSCell::put(ExecState* exec, unsigned identifier, JSValue* value)
+{
+    JSObject* object = toObject(exec);
+    if (UNLIKELY(exec->hadException()))
+        return;
+    object->put(exec, identifier, value);
+}

In the cases above, you should just ASSERT(!exec->hadException()), for a small
extra speedup. toObject can only throw for undefined and null, and since we're
a JSCell, we're not undefined or null.

Really, it's not right for ToNumber, ToBoolean, ToString, and ToObject to be
virtual functions. It gives the impression that subclasses can arbitrarily
override them, but they can't / don't / shouldn't.

Man, this "JSCell::getOwnPropertySlot always returns true" stuff is pretty subtle.
But I can't say no to the numbers!

r=me

[Darin Adler]

(In reply to comment #7)
> You ended up leaving out delete.

Will fix. I had another version of this patch that did the delete thing.

> deleteProperty? o'rly?

Ditto.

> +        * kjs/lexer.cpp:
> 
> Separate patch for this stuff, please.

Will do.

> +        (KJS::JSCell::getOwnPropertySlot): Added. Calls toObject and then sets
> the slot.
> +        This function has to always return true, because the caller can't walk
> the prototype
> +        chain. Because of that, we do a getPropertySlot, not
> getOwnPropertySlot, which works
> +        for the caller. This is private, only called by
> getOwnPropertySlotInternal.
> 
> I think you meant something other than "getOwnPropertySlotInternal."

I had another version of this patch where I made these virtual functions private and called only by a function named getOwnPropertySlotInternal. I'll try to redo that later and see if I can preserve the speedup.

>  static AddNode* makeAddNode(ExpressionNode* left, ExpressionNode* right)
>  {
> -    JSType t1 = left->expectedReturnType();
> -    JSType t2 = right->expectedReturnType();
> -
> -    if (t1 == NumberType && t2 == NumberType)
> -        return new AddNumbersNode(left, right);
> -    if (t1 == StringType && t2 == StringType)
> -        return new AddStringsNode(left, right);
> -    if (t1 == StringType)
> -        return new AddStringLeftNode(left, right);
> -    if (t2 == StringType)
> -        return new AddStringRightNode(left, right);
>      return new AddNode(left, right);
>  }
> 
>  static LessNode* makeLessNode(ExpressionNode* left, ExpressionNode* right)
>  {
> -    JSType t1 = left->expectedReturnType();
> -    JSType t2 = right->expectedReturnType();
> -    
> -    if (t1 == StringType && t2 == StringType)
> -        return new LessStringsNode(left, right);
> -
> -    // There are certainly more efficient ways to do this type check if
> necessary
> -    if (t1 == NumberType || t1 == BooleanType || t1 == UndefinedType || t1 ==
> NullType ||
> -        t2 == NumberType || t2 == BooleanType || t2 == UndefinedType || t2 ==
> NullType)
> -        return new LessNumbersNode(left, right);
> -
> -    // Neither is certain to be a number, nor were both certain to be strings,
> so we use the default (slow) implementation.
>      return new LessNode(left, right);
>  }
> 
> Is this change related?

Yes, it's part of removing the "old interpreter state" code.

> In the cases above, you should just ASSERT(!exec->hadException())

Will do.

[Darin Adler]

http://trac.webkit.org/changeset/34355