Closed Bug 911578 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: till, Assigned: till)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(2 files)

Brought up in bug 902822 comment 15, and seems like a very good idea.
Used in the following patch
Attachment #8340399 - Flags: review?(jdemooij)
Assignee: general → till
Status: NEW → ASSIGNED
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
Attachment #8340403 - Flags: review?(jdemooij)
Attachment #8340399 - Flags: review?(jdemooij) → review+
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.
Attachment #8340403 - Flags: review?(jdemooij) → review+
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!
Summary: introduce self-hosting intrinsic IsDenseArray and use it for implementing optimized loops in array extras → introduce self-hosting intrinsic IsPackedArray and use it for implementing optimized loops in array extras
(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]`
(In reply to André Bargull from comment #5)
> Maybe you do want new test case. ;-)

Good catch. Sorry, I should have seen this..
Flags: needinfo?(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
Flags: needinfo?(till)
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.