Consider removing unboxed arrays

RESOLVED FIXED in Firefox 58

Status

()

Core
JavaScript Engine
P3
normal
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: evilpie, Assigned: jandem)

Tracking

(Blocks: 1 bug)

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

5 months 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

5 months 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

5 months ago
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 months ago
Created attachment 8911751 [details] [diff] [review]
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.
Attachment #8911751 - Flags: review?(evilpies)
(Reporter)

Comment 4

5 months 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

5 months 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

5 months ago
Created attachment 8912130 [details] [diff] [review]
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
Attachment #8912130 - Flags: review?(evilpies)
(Reporter)

Comment 7

5 months 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

5 months ago
Attachment #8912130 - Flags: review?(evilpies) → review+

Comment 8

5 months 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

5 months ago
Keywords: leave-open
(Assignee)

Comment 9

5 months ago
Created attachment 8912735 [details] [diff] [review]
Part 3 - Use ArrayObject* instead of JSObject*

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

Comment 10

5 months ago
Created attachment 8912743 [details] [diff] [review]
Part 4 - Remove temps
Attachment #8912743 - Flags: review?(evilpies)
(Assignee)

Comment 11

5 months ago
Created attachment 8912766 [details] [diff] [review]
Part 5 - Remove flags
Attachment #8912766 - Flags: review?(evilpies)
(Assignee)

Comment 12

5 months ago
Created attachment 8912768 [details] [diff] [review]
Part 6 - Remove functors for array natives
Attachment #8912768 - Flags: review?(evilpies)
(Reporter)

Comment 13

5 months 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

5 months ago
Attachment #8912743 - Flags: review?(evilpies) → review+
(Reporter)

Comment 14

5 months 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

5 months 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

5 months 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

5 months ago
Created attachment 8913133 [details] [diff] [review]
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!).
Attachment #8912768 - Attachment is obsolete: true
Attachment #8912768 - Flags: review?(evilpies)
Attachment #8913133 - Flags: review?(evilpies)
(Reporter)

Updated

5 months ago
Attachment #8913133 - Flags: review?(evilpies) → review+
(Assignee)

Comment 19

5 months ago
Created attachment 8913155 [details] [diff] [review]
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.
Attachment #8913155 - Flags: review?(evilpies)
(Assignee)

Comment 20

5 months ago
Created attachment 8913169 [details] [diff] [review]
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.
Attachment #8913169 - Flags: review?(evilpies)
(Assignee)

Comment 21

5 months ago
Created attachment 8913171 [details] [diff] [review]
Part 9 - Clean up SetOrExtendBoxedOrUnboxedDenseElements

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

Comment 22

5 months ago
Created attachment 8913182 [details] [diff] [review]
Part 10 - Misc changes

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

Comment 23

5 months ago
Created attachment 8913188 [details] [diff] [review]
Part 11 - Remove JSOP_SPREADCALLARRAY

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

5 months ago
Created attachment 8913192 [details] [diff] [review]
Part 12 - Inline CanonicalizeArrayLengthValue

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

5 months ago
Attachment #8913155 - Flags: review?(evilpies) → review+
(Reporter)

Updated

5 months ago
Attachment #8913169 - Flags: review?(evilpies) → review+
(Reporter)

Updated

5 months ago
Attachment #8913171 - Flags: review?(evilpies) → review+
(Reporter)

Updated

5 months ago
Attachment #8913182 - Flags: review?(evilpies) → review+
(Reporter)

Updated

5 months ago
Attachment #8913188 - Flags: review?(evilpies) → review+

Comment 27

5 months 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

5 months 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

5 months 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

5 months 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

5 months ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/320e6535b5e7
https://hg.mozilla.org/mozilla-central/rev/dff7893dce77
https://hg.mozilla.org/mozilla-central/rev/a483b64bfc05
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
*applause*
You need to log in before you can comment on or make changes to this bug.