911578 - introduce self-hosting intrinsic IsPackedArray and use it for implementing optimized loops in array extras

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

Brought up in bug 902822 comment 15, and seems like a very good idea.

Comment 1

[Till Schneidereit [:till]]

Attachment: Introduce self-hosting intrinsic isPackedArray

Used in the following patch

Comment 2

[Till Schneidereit [:till]]

Attachment: Use self-hosting intrinsic isPackedArray to optimize loops in array extras

Changes the following Array extras to use fast-paths for packed arrays:

every
some
forEach
map
find
findIndex
indexOf
lastIndexOf
reduce
reduceRight

I only tested the performance of indexOf, which got about 33% faster.

js- and jittests are green, try-servering here: https://tbpl.mozilla.org/?tree=Try&rev=b5fdd551b52c

We have a few tests that fail if I change IsPackedArray to always return true, so I don't think we need new ones:

ecma_3/Array/15.5.4.8-01.js
js1_6/Array/regress-290592.js
js1_6/Array/regress-386030.js

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8340403 [details] [diff] [review]
Use self-hosting intrinsic isPackedArray to optimize loops in array extras

Review of attachment 8340403 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: js/src/builtin/Array.js
@@ +345,5 @@
>              ThrowError(JSMSG_EMPTY_ARRAY_REDUCE);
>          var kPresent = false;
> +        if (isPacked) {
> +            accumulator = O[k++];
> +            kPresent = true;

You can remove this line and move |var kPresent = false;| to the else branch below.

@@ +358,3 @@
>              }
> +            if (!kPresent)
> +              ThrowError(JSMSG_EMPTY_ARRAY_REDUCE);

Nit: indentation looks wrong.

@@ +423,5 @@
>              ThrowError(JSMSG_EMPTY_ARRAY_REDUCE);
>          var kPresent = false;
> +        if (isPacked) {
> +            accumulator = O[k--];
> +            kPresent = true;

Same here; can move kPresent to the else branch.

Comment 4

[Till Schneidereit [:till]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f86fd4fa60
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7b039ed2dbac

Thanks for the quick review!

Comment 5

[André Bargull [:anba]]

(In reply to Till Schneidereit [:till] from comment #2)
> We have a few tests that fail if I change IsPackedArray to always return
> true, so I don't think we need new ones:

Maybe you do want new test case. ;-)

js> [1,2,3].map(function(v, i, a) {if (i == 0) delete a[1]; return v * 2})
[2, NaN, 6]
js> [1,2,3].map(function(v, i, a) {if (i == 0) delete a[1]; return v * 2})
[2, , 6]

Expected: always return `[2, , 6]`

Comment 6

[Jan de Mooij [:jandem]]

(In reply to André Bargull from comment #5)
> Maybe you do want new test case. ;-)

Good catch. Sorry, I should have seen this..

Comment 7

[Till Schneidereit [:till]]

Argh, *I* should've seen this! André, thanks for catching it!

I've backed out all usages of IsPackedArray for loops that invoke callbacks. All told, only indexOf, lastIndexOf and the accumulator-finding loops in reduce and reduceRight contain the optimization now. :(

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9439760a6196

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d9f86fd4fa60
https://hg.mozilla.org/mozilla-central/rev/7b039ed2dbac
https://hg.mozilla.org/mozilla-central/rev/9439760a6196