1398768 - Consider removing unboxed arrays

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tom S [:evilpie]]

The unboxed arrays code has never been enabled. It's relatively complex and touches many parts of the engine. Last I checked it doesn't pass our test suite. Over the recent months we added many optimizations to normal arrays, like shifting, and we would need to make sure unboxed arrays are not slower in those cases. I think we should remove unboxed arrays unless somebody can spend the time (probably quite a bit) to optimize and fix the code.

Comment 1

[Jan de Mooij [:jandem]]

Brian, WDYT? There's definitely performance overhead and complexity from supporting unboxed arrays (in array natives for instance). As Tom mentioned in comment 0, unboxed arrays also don't support the shifting optimization I added and they come with other potential perf cliffs (when adding named properties for instance).

Comment 2

[Brian Hackett [Laid off!]]

Sure, these would be fine to remove.  They do improve microbenchmarks but never helped much with larger benchmarks, and there are better things to work on.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Remove most code

This removes most (but not all) unboxed array code. There's a lot more code we can remove and cleanup in follow-up patches.

 41 files changed, 442 insertions(+), 3026 deletions(-)

evilpie volunteered to review.

Comment 4

[Tom S [:evilpie]]

Comment on attachment 8911751 [details] [diff] [review]
Part 1 - Remove most code

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

Looks pretty good, but this is an insane amount of code. I want to do a second pass later.

::: js/src/jit/Lowering.cpp
@@ -3362,5 @@
>      const LUse elements = useRegister(ins->elements());
>      const LAllocation index = useRegisterOrConstant(ins->index());
>  
>      // Use a temp register when adding new elements to unboxed arrays.
>      LDefinition tempDef = LDefinition::BogusTemp();

I guess we don't need this anymore?

@@ -3397,5 @@
>      const LAllocation index = useRegisterOrConstant(ins->index());
>  
>      // Use a temp register when adding new elements to unboxed arrays.
>      LDefinition tempDef = LDefinition::BogusTemp();
> -    if (ins->unboxedType() != JSVAL_TYPE_MAGIC)

Same

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #4)
> I guess we don't need this anymore?

Yes I wanted to remove this in a follow-up patch.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Inline helper functions

This inlines most of the helper functions (like GetAnyBoxedOrUnboxedInitializedLength) in UnboxedObject-inl.h into the callers.

I also changed some functions to take ArrayObject* or NativeObject* instead of JSObject* to avoid a lot of as<NativeObject>() casts.

Again there's still a lot left to clean up here, especially in jsarray.cpp

Comment 7

[Tom S [:evilpie]]

Comment on attachment 8911751 [details] [diff] [review]
Part 1 - Remove most code

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

Looks pretty good, but this is an insane amount of code. I want to do a second pass later.

::: js/src/jit/Lowering.cpp
@@ -3362,5 @@
>      const LUse elements = useRegister(ins->elements());
>      const LAllocation index = useRegisterOrConstant(ins->index());
>  
>      // Use a temp register when adding new elements to unboxed arrays.
>      LDefinition tempDef = LDefinition::BogusTemp();

I guess we don't need this anymore?

@@ -3397,5 @@
>      const LAllocation index = useRegisterOrConstant(ins->index());
>  
>      // Use a temp register when adding new elements to unboxed arrays.
>      LDefinition tempDef = LDefinition::BogusTemp();
> -    if (ins->unboxedType() != JSVAL_TYPE_MAGIC)

Same

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/442671394512
part 1 - Remove most unboxed array code. r=evilpie

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Use ArrayObject* instead of JSObject*

This changes a bunch of functions to take/return ArrayObject* instead of JSObject*.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Remove temps

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Remove flags

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Remove functors for array natives

Comment 13

[Tom S [:evilpie]]

Comment on attachment 8912735 [details] [diff] [review]
Part 3 - Use ArrayObject* instead of JSObject*

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

::: js/src/jit/BaselineIC.cpp
@@ +2431,3 @@
>      if (!nobj)
>          return false;
> +    EnsureArrayGroupAnalyzed(cx, nobj); //XXX

todo?

Comment 14

[Tom S [:evilpie]]

Comment on attachment 8912766 [details] [diff] [review]
Part 5 - Remove flags

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

::: js/src/vm/ObjectGroup.cpp
@@ -1726,5 @@
> -            group->setPreliminaryObjects(preliminaryObjects);
> -        else
> -            cx->recoverFromOutOfMemory();
> -    }
> -

There is similar code in ObjectGroup::newArrayObject, not guarded by unboxedArrays, could we remove that as well?

Comment 15

[Tom S [:evilpie]]

Comment on attachment 8912768 [details] [diff] [review]
Part 6 - Remove functors for array natives

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

That functor stuff is definitely complicated. I don't like using DenseElementResult for stuff that can't fail.

::: js/src/jsarray.cpp
@@ +1522,3 @@
>  {
>      /* An empty array or an array with no elements is already reversed. */
> +    if (length == 0 || obj->getDenseInitializedLength() == 0)

Much nicer with HandleNativeObject.

@@ +2449,2 @@
>      if (result != DenseElementResult::Incomplete) {
> +        MOZ_ASSERT(result == DenseElementResult::Success);

Not quite sure why this holds. MoveBoxedOrUnboxedDenseElements seems fallible. MoveBoxedOrUnboxedDenseElements vs ShiftMoveBoxedOrUnboxedDenseElements ...

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/442671394512

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9e0402afdb
part 2 - Inline some small helper functions into the callers. r=evilpie

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Remove functors for array natives

This makes GetDenseElements and ArrayJoinDenseKernel return bool instead of DenseElementResult, and it fixes the ArrayShiftDenseKernel return value check (good catch!).

Comment 19

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Remove CopyBoxedOrUnboxedDenseElements

This removes CopyBoxedOrUnboxedDenseElements and replaces it with a NativeObject::initDenseElements overload.

It also cleans up initDenseElements a bit: the dstStart argument is always 0 so we can remove it, and initDenseElements can call setDenseInitializedLength so the callers don't have to do that.

Comment 20

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Clean up MoveBoxedOrUnboxedDenseElements

I renamed MoveBoxedOrUnboxedDenseElements to MoveDenseElements and moved it to jsarray.cpp. It also takes a NativeObject* instead of JSObject* now.

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Part 9 - Clean up SetOrExtendBoxedOrUnboxedDenseElements

This renames SetOrExtendBoxedOrUnboxedDenseElements to NativeObject::setOrExtendDenseElements

Comment 22

[Jan de Mooij [:jandem]]

Attachment: Part 10 - Misc changes

Removes remaining code and fixes some comments mentioning unboxed arrays.

Comment 23

[Jan de Mooij [:jandem]]

Attachment: Part 11 - Remove JSOP_SPREADCALLARRAY

We can now use JSOP_NEWARRAY instead of JSOP_SPREADCALLARRAY. See the comment in vm/Opcodes.h

Comment 24

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #22)
> Created attachment 8913182 [details] [diff] [review]
> Part 10 - Misc changes
> 
> Removes remaining code and fixes some comments mentioning unboxed arrays.

I think we can now also change js::CanonicalizeArrayLengthValue [1] to a static function resp. directly inline it in js::ArraySetLength (IIRC I saw CanonicalizeArrayLengthValue in µ-benchmarks because it didn't get inlined by gcc/clang).

[1] http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/js/src/jsarray.cpp#705

Comment 25

[Jan de Mooij [:jandem]]

Attachment: Part 12 - Inline CanonicalizeArrayLengthValue

Thanks for the suggestion :)

Comment 26

[André Bargull [:anba]]

Comment on attachment 8913192 [details] [diff] [review]
Part 12 - Inline CanonicalizeArrayLengthValue

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

\o/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec5b0fd9f4d
part 3 - Use ArrayObject* instead of JSObject* in some places. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2d0f8a0196f
part 4 - Remove unused temps from some LIR instructions. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/62a67f9e8966
part 5 - Remove unboxed arrays shell flag and context option. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/923ba800cbb8
part 6 - Remove functors for array natives. r=evilpie

Comment 28

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #13)
> todo?

(In reply to Tom Schuster [:evilpie] from comment #14)
> There is similar code in ObjectGroup::newArrayObject, not guarded by
> unboxedArrays, could we remove that as well?

Part 10 addresses these comments.

Comment 29

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

https://hg.mozilla.org/mozilla-central/rev/5d9e0402afdb
https://hg.mozilla.org/mozilla-central/rev/6ec5b0fd9f4d
https://hg.mozilla.org/mozilla-central/rev/d2d0f8a0196f
https://hg.mozilla.org/mozilla-central/rev/62a67f9e8966
https://hg.mozilla.org/mozilla-central/rev/923ba800cbb8

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8358f3e52fa4
part 7 - Replace CopyBoxedOrUnboxedDenseElements with a NativeObject::initDenseElements overload. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/4403e849f860
part 8 - Clean up and rename MoveBoxedOrUnboxedDenseElements. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/a02ebffc8cce
part 9 - Clean up and rename SetOrExtendBoxedOrUnboxedDenseElements. r=evilpie

Comment 31

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

https://hg.mozilla.org/mozilla-central/rev/8358f3e52fa4
https://hg.mozilla.org/mozilla-central/rev/4403e849f860
https://hg.mozilla.org/mozilla-central/rev/a02ebffc8cce

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/320e6535b5e7
part 10 - Remove and clean up more code. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/dff7893dce77
part 11 - Remove JSOP_SPREADCALLARRAY and just use JSOP_NEWARRAY again. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/a483b64bfc05
part 12 - Remove/inline CanonicalizeArrayLengthValue. r=anba

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/320e6535b5e7
https://hg.mozilla.org/mozilla-central/rev/dff7893dce77
https://hg.mozilla.org/mozilla-central/rev/a483b64bfc05

Comment 34

[Jason Orendorff [:jorendorff]]

*applause*