1517259 - Various clean-ups for inlined TypedArray construction

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

I was taking another look at bug 1394386 and found a couple of spots which could be cleaned up in the code for inlined TypedArray construction.

Comment 1

[André Bargull [:anba]]

Attachment: bug1517259-part1-macro-name-clang-format.patch

Change the macro name to get better clang-format output.

(More small changes coming! :-)

Comment 2

[André Bargull [:anba]]

Attachment: bug1517259-part2-remove-dead-code.patch

Remove a bit of dead code. (The AssertedCast can be removed, because its input is already an int32_t, so an asserted-cast to int32_t is a no-op.)

Comment 3

[André Bargull [:anba]]

Attachment: bug1517259-part3-add-constexpr.patch

Sprinkle a few constexpr throughout TypedArray code.

- The |static const| are already implicit constexpr, but it seems nicer to be explicit here.
- Adding constexpr for ArrayTypeIsUnsigned/ArrayTypeIsFloatingPoint will allow us to use constexpr-if in [1] as soon as we're allowed to use C++17.
- This condition [1] in |MacroAssembler::initTypedArraySlots| was confusing for me, because everywhere else we're using |TypedArrayObject::INLINE_BUFFER_LIMIT| to check if we can use inline elements. Only after substituting the constants and reordering the operands on both sides of the comparison, I saw that |JSObject::MAX_BYTE_SIZE - dataOffset| is exactly equal to |TypedArrayObject::INLINE_BUFFER_LIMIT|. So change the condition to use |TypedArrayObject::INLINE_BUFFER_LIMIT| and additionally add a static_assert to ensure |JSObject::MAX_BYTE_SIZE - dataOffset| matches |TypedArrayObject::INLINE_BUFFER_LIMIT|.


[1] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/vm/TypedArrayObject.cpp#353-367
[2] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/jit/MacroAssembler.cpp#1220-1221

Comment 4

[André Bargull [:anba]]

Attachment: bug1517259-part4-replace-sizeof-with-bytesperelement.patch

We're currently using |TypedArrayObject::bytesPerElement()| [1] to convert a typed array length to its byte length value, but within TypedArrayObjectTemplate we were mostly using |sizeof(NativeType)| instead of |BYTES_PER_ELEMENT|. So replace most |sizeof(NativeType)| with |BYTES_PER_ELEMENT| to align the code with the more generic |TypedArrayObject::bytesPerElement()| callers.

[1] https://searchfox.org/mozilla-central/search?q=bytesPerElement()&case=true&path=

Comment 5

[André Bargull [:anba]]

Attachment: bug1517259-part5-remove-code-duplication-typedarray-copy-ctor.patch

We don't need to have separate branches for the two |AllocateArrayBuffer| callers in |TypedArrayObjectTemplate::fromTypedArray|, because |byteLength| in [1] is exactly equal to |elementLength * BYTES_PER_ELEMENT| [2] when |ArrayTypeID() == srcType| [3] is true. 

This also allows us to remove the |unit| parameter from |AllocateArrayBuffer| and |maybeCreateArrayBuffer|, because now all callers pass |BYTES_PER_ELEMENT|.

[1] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/vm/TypedArrayObject.cpp#1149
[2] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/vm/TypedArrayObject.cpp#1126
[3] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/vm/TypedArrayObject.cpp#1147

Comment 6

[André Bargull [:anba]]

Attachment: bug1517259-part6-replace-CalculateAllocSize.patch

We can replace these two |js::CalculateAllocSize| callers with a direct multiplication, because directly before the |js::CalculateAllocSize| call, we've already checked for a possible overflow.

This aligns the code with the rest of TypedArrayObject.cpp, which is also using a plain multiplication instead of using |js::CalculateAllocSize|.

Comment 7

[André Bargull [:anba]]

Attachment: bug1517259-part7-fix-more-nits.patch

Switching reviewers for the next patches to balance the reviews a bit.

Some small nits which I think make sense to fix.

TypedArrayObject::objectMoved
- Add |const| to the |oldObj| variable to match other |objectMoved| functions in the tree.

TypedArrayObject::finalize
- We can directly call |oldObj->byteLength()| to compute the length in bytes.

SpeciesConstructorOverride, CreateSingleton:
- Use typed enums to ensure the compiler treats the enum values as bool.

makeTypedArrayWithTemplate
- Assert the instance-class matches the group's class, and use the instance-class for GetGCObjectKind to match |makeTemplateObject| and |makeInstance|.
- Assert |len| is greater than zero if |fitsInline| is false instead of checking |len > 0| in the if-statement's condition.

Comment 8

[André Bargull [:anba]]

Attachment: bug1517259-part8-remove-explicit-GetBackgroundAllocKind-calls.patch

We don't need to have explicit calls to |GetBackgroundAllocKind|, because both |NewBuiltinClassInstance| (in makeTemplateObject) and |NewObjectWithGroup| (in makeTypedArrayWithTemplate) already select the background-allocation kind automatically. So I think it less distracting to just pass the normal AllocKind and let NewBuiltinClassInstance/NewObjectWithGroup handle the check for possible background allocation.

---

We do seem to miss an explicit call to |GetBackgroundAllocKind| for typed arrays, but that's at a completely different place: https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/vm/JSObject.cpp#3982

That means tenured typed arrays which were allocated from jitted code (and don't have inline elements) are always foreground finalized, which doesn't seem correct - I still need to check if the foregound finalization is for some reason required in that case, though.

Comment 9

[André Bargull [:anba]]

Attachment: bug1517259-part9-remove-unreachable-code-for-arraybuffer-tracking.patch

Add assertions that ArrayBufferViews whose buffer gets detached always use non-shared memory and also do have a buffer in the first place.

This complete block [1] seems unreachable, because it means a TypedArray object without a buffer object gets notified its buffer was detached - and that sounds like a contradiction. 

Maybe the block was added because [2] was misinterpreted as a possible place where a TypedArray object is added to the tracked views of an ArrayBuffer, without also setting the buffer-slot of the TypedArray. For example when |addView| fails after inserting the TypedArray in its tracked views. But that case cannot, because the very first call to |addView| is always infallible, which can assert there.

[1] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/vm/ArrayBufferViewObject.cpp#70-79
[2] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/vm/TypedArrayObject.cpp#93-95

Comment 10

[André Bargull [:anba]]

Attachment: bug1517259-part10-call-initFixedSlot-for-initialization.patch

We don't need to trigger any pre-barriers when initializing these slots, so we can use |initFixedSlot| in |initTypedArraySlots| to match the equivalent code in |ArrayBufferViewObject::init|.

Comment 11

[André Bargull [:anba]]

Attachment: bug1517259-part11-move-friendapi-functions.patch

Just moves the friend API functions from the "TypedArrayObject boilerplate" block [1] to the "JS Friend API" block [2].

[1] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/vm/TypedArrayObject.cpp#1820
[2] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/vm/TypedArrayObject.cpp#2191

Comment 12

[André Bargull [:anba]]

Attachment: bug1517259-part12-use-macro-instead-of-manual-enumeration.patch

We can use the |JS_FOR_EACH_TYPED_ARRAY| macro to initialize/define these fields/functions instead of manually enumerating all TypedArray kinds.

While there I've also added braces around if-statement blocks and replaced the duplicated CheckedUnwrap + TypedArrayObjectTemplate<NativeType>::instanceClass() checks in |JS_Is##Name##Array| and |JS_GetObjectAs##Name##Array| with calls to |js::Unwrap##Name##Array|. And also moved |JS_Get##Name##ArrayData| into the macro-generated block to get rid of the manually copied code at [1].

[1] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/vm/TypedArrayObject.cpp#2244-2363

----

I wonder if it makes sense to remove/consolidate some of the unwrapping friend-API functions. For example [2] care about GC and moved objects/buffers, whereas [3,4] directly expose the data pointer. [4] is additionally marked as |inline|, but may not necessarily be completely inlinable anymore, because it contains a call to |JS_GetTypedArraySharedness|, which doesn't look like inlinable.

[2] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/jsfriendapi.h#1885-1908
[3] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/jsfriendapi.h#1712-1745
[4] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/src/jsfriendapi.h#1654-1675

Comment 13

[André Bargull [:anba]]

Attachment: bug1517259-part13-move-into-namespace.patch

And the last one for now:

Moves the default |getIndexValue| definition into the anonymous namespace.

Comment 14

[André Bargull [:anba]]

(In reply to André Bargull [:anba] from comment #8)
> We do seem to miss an explicit call to |GetBackgroundAllocKind| for typed
> arrays, [...]

Filed bug 1517461.

Comment 15

[André Bargull [:anba]]

Attachment: bug1517259-part14-constexpr-variables.patch

I've missed to mark these variables as |constexpr| when I wrote part 3.

Comment 16

[Ted Campbell [:tcampbell]]

Comment on attachment 9034141 [details] [diff] [review]
bug1517259-part1-macro-name-clang-format.patch

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

:)

Comment 17

[Ted Campbell [:tcampbell]]

Comment on attachment 9034152 [details] [diff] [review]
bug1517259-part3-add-constexpr.patch

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

::: js/src/jit/MacroAssembler.cpp
@@ +1206,5 @@
>    MOZ_ASSERT(templateObj->hasPrivate());
>    MOZ_ASSERT(!templateObj->hasBuffer());
>  
> +  constexpr size_t dataSlotOffset = TypedArrayObject::dataOffset();
> +  constexpr size_t dataOffset = dataSlotOffset + sizeof(HeapSlot);

This probably should have been in TypedArrayObject to begin with. Oh well.

Comment 18

[Ted Campbell [:tcampbell]]

Comment on attachment 9034153 [details] [diff] [review]
bug1517259-part4-replace-sizeof-with-bytesperelement.patch

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

"BYTES_PER_ELEMENT" makes me think this is a global value rather than per-template-type, but that is pre-existing and the consistency you add here is good.

Comment 19

[Ted Campbell [:tcampbell]]

Comment on attachment 9034154 [details] [diff] [review]
bug1517259-part5-remove-code-duplication-typedarray-copy-ctor.patch

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

Great.

Comment 20

[Ted Campbell [:tcampbell]]

Comment on attachment 9034157 [details] [diff] [review]
bug1517259-part7-fix-more-nits.patch

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

::: js/src/vm/TypedArrayObject.cpp
@@ +264,2 @@
>  
> +enum class CreateSingleton : bool { Yes, No };

My preference is not to specify type for enum classes if there isn't a specific memory packing optimization we are doing. Do you see compiler generating specifically worse code here?

(This also has the fun |Yes == 0| condition which we could fix too..)

@@ +525,4 @@
>                                            : AllocKindForLazyBuffer(nbytes);
>      MOZ_ASSERT(CanBeFinalizedInBackground(allocKind, clasp));
>      allocKind = GetBackgroundAllocKind(allocKind);
>      RootedObjectGroup group(cx, templateObj->group());

Nit: Move this before the clasp calculation and share.

Comment 21

[Ted Campbell [:tcampbell]]

Comment on attachment 9034169 [details] [diff] [review]
bug1517259-part12-use-macro-instead-of-manual-enumeration.patch

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

Excellent!

Comment 22

[André Bargull [:anba]]

(In reply to Ted Campbell [:tcampbell] from comment #20)
> ::: js/src/vm/TypedArrayObject.cpp
> @@ +264,2 @@
> >  
> > +enum class CreateSingleton : bool { Yes, No };
> 
> My preference is not to specify type for enum classes if there isn't a
> specific memory packing optimization we are doing. Do you see compiler
> generating specifically worse code here?
> 

I guess "worse" is debatable. :-)

When using godbolt to check the generated assembly for the following simple toy program on GCC (8.2, x64), MSVC (19.16, x64), and Clang (7.0, x64), there were some differences (tested with -O2 and -O3 optimization levels):
---
#include <cstdlib>

enum class E { No, Yes };

void* f1(E e, size_t size) {
    if (e == E::Yes)
        return std::malloc(size);
    return std::calloc(size, sizeof(char));
}

enum class T : bool { No, Yes };

void* f2(T e, size_t size) {
    if (e == T::Yes)
        return std::malloc(size);
    return std::calloc(size, sizeof(char));
}

void* f3(bool e, size_t size) {
    if (e)
        return std::malloc(size);
    return std::calloc(size, sizeof(char));
}
---

GCC:
- Untyped enum value is compared using |cmp edi, 1|
- Typed enum is compared using |cmp dil, 1|
- Bool is compared using |test dil, dil|

MSVC:
- Untyped enum value is compared using |cmp ecx, 1|
- Typed enum is compared using |cmp cl, 1|
- Bool is compared using |test cl, cl|

Clang:
- Untyped enum value is compared using |cmp edi, 1|
- Typed enum is compared using |test dil, dil|
- Bool is compared using |test dil, dil|


GCC/MSVC results:
- I don't think it makes any difference to load |dil| instead of the complete register |edi| in this case.

Clang results:
- Using |test| instead of |cmp| seems to be a bit better.

But when I switched the enum values, so that |Yes| is before |No|, all three compilers started to emit |test| instructions!


> 
> @@ +525,4 @@
> >                                            : AllocKindForLazyBuffer(nbytes);
> >      MOZ_ASSERT(CanBeFinalizedInBackground(allocKind, clasp));
> >      allocKind = GetBackgroundAllocKind(allocKind);
> >      RootedObjectGroup group(cx, templateObj->group());
> 
> Nit: Move this before the clasp calculation and share.

Part 8 will remove the |clasp| local variable.

Comment 23

[Jan de Mooij [:jandem]]

Comment on attachment 9034161 [details] [diff] [review]
bug1517259-part8-remove-explicit-GetBackgroundAllocKind-calls.patch

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

Makes sense.

::: js/src/vm/TypedArrayObject.cpp
@@ -432,5 @@
>      MOZ_ASSERT(nbytes < TypedArrayObject::SINGLETON_BYTE_LENGTH);
>      NewObjectKind newKind = TenuredObject;
>      bool fitsInline = nbytes <= INLINE_BUFFER_LIMIT;
>      const Class* clasp = instanceClass();
>      gc::AllocKind allocKind = !fitsInline ? gc::GetGCObjectKind(clasp)

While you're here (and assuming you agree) mind changing the condition here and below from !fitsInline to fitsInline (and swapping the two arms)? It's easier to read when you don't have to do this in your head :)

Comment 24

[Jan de Mooij [:jandem]]

Comment on attachment 9034164 [details] [diff] [review]
bug1517259-part9-remove-unreachable-code-for-arraybuffer-tracking.patch

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

Excellent, thanks.

Comment 25

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #23)
> ::: js/src/vm/TypedArrayObject.cpp
> @@ -432,5 @@
> >      MOZ_ASSERT(nbytes < TypedArrayObject::SINGLETON_BYTE_LENGTH);
> >      NewObjectKind newKind = TenuredObject;
> >      bool fitsInline = nbytes <= INLINE_BUFFER_LIMIT;
> >      const Class* clasp = instanceClass();
> >      gc::AllocKind allocKind = !fitsInline ? gc::GetGCObjectKind(clasp)
> 
> While you're here (and assuming you agree) mind changing the condition here
> and below from !fitsInline to fitsInline (and swapping the two arms)? It's
> easier to read when you don't have to do this in your head :)

I think [1,2] are both using |!fitsInline| to match the conditional operator usage from the default construction path at [3].

[1] https://searchfox.org/mozilla-central/rev/ecf61f8f3904549f5d65a8a511dbd7ea4fd1a51d/js/src/vm/TypedArrayObject.cpp#457-458
[2] https://searchfox.org/mozilla-central/rev/ecf61f8f3904549f5d65a8a511dbd7ea4fd1a51d/js/src/vm/TypedArrayObject.cpp#548-549
[3] https://searchfox.org/mozilla-central/rev/ecf61f8f3904549f5d65a8a511dbd7ea4fd1a51d/js/src/vm/TypedArrayObject.cpp#419-421

Comment 26

[Jan de Mooij [:jandem]]

Ah that's fair enough.

Comment 27

[André Bargull [:anba]]

Attachment: bug1517259-part10-call-initFixedSlot-for-initialization.patch

Update patch commit message to reflect actual reviewer. Carrying r+.

Comment 28

[André Bargull [:anba]]

Attachment: bug1517259-part11-move-friendapi-functions.patch

Update patch commit message to reflect actual reviewer. Carrying r+.

Comment 29

[André Bargull [:anba]]

Attachment: bug1517259-part12-use-macro-instead-of-manual-enumeration.patch

Update patch commit message to reflect actual reviewer. Carrying r+.

Comment 30

[André Bargull [:anba]]

Attachment: bug1517259-part7-fix-more-nits.patch

Update commit message to reflect actual reviewer and applied review comments from comment #20. Carrying r+.

Comment 31

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22537cf2fd00e4af6132abd7a05827c37bd79eda

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ee71cbdb6ab
Part 2: Remove dead or useless code. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f35396665f5
Part 3: Sprinkle a few constexpr throughout TypedArray code. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4b4151af797
Part 4: Replace most sizeof with BYTES_PER_ELEMENT to align with TypedArrayObject::bytesPerElement() users. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe6986e02b5
Part 5: Remove unnecessary extra branches in TypedArrayObjectTemplate<T>::fromTypedArray. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/26f01305411c
Part 6: Replace CalculateAllocSize with simple multiplication. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/f428c73aa133
Part 7: Fix a comment, add |const|, use typed enums, and more nits. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/f46527ec0104
Part 8: Remove explicit call to GetBackgroundAllocKind. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff9408e5956
Part 9: Replace unreachable code in ArrayBufferView tracking with assertions. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/10cd03b453f8
Part 10: Use initFixedSlot when initializing typed array slots. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4a25c85b318
Part 11: Move friend-api typed array functions closer together. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/247a8fda0496
Part 12: Use JS_FOR_EACH_TYPED_ARRAY instead of enumerating each typed array kind manually. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/0698427ff246
Part 1: Change parameter name in JS_FOR_EACH_TYPED_ARRAY macro for better clang-format output. r=tcampbell

Comment 33

[André Bargull [:anba]]

The push in comment #32 missed "Part 13" and "Part 14", setting "checkin-needed" again for those two parts.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5259ce5b5a40
Part 14: Add constexpr modifier to variables holding constexpr values. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/500c1db30533
Part 13: Move the default getIndexValue method into the anonymous namespace. r=jandem

Comment 35

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/9ee71cbdb6ab
https://hg.mozilla.org/mozilla-central/rev/8f35396665f5
https://hg.mozilla.org/mozilla-central/rev/b4b4151af797
https://hg.mozilla.org/mozilla-central/rev/fbe6986e02b5
https://hg.mozilla.org/mozilla-central/rev/26f01305411c
https://hg.mozilla.org/mozilla-central/rev/f428c73aa133
https://hg.mozilla.org/mozilla-central/rev/f46527ec0104
https://hg.mozilla.org/mozilla-central/rev/0ff9408e5956
https://hg.mozilla.org/mozilla-central/rev/10cd03b453f8
https://hg.mozilla.org/mozilla-central/rev/f4a25c85b318
https://hg.mozilla.org/mozilla-central/rev/247a8fda0496
https://hg.mozilla.org/mozilla-central/rev/0698427ff246
https://hg.mozilla.org/mozilla-central/rev/5259ce5b5a40
https://hg.mozilla.org/mozilla-central/rev/500c1db30533