Closed Bug 1163091 Opened 7 years ago Closed 7 years ago

Handle unboxed arrays in jsarray.cpp fast paths


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox40 --- affected
firefox41 --- fixed


(Reporter: bhackett1024, Assigned: bhackett1024)




(4 files, 1 obsolete file)

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.
Attached patch patchSplinter Review
Rolled up patch which I'll split up for review.
Assignee: nobody → bhackett1024
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.
Attachment #8603610 - Flags: review?(jdemooij)
Attached patch main patchSplinter Review
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.
Attachment #8603612 - Flags: review?(jdemooij)
Attached patch fix crash (obsolete) — Splinter Review
Fix an unrelated crash with unboxed arrays.
Attachment #8603615 - Flags: review?(jdemooij)
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.
Attachment #8603617 - Flags: review?(jorendorff)
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 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.
Attachment #8603615 - Attachment is obsolete: true
Attachment #8603615 - Flags: review?(jdemooij)
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.
Attachment #8603610 - Flags: review?(jdemooij) → review+
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.
Attachment #8603612 - Flags: review?(jdemooij) → review+
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...
Attachment #8603617 - Flags: review?(jorendorff)
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
A regression/improvement was reported on AWFY:
- slave: Mac OS X 10.10 32-bit (Mac Pro, shell)
- mode: Ion

- 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:

More details:

(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.)
A regression/improvement was reported on AWFY:
- slave: Mac OS X 10.10 32-bit (Mac Pro, shell)
- mode: Ion

- ss: format-xparb: -3.77% (improvement)
- kraken: crypto-aes: -1.75% (improvement)

Recorded range:

More details:

Reduces the regression of kraken-crypto-aes from 2ms to 1ms
You need to log in before you can comment on or make changes to this bug.