[webkit-reviews] review granted: [Bug 19269] speed up SunSpider by eliminating the toObject call for most get/put/delete : [Attachment 21366] patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 19269: speed up SunSpider by eliminating the toObject call for most
get/put/delete
http://bugs.webkit.org/show_bug.cgi?id=19269

Attachment 21366: patch
http://bugs.webkit.org/attachment.cgi?id=21366&action=edit

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
+	   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