Handle unboxed arrays in jsarray.cpp fast paths

RESOLVED FIXED in Firefox 41

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

(Blocks: 1 bug)

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected, firefox41 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8603608 [details] [diff] [review]
patch

Rolled up patch which I'll split up for review.
Assignee: nobody → bhackett1024
(Assignee)

Comment 2

3 years ago
Created attachment 8603610 [details] [diff] [review]
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.
Attachment #8603610 - Flags: review?(jdemooij)
(Assignee)

Comment 3

3 years ago
Created attachment 8603612 [details] [diff] [review]
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.
Attachment #8603612 - Flags: review?(jdemooij)
(Assignee)

Comment 4

3 years ago
Created attachment 8603615 [details] [diff] [review]
fix crash

Fix an unrelated crash with unboxed arrays.
Attachment #8603615 - Flags: review?(jdemooij)
(Assignee)

Comment 5

3 years ago
Created attachment 8603617 [details] [diff] [review]
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.
Attachment #8603617 - Flags: review?(jorendorff)

Comment 6

3 years ago
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.
(Assignee)

Comment 7

3 years ago
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)
Depends on: 1166678
https://hg.mozilla.org/mozilla-central/rev/1410ca139039
https://hg.mozilla.org/mozilla-central/rev/16484c271f15
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
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

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.)
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
Depends on: 1170355
Depends on: 1206265
You need to log in before you can comment on or make changes to this bug.