Bug 43742 - REGRESSION(r58469): Math.pow() always returns double-backed JSValue which is extremely slow as array subscript
Summary: REGRESSION(r58469): Math.pow() always returns double-backed JSValue which is ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Nobody
URL:
Keywords: Performance
Depends on:
Blocks:
 
Reported: 2010-08-09 13:12 PDT by Andreas Kling
Modified: 2010-08-18 22:57 PDT (History)
9 users (show)

See Also:


Attachments
Proposed patch (1.43 KB, patch)
2010-08-09 13:17 PDT, Andreas Kling
oliver: review-
Details | Formatted Diff | Diff
Proposed patch v2 (3.21 KB, patch)
2010-08-09 14:43 PDT, Andreas Kling
kling: review-
Details | Formatted Diff | Diff
Proposed patch v3 (2.38 KB, patch)
2010-08-18 14:56 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2010-08-09 13:12:55 PDT
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.
Comment 1 Andreas Kling 2010-08-09 13:17:00 PDT
Created attachment 63921 [details]
Proposed patch
Comment 2 Oliver Hunt 2010-08-09 13:22:52 PDT
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
Comment 3 Andreas Kling 2010-08-09 13:31:47 PDT
(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.
Comment 4 Andreas Kling 2010-08-09 14:43:48 PDT
Created attachment 63930 [details]
Proposed patch v2
Comment 5 Oliver Hunt 2010-08-09 14:49:23 PDT
Comment on attachment 63930 [details]
Proposed patch v2

Have you tested sunspider with this?
Comment 6 WebKit Review Bot 2010-08-09 15:59:31 PDT
Attachment 63930 [details] did not build on win:
Build output: http://queues.webkit.org/results/3704005
Comment 7 Darin Adler 2010-08-09 16:36:41 PDT
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.
Comment 8 Eric Seidel (no email) 2010-08-09 19:54:02 PDT
Attachment 63930 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3748006
Comment 9 Andreas Kling 2010-08-13 12:55:45 PDT
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 :-)
Comment 10 Andreas Kling 2010-08-15 20:04:07 PDT
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 ;-))
Comment 11 Geoffrey Garen 2010-08-18 14:56:44 PDT
> 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.
Comment 12 Andreas Kling 2010-08-18 14:56:59 PDT
Created attachment 64777 [details]
Proposed patch v3

New approach - add codegen for pow() to return Int32 values when possible.
Comment 13 Andreas Kling 2010-08-18 15:07:34 PDT
(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.
Comment 14 Geoffrey Garen 2010-08-18 20:41:31 PDT
Comment on attachment 64777 [details]
Proposed patch v3

r=me
Comment 15 WebKit Commit Bot 2010-08-18 22:57:03 PDT
Comment on attachment 64777 [details]
Proposed patch v3

Clearing flags on attachment: 64777

Committed r65653: <http://trac.webkit.org/changeset/65653>
Comment 16 WebKit Commit Bot 2010-08-18 22:57:11 PDT
All reviewed patches have been landed.  Closing bug.