Consider removing unboxed arrays

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: evilpie, Assigned: jandem)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [js:tech-debt])

Attachments

(12 attachments, 1 obsolete attachment)

272.49 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
58.97 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
33.74 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
6.96 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
8.48 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
25.04 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
8.52 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
6.10 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
18.82 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
21.77 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
10.96 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
3.08 KB, patch
anba
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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.
Priority: -- → P3
Whiteboard: [js:tech-debt]
(Assignee)

Comment 1

2 years ago
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).
Flags: needinfo?(bhackett1024)
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.
Flags: needinfo?(bhackett1024)
(Assignee)

Updated

2 years ago
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 years ago
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.
Attachment #8911751 - Flags: review?(evilpies)
(Reporter)

Comment 4

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

Comment 5

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

Comment 6

2 years ago
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
Attachment #8912130 - Flags: review?(evilpies)
(Reporter)

Comment 7

2 years ago
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
Attachment #8911751 - Flags: review?(evilpies) → review+
(Reporter)

Updated

2 years ago
Attachment #8912130 - Flags: review?(evilpies) → review+

Comment 8

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/442671394512
part 1 - Remove most unboxed array code. r=evilpie
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 9

2 years ago
This changes a bunch of functions to take/return ArrayObject* instead of JSObject*.
Attachment #8912735 - Flags: review?(evilpies)
(Assignee)

Comment 10

2 years ago
Attachment #8912743 - Flags: review?(evilpies)
(Assignee)

Comment 11

2 years ago
Attachment #8912766 - Flags: review?(evilpies)
(Assignee)

Comment 12

2 years ago
Attachment #8912768 - Flags: review?(evilpies)
(Reporter)

Comment 13

2 years ago
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?
Attachment #8912735 - Flags: review?(evilpies) → review+
(Reporter)

Updated

2 years ago
Attachment #8912743 - Flags: review?(evilpies) → review+
(Reporter)

Comment 14

2 years ago
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?
Attachment #8912766 - Flags: review?(evilpies) → review+
(Reporter)

Comment 15

2 years ago
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 17

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9e0402afdb
part 2 - Inline some small helper functions into the callers. r=evilpie
(Assignee)

Comment 18

2 years ago
This makes GetDenseElements and ArrayJoinDenseKernel return bool instead of DenseElementResult, and it fixes the ArrayShiftDenseKernel return value check (good catch!).
Attachment #8912768 - Attachment is obsolete: true
Attachment #8912768 - Flags: review?(evilpies)
Attachment #8913133 - Flags: review?(evilpies)
(Reporter)

Updated

2 years ago
Attachment #8913133 - Flags: review?(evilpies) → review+
(Assignee)

Comment 19

2 years ago
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.
Attachment #8913155 - Flags: review?(evilpies)
(Assignee)

Comment 20

2 years ago
I renamed MoveBoxedOrUnboxedDenseElements to MoveDenseElements and moved it to jsarray.cpp. It also takes a NativeObject* instead of JSObject* now.
Attachment #8913169 - Flags: review?(evilpies)
(Assignee)

Comment 21

2 years ago
This renames SetOrExtendBoxedOrUnboxedDenseElements to NativeObject::setOrExtendDenseElements
Attachment #8913171 - Flags: review?(evilpies)
(Assignee)

Comment 22

2 years ago
Removes remaining code and fixes some comments mentioning unboxed arrays.
Attachment #8913182 - Flags: review?(evilpies)
(Assignee)

Comment 23

2 years ago
We can now use JSOP_NEWARRAY instead of JSOP_SPREADCALLARRAY. See the comment in vm/Opcodes.h
Attachment #8913188 - Flags: review?(evilpies)
(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
(Assignee)

Comment 25

2 years ago
Thanks for the suggestion :)
Attachment #8913192 - Flags: review?(andrebargull)
Comment on attachment 8913192 [details] [diff] [review]
Part 12 - Inline CanonicalizeArrayLengthValue

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

\o/
Attachment #8913192 - Flags: review?(andrebargull) → review+
(Reporter)

Updated

2 years ago
Attachment #8913155 - Flags: review?(evilpies) → review+
(Reporter)

Updated

2 years ago
Attachment #8913169 - Flags: review?(evilpies) → review+
(Reporter)

Updated

2 years ago
Attachment #8913171 - Flags: review?(evilpies) → review+
(Reporter)

Updated

2 years ago
Attachment #8913182 - Flags: review?(evilpies) → review+
(Reporter)

Updated

2 years ago
Attachment #8913188 - Flags: review?(evilpies) → review+

Comment 27

2 years ago
Pushed by jandemooij@gmail.com:
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
(Assignee)

Comment 28

2 years ago
(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 30

2 years ago
Pushed by jandemooij@gmail.com:
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 32

2 years ago
Pushed by jandemooij@gmail.com:
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
(Assignee)

Updated

2 years ago
Keywords: leave-open
*applause*
You need to log in before you can comment on or make changes to this bug.