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.
Created attachment 63921 [details] Proposed patch
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
(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.
Created attachment 63930 [details] Proposed patch v2
Comment on attachment 63930 [details] Proposed patch v2 Have you tested sunspider with this?
Attachment 63930 [details] did not build on win: Build output: http://queues.webkit.org/results/3704005
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.
Attachment 63930 [details] did not build on mac: Build output: http://queues.webkit.org/results/3748006
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 on attachment 63930 [details] Proposed patch v2 r-'ing this to avoid confusion as to what I'm soliciting right now (comments, not reviews ;-))
> 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.
Created attachment 64777 [details] Proposed patch v3 New approach - add codegen for pow() to return Int32 values when possible.
(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 on attachment 64777 [details] Proposed patch v3 r=me
Comment on attachment 64777 [details] Proposed patch v3 Clearing flags on attachment: 64777 Committed r65653: <http://trac.webkit.org/changeset/65653>
All reviewed patches have been landed. Closing bug.