[Webkit-unassigned] [Bug 15879] Add evaluateToNumber fast path for numeric operations

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=15879





------- Comment #6 from eric at webkit.org  2007-11-07 10:08 PDT -------
(In reply to comment #5)
> (From update of attachment 17106 [edit])
> Seems clear that ImmediateNumberNode should store both forms, not just the
> JSValue.

Yeah.  I'll try that.

> 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.

I was skeptical if a function declared in the cpp file would correctly always
inline, but given that the header is simply included directly in the file (and
processed by the preprocessor) it shouldn't matter to the compiler where the
function is, so long as it's marked ALWAYS_INLINE :)

> +    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.

Hum... I need to go back and find that case where SunSpider seemed to show
removing an else as being a slowdown, and stare at both sets of assembly to
prove to myself once and for-all that the generated code is identical.  I'll
definitely remove this one when I abstract that into an inline.

> +    i2 = right->evaluate(exec)->toInt32(exec);
> 
> Obviously this should be using evaluateToNumber, unless you're planning on
> adding evaluateToInt32.

As I mentioned in my above comment, we don't yet have any way to convert a
double to a UInt via our fancy JSImmediate functions, I could just cast it...
assuming static_cast<uint32_t>(right->evaluateToNumber(exec)) the equivilent to
toInt32().  I'll check.

> 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.

Yeah, I like that idea.  But that's for a separate bug.  Filed as bug 15885.

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

I'm going to remove some of the copy-paste code and investigate the
toInt32()/static_cast<uint32_t>(evaluateToNumber()) change and re-post.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.