[webkit-reviews] review granted: [Bug 15879] Add evaluateToNumber fast path for numeric operations : [Attachment 17106] Improved patch, 0.7% improvement for Sunspider

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 15879: Add evaluateToNumber fast path for numeric operations
http://bugs.webkit.org/show_bug.cgi?id=15879

Attachment 17106: Improved patch, 0.7% improvement for Sunspider
http://bugs.webkit.org/attachment.cgi?id=17106&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Seems clear that ImmediateNumberNode should store both forms, not just the
JSValue.

And yes, it's entirely practical to use an inline for cases like
BracketAccessorNode::evaluateToNumber, since it's really identical to the
normal evaluate followed by toNumber, and for MultNode::evaluate, which is
identical to evaluateToNumber followed by jsNumber. ALWAYS_INLINE will work for
such cases.

+    if (bothTypes == ((NumberType << 3) | NumberType))
+	 return v1->toNumber(exec) + v2->toNumber(exec);
+    else if (bothTypes == ((StringType << 3) | StringType)) {

Please don't use else after return.

+    i2 = right->evaluate(exec)->toInt32(exec);

Obviously this should be using evaluateToNumber, unless you're planning on
adding evaluateToInt32.

My general comment is that this looks good as-is.

My only complaint is that I think that both evaluate and evaluateToNumber
should be moved down out of Node to some other class, perhaps named
ExpressionNode. It's really unnecessary to have those functions in classes like
StatementNode and I think eventually we could get some optimizations by not
having them in all nodes. For example, in some nodes they don't need to be
virtual functions.

r=me, as-is -- this is good the way it is.