Various clean-ups for inlined TypedArray construction

RESOLVED FIXED in Firefox 66

Status

()

enhancement
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla66
Points:
---

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(14 attachments, 4 obsolete attachments)

1.63 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
3.07 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
9.25 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
8.46 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
7.18 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
2.59 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
2.12 KB, patch
jandem
: review+
Details | Diff | Splinter Review
4.49 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.62 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.97 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
1.43 KB, patch
anba
: review+
Details | Diff | Splinter Review
10.88 KB, patch
anba
: review+
Details | Diff | Splinter Review
22.06 KB, patch
anba
: review+
Details | Diff | Splinter Review
4.27 KB, patch
anba
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 months ago
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.
(Assignee)

Comment 1

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

(More small changes coming! :-)
Attachment #9034141 - Flags: review?(tcampbell)
(Assignee)

Comment 2

4 months ago
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.)
Attachment #9034144 - Flags: review?(tcampbell)
(Assignee)

Comment 3

4 months ago
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
Attachment #9034152 - Flags: review?(tcampbell)
(Assignee)

Comment 4

4 months ago
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=
Attachment #9034153 - Flags: review?(tcampbell)
(Assignee)

Comment 5

4 months ago
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
Attachment #9034154 - Flags: review?(tcampbell)
(Assignee)

Comment 6

4 months ago
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|.
Attachment #9034155 - Flags: review?(tcampbell)
(Assignee)

Comment 7

4 months ago
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.
Attachment #9034157 - Flags: review?(jdemooij)
(Assignee)

Comment 8

4 months ago
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.
Attachment #9034161 - Flags: review?(jdemooij)
(Assignee)

Comment 9

4 months ago
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
Attachment #9034164 - Flags: review?(jdemooij)
(Assignee)

Comment 10

4 months ago
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|.
Attachment #9034166 - Flags: review?(jdemooij)
(Assignee)

Comment 12

4 months ago
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
Attachment #9034169 - Flags: review?(jdemooij)
(Assignee)

Comment 13

4 months ago
And the last one for now:

Moves the default |getIndexValue| definition into the anonymous namespace.
Attachment #9034170 - Flags: review?(jdemooij)
(Assignee)

Comment 14

4 months ago
(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.
(Assignee)

Comment 15

4 months ago
I've missed to mark these variables as |constexpr| when I wrote part 3.
Attachment #9034176 - Flags: review?(tcampbell)
Comment on attachment 9034141 [details] [diff] [review]
bug1517259-part1-macro-name-clang-format.patch

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

:)
Attachment #9034141 - Flags: review?(tcampbell) → review+
Attachment #9034144 - Flags: review?(tcampbell) → review+
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.
Attachment #9034152 - Flags: review?(tcampbell) → review+
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.
Attachment #9034153 - Flags: review?(tcampbell) → review+
Comment on attachment 9034154 [details] [diff] [review]
bug1517259-part5-remove-code-duplication-typedarray-copy-ctor.patch

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

Great.
Attachment #9034154 - Flags: review?(tcampbell) → review+
Attachment #9034155 - Flags: review?(tcampbell) → review+
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.
Attachment #9034157 - Flags: review?(jdemooij) → review+
Attachment #9034166 - Flags: review?(jdemooij) → review+
Attachment #9034167 - Flags: review?(jdemooij) → review+
Comment on attachment 9034169 [details] [diff] [review]
bug1517259-part12-use-macro-instead-of-manual-enumeration.patch

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

Excellent!
Attachment #9034169 - Flags: review?(jdemooij) → review+
Attachment #9034176 - Flags: review?(tcampbell) → review+
(Assignee)

Comment 22

4 months ago
(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 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 :)
Attachment #9034161 - Flags: review?(jdemooij) → review+
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.
Attachment #9034164 - Flags: review?(jdemooij) → review+
Attachment #9034170 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 25

4 months ago
(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
Ah that's fair enough.
(Assignee)

Comment 27

4 months ago
Update patch commit message to reflect actual reviewer. Carrying r+.
Attachment #9034166 - Attachment is obsolete: true
Attachment #9034692 - Flags: review+
(Assignee)

Comment 28

4 months ago
Update patch commit message to reflect actual reviewer. Carrying r+.
Attachment #9034167 - Attachment is obsolete: true
Attachment #9034693 - Flags: review+
(Assignee)

Comment 29

4 months ago
Update patch commit message to reflect actual reviewer. Carrying r+.
Attachment #9034169 - Attachment is obsolete: true
Attachment #9034694 - Flags: review+
(Assignee)

Comment 30

4 months ago
Update commit message to reflect actual reviewer and applied review comments from comment #20. Carrying r+.
Attachment #9034157 - Attachment is obsolete: true
Attachment #9034708 - Flags: review+

Comment 32

4 months ago

Pushed by rgurzau@mozilla.com:
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

Keywords: checkin-needed
(Assignee)

Comment 33

4 months ago

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

Keywords: checkin-needed

Comment 34

4 months ago

Pushed by rgurzau@mozilla.com:
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

Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.