Bug 19269 - speed up SunSpider by eliminating the toObject call for most get/put/delete
: speed up SunSpider by eliminating the toObject call for most get/put/delete
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2008-05-27 09:29 PST by
Modified: 2008-06-05 09:33 PST (History)


Attachments
patch (399.51 KB, patch)
2008-05-27 12:23 PST, Darin Adler
no flags Review Patch | Details | Formatted Diff | Diff
patch (399.37 KB, patch)
2008-05-27 12:27 PST, Darin Adler
ggaren: review+
Review Patch | Details | Formatted Diff | Diff
earlier try -- this made things slower because I went too far in some way (474.95 KB, patch)
2008-05-27 12:28 PST, Darin Adler
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-05-27 09:29:52 PST
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.
------- Comment #1 From 2008-05-27 12:23:27 PST -------
Created an attachment (id=21365) [details]
patch
------- Comment #2 From 2008-05-27 12:24:02 PST -------
(From update of attachment 21365 [details])
Oops, broken!
------- Comment #3 From 2008-05-27 12:27:38 PST -------
Created an attachment (id=21366) [details]
patch
------- Comment #4 From 2008-05-27 12:28:12 PST -------
Created an attachment (id=21367) [details]
earlier try -- this made things slower because I went too far in some way
------- Comment #5 From 2008-05-27 12:29:13 PST -------
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?
------- Comment #6 From 2008-05-28 09:12:46 PST -------
Note. This patch will also resolve bug 19286.
------- Comment #7 From 2008-06-02 21:30:28 PST -------
(From update of attachment 21366 [details])
+          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
------- Comment #8 From 2008-06-03 09:33:28 PST -------
(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.
------- Comment #9 From 2008-06-05 09:33:46 PST -------
http://trac.webkit.org/changeset/34355