Bug 43742 – REGRESSION(r58469): Math.pow() always returns double-backed JSValue which is extremely slow as array subscript

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andreas Kling]

JSC currently chokes on this (rather neat) plasma demo:
http://www.bel.fi/~alankila/plasma.html

What happens is they store a precalc'd square roots in a huge array and look them up instead of calling Math.sqrt(). All fine, except for array lookups with double subscripts we currently convert the subscript to a string and look that up as a property.

We can detect this case and implement a fast-path for double subscripts that are 0 <= value <= UINT_MAX.

[Andreas Kling]

Created attachment 63921 [details]
Proposed patch

[Oliver Hunt]

Comment on attachment 63921 [details]
Proposed patch

This is incorrect. 1.5 is a valid property, eg.
a=[];
a[1.5] = "Foo"

a["1.5"] === "Foo"; // true
a[1] === undefined; // true

Your patch results in a look up of 1.5 converting to a lookup of 1, which is incorrect.  It's somewhat worrying that this passes tests :-(

Is the double that's being looked up actually an integral value?

--Oliver

[Andreas Kling]

(In reply to comment #2)
> Your patch results in a look up of 1.5 converting to a lookup of 1, which is incorrect.  It's somewhat worrying that this passes tests :-(

Indeed, I expected this thing to be well covered, and since no tests failed, I figured..... ;-)

> Is the double that's being looked up actually an integral value?

In this case, yes.

[Andreas Kling]

Created attachment 63930 [details]
Proposed patch v2

[Oliver Hunt]

Comment on attachment 63930 [details]
Proposed patch v2

Have you tested sunspider with this?

[WebKit Review Bot]

Attachment 63930 [details] did not build on win:
Build output: http://queues.webkit.org/results/3704005

[Darin Adler]

Comment on attachment 63930 [details]
Proposed patch v2

> +        } else if (subscript.isDouble()) {
> +            i = subscript.toUInt32(callFrame);
> +            haveIntegralIndex = static_cast<double>(i) == subscript.asDouble();
> +        }

Once you have a "true" for isDouble, toUInt32 is an unnecessarily slow way to convert it to an integer. This is more like what you want:

    } else if (subscript.isDouble()) {
        i = static_cast<uint32_t>(subscript.asDouble());
        haveIntegralIndex = static_cast<double>(i) == subscript.asDouble();
    }

But I suspect this will do the wrong thing for negative zero. Please test that case. We need to add some test cases here if there are not existing tests covering these.

[Eric Seidel (no email)]

Attachment 63930 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3748006

[Andreas Kling]

Oliver mentioned on IRC that this might be a performance regression, so I did a little detective work.

Turns out it did regress severely with http://trac.webkit.org/changeset/58469

With this change, pow() always returns JSValues that are doubles. Previously it would go through the JSValue constructor that stores integral double values as int32s.

I am unsure what the best fix is, move forward with the patch I've posted here, modify the JIT's storeDouble() to emulate JSValue(double), or something else entirely. Please advise :-)

[Andreas Kling]

Comment on attachment 63930 [details]
Proposed patch v2

r-'ing this to avoid confusion as to what I'm soliciting right now (comments, not reviews ;-))

[Geoffrey Garen]

> I am unsure what the best fix is, move forward with the patch I've posted here, modify the JIT's storeDouble() to emulate JSValue(double), or something else entirely. Please advise :-)

I don't think you should modify storeDouble. storeDouble is used after an operation on doubles -- something that's unlikely to produce an int.

I think the two options are:

1. Add code to the specialized assembly math functions to try to convert to int if possible.
2. Move forward with a faster case for double indexing into an array.

I'd say try #1 and see if it's a performance regression. If not, it's pure win.

#2 also seems reasonable, but it won't pay off as much as #1 would, since it still involves a call out to a helper function.

[Andreas Kling]

Created attachment 64777 [details]
Proposed patch v3

New approach - add codegen for pow() to return Int32 values when possible.

[Andreas Kling]

(In reply to comment #12)
> Created an attachment (id=64777) [details]
> Proposed patch v3
> 
> New approach - add codegen for pow() to return Int32 values when possible.

Callgrind says: SunSpider is 0.000139% slower with my change.

But comparing http://www.bel.fi/~alankila/plasma.html before/after is night/day.

[Geoffrey Garen]

Comment on attachment 64777 [details]
Proposed patch v3

r=me

[WebKit Commit Bot]

Comment on attachment 64777 [details]
Proposed patch v3

Clearing flags on attachment: 64777

Committed r65653: <http://trac.webkit.org/changeset/65653>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.