1170372 - Use unboxed arrays for Array() and other functions keyed to allocation sites

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Currently [] arrays can be unboxed, but not arrays created by calling the Array() method.  The attached patch fixes this, and also handles similar methods that create arrays keyed to allocation sites: Array.of, String.split, and the self hosted intrinsic NewDenseArray.  This also makes some related cleanups so that there is a fair amount of removed code.

 26 files changed, 447 insertions(+), 774 deletions(-)

I'll break this patch up for review.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: remove UnsafePutElements

There is a ton of complexity handling the UnsafePutElements intrinsic, and this function's existence puts some extra requirements on how we can convert unboxed arrays to natives.  This function was added for PJS but is still used in one place, by the Array.map intrinsic.  The attached patch removes UnsafePutElements and changes Array.map to use _DefineProperty instead, like Array.from does.  This also includes the changes to the NewDenseArray intrinsic, because hg diff intertwined the two.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: everything else

This patch expands the repertoire of functions that can create either boxed or unboxed arrays, adds uses of them in the above native functions, and updates the JITs accordingly.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #1)
> The attached patch removes
> UnsafePutElements and changes Array.map to use _DefineProperty instead

Won't this regress Array.map performance a lot? I think till did some work optimizing it a while ago...

Comment 4

[Till Schneidereit [:till]]

I tried to optimize it, but see bug 911578 comment 5.

In any case, it'd be interesting to see what the performance implications here would be. _DefineDataProperty is unfortunately really slow, so this is bound to drastically reduce Array#map's performance.

Comment 5

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> Won't this regress Array.map performance a lot? I think till did some work
> optimizing it a while ago...

I can write a patch to optimize _DefineProperty in Ion, though tbh that is what should have been done in the first place.

Comment 6

[Till Schneidereit [:till]]

(In reply to Brian Hackett (:bhackett) from comment #5)
> I can write a patch to optimize _DefineProperty in Ion, though tbh that is
> what should have been done in the first place.

That'd be quite excellent, as it'd allow us to self-host Object.defineProperty and enable escape analysis to eliminate object allocations in the common case where it's called with an object literal (or a nearby-allocated object). Making that work for all properties would of course be best, but just inlining _DefineDataProperty and falling back to a non-inlined intrinsic for accessors would still be very useful.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: inline _DefineDataProperty

Inline _DefineDataProperty for integer indexes on boxed or unboxed arrays.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: other fixes

More perf fixes to fix unboxed array performance with Array.map.  This optimizes JSOP_IN for unboxed array indexes, and fixes an issue where calling DefineProperty on an unboxed object would always convert it to native.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8613791 [details] [diff] [review]
remove UnsafePutElements

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

Nice.

::: js/src/vm/SelfHosting.cpp
@@ -316,5 @@
> -        return true;
> -
> -      case DenseElementResult::Incomplete: // shouldn't happen!
> -        MOZ_ASSERT(!"%EnsureDenseArrayElements() would yield sparse array");
> -        JS_ReportError(cx, "%EnsureDenseArrayElements() would yield sparse array");

Hm maybe we should keep a MOZ_ASSERT to ensure the array we return is not sparse?

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8613792 [details] [diff] [review]
everything else

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

r=me with comments below addressed.

::: js/src/jit/BaselineIC.cpp
@@ +9927,5 @@
>              count = args[0].toInt32();
> +
> +        if (count <= ArrayObject::EagerAllocationMaxLength) {
> +            res.set(NewFullyAllocatedArrayForCallingAllocationSite(cx, count, TenuredObject,
> +                                                                   /* forceAnalyze = */ true));

Why are we passing forceAnalyze = true here? Can you add a comment explaining that, here or somewhere else?

@@ -9935,5 @@
> -        res->setGroup(group);
> -        return true;
> -    }
> -
> -    if (native == intrinsic_NewDenseArray) {

Hm I was going to say this will break Ion inlining of NewDenseArray, but we don't inline it? I wonder if that changed at some point.

::: js/src/jsarray.cpp
@@ +249,5 @@
>      MOZ_ASSERT(index_ < length_);
> +    if (resObj_) {
> +        DenseElementResult result = SetOrExtendAnyBoxedOrUnboxedDenseElements(cx, resObj_, index_,
> +                                                                              v.address(), 1,
> +                                                                              UpdateTypes);

We should increment index_. Maybe it'd be clearer now to do index_++ right before the |return true;| below (and remove the index_++ in the else-branch).

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8613792 [details] [diff] [review]
everything else

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

::: js/src/jit/MCallOptimize.cpp
@@ +602,5 @@
>                  value = valueDouble;
>              }
>  
> +            if (!initializeArrayElement(ins, i, value, unboxedType, /* addResumePoint = */ i == initLength - 1))
> +                return InliningStatus_Error;

I just realized this will now increment initLength after each element, instead of only doing it at the end. I think this would be pretty easy (and nice) to avoid.

Also, the ins->convertDoubleElements() case is also handled by initializeArrayElement, AFAICS. Can we remove that here?

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8614794 [details] [diff] [review]
inline _DefineDataProperty

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

::: js/src/jit/MCallOptimize.cpp
@@ +2108,5 @@
> +    // setElemTryDense will push the value as the result of the define instead
> +    // of |undefined|, but this is fine if the rval is ignored (as it should be
> +    // in self hosted code.)
> +    if (*GetNextPc(pc) != JSOP_POP)
> +        return InliningStatus_NotInlined;

I think it'd be fine to turn this into an assert. It shouldn't happen in self-hosted code and if it ever does happen there's likely a bug somewhere.

@@ +2116,5 @@
> +        return InliningStatus_Error;
> +    if (!emitted)
> +        return InliningStatus_NotInlined;
> +
> +    // setElemTryDense will push the value as the result of the define, 

Remove this unfinished comment; there's a similar comment above.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8614795 [details] [diff] [review]
other fixes

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

::: js/src/jit/MIR.h
@@ +11896,5 @@
>      void collectRangeInfoPreTrunc() override;
>      AliasSet getAliasSet() const override {
>          return AliasSet::Load(AliasSet::Element);
>      }
>      bool congruentTo(const MDefinition* ins) const override {

congruentTo should check unboxedType == ins->unboxedType. Maybe there's no observable difference but I think it's good to be careful.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dbe1acdee3c

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c79bf9d31d8

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Pulsebot from comment #15)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7c79bf9d31d8

Any idea why "js1_5/Regress/regress-312588.js" is timing out with the cgc build?

On IRC I suggested disabling it for now with cgc but it'd be good to check the test didn't get way slower due to a performance fault somewhere.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5dbe1acdee3c
https://hg.mozilla.org/mozilla-central/rev/7c79bf9d31d8

Comment 18

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #16)
> Any idea why "js1_5/Regress/regress-312588.js" is timing out with the cgc
> build?

No, I don't.  This test creates a million arrays and does a compacting GC every 100 allocations, so it was probably taking quite a long time already.