1163091 - Handle unboxed arrays in jsarray.cpp fast paths

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Many array natives which are implemented in C++ have fast paths for handling dense elements in native objects.  It would be good to handle unboxed arrays here too, to avoid performance problems with these objects.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

Rolled up patch which I'll split up for review.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Rename EnsureDenseResult

The three valued result for ensureDenseElements and other NativeObject methods also applies to unboxed array methods and methods shared between both types of objects.  This patch renames EnsureDenseResult to DenseElementResult and moves it out of NativeObject so other classes can use it more easily.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: main patch

Main changes.  This adds methods which can operate on either boxed or unboxed methods (templatized on the JSValueType to better ensure that fast code is generated) and cleans up the array code to use them.  It also relaxes a restriction on unboxed arrays that their length had to be int32, as this was unnecessary, implemented incorrectly, and complicated some parts of the jsarray.cpp changes.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: fix crash

Fix an unrelated crash with unboxed arrays.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: fix SetArrayElement

As discussed on IRC, SetArrayElement is broken and sometimes defines elements, sometimes sets them and possibly invokes prototype setters.  This patch changes it so that it always sets the elements, which breaks a couple jit-tests and exposes bugs in callers that call SetArrayElement when they really need to be defining the element.  I'm not super sure about this patch since it is more consistent but does move us further away from spec compliance and could break websites.  An alternative would be writing this function in a way that preserves the current broken behavior, and then fixing both SetArrayElement and its callers together later on.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8603617 [details] [diff] [review]
fix SetArrayElement

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

::: js/src/jsarray.cpp
@@ +352,5 @@
>  SetArrayElement(JSContext* cx, HandleObject obj, double index, HandleValue v)
>  {
>      MOZ_ASSERT(index >= 0);
>  
> +    if (!ObjectMayHaveExtraIndexedProperties(obj) && index > uint32_t(-1)) {

...I think that was supposed to be |index < UINT32_MAX|, no?

I'm not sure why we're not just fixing this to do property-setting, adding a version that does property-defining correctly, then fixing the callers to whichever is correct.  It looks like about 19 places (including InitArrayElements callers) to me, nothing too crazy.  In any case, I doubt we can make *just* the change here and fix some things, regress others.

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 8603615 [details] [diff] [review]
fix crash

Hmm, I guess I was working with an old tree when writing this patch because the initial landing of bug 1161077 fixed this bug.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8603610 [details] [diff] [review]
Rename EnsureDenseResult

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

::: js/src/vm/NativeObject.h
@@ +315,5 @@
> +{
> +    DenseElement_Success,
> +    DenseElement_Failure,
> +    DenseElement_Incomplete
> +};

This is fine but personally I really like enum classes: better type safety, the enum values don't pollute the global namespace so you can remove the DenseElement_ prefix and they allow forward declaration.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8603612 [details] [diff] [review]
main patch

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

::: js/src/jsarray.cpp
@@ +773,5 @@
> +           obj->isIndexed() ||
> +           IsAnyTypedArray(obj) ||
> +           (obj->getClass()->mayResolve &&
> +            obj->getClass()->mayResolve(*obj->runtimeFromAnyThread()->commonNames,
> +                                        INT_TO_JSID(0), obj));

A class can have a resolve hook without a mayResolve hook; the mayResolve hook is optional.

You can do ClassMayResolveId(names, obj->getClass(), id, obj), it will handle this correctly and also has an AutoSuppressGCAnalysis to avoid false positives.

@@ +923,5 @@
>  
> +template <typename SeparatorOp, JSValueType Type>
> +static DenseElementResult
> +ArrayJoinDenseKernel(JSContext* cx, SeparatorOp sepOp, HandleObject obj, uint32_t length,
> +                     StringBuffer& sb, uint32_t *numProcessed)

Nit: uint32_t* numProcessed and a few times below.

@@ +2369,4 @@
>       * Another potential wrinkle: what if the enumeration is happening on an
>       * object which merely has |arr| on its prototype chain?  It turns out this
>       * case can't happen, because any dense array used as the prototype of
>       * another object is first slowified, for type inference's sake.

Hm really? The last sentence doesn't match the isDelegate check added below.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8603617 [details] [diff] [review]
fix SetArrayElement

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

Clearing, since Brian agrees with Jeff on just switching to comply with the spec, and I'm totally willing to review (or write) that longer patch...

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1410ca139039

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16484c271f15

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1410ca139039
https://hg.mozilla.org/mozilla-central/rev/16484c271f15

Comment 14

[Hannes Verschore [:h4writer]]

A regression/improvement was reported on AWFY:
- slave: Mac OS X 10.10 32-bit (Mac Pro, shell)
- mode: Ion

Regression(s)/Improvement(s):
- kraken: 1.42% (regression)
- kraken: crypto-aes: 3.69% (regression)
- kraken: crypto-ccm: 5.47% (regression)
- kraken: crypto-pbkdf2: 4.86% (regression)
- kraken: crypto-sha256-iterative: 7.17% (regression)
- octane: PdfJS: -7.35% (regression)
- dart: DeltaBlue: 7.3% (regression)

Recorded range:
- http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3b142e9bc1f5&tochange=48fa8f0a227c

More details: http://arewefastyet.com/regressions/#/regression/1594367

(This regression was also noticed on 64bit, firefox os, win 8 and mac browser. I can give the links too, but this 32bit detection has all subbenchmarks that regressed.)

Comment 15

[Hannes Verschore [:h4writer]]

A regression/improvement was reported on AWFY:
- slave: Mac OS X 10.10 32-bit (Mac Pro, shell)
- mode: Ion

Regression(s)/Improvement(s):
- ss: format-xparb: -3.77% (improvement)
- kraken: crypto-aes: -1.75% (improvement)

Recorded range:
- http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a5f31bacc839&tochange=70a376c0f23d

More details: http://arewefastyet.com/regressions/#/regression/1674239

Reduces the regression of kraken-crypto-aes from 2ms to 1ms