1525352 - Add VM-call for TypedArray constructor with ArrayBuffer argument

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P1)

Description

[André Bargull [:anba]]

bug 1520286 only covered the case when the input is not an ArrayBuffer.

Comment 1

[André Bargull [:anba]]

Attachment: bug1525352.patch

Adds a simple VMCall when the TypedArray constructor is called with a (Shared)ArrayBuffer to avoid the expensive ObjectGroup::setAllocationSiteObjectGroup call. The new MIR node MNewTypedArrayFromArrayBuffer uses a simple BoxPolicy for the byteOffset and length arguments for now. As a follow-up we could consider changing it to BoxExceptPolicy<?, MIRType::Int32> to delay the boxing until the callVM, which may reduce register pressure. We'll need some additional helpers like
LIRGeneratorShared::useBoxOrTypedOrConstantAtStart for this to work, therefore I think this should happen in a follow-up bug. (*)

• IonBuilder::inlineTypedArray is now a bit more permissive in so far that we no longer reject inlining if too many arguments are present. This doesn't match how we normally decide whether or not to inline a built-in function, but apart from that shouldn't make any difference.
• I've moved the arguments length check from BaselineIC.cpp to TypedArrayObject.cpp, because it seemed a bit cleaner to me to have a single point where we inspect the arguments to decide whether or not to create a template object.
• TypedArrayObject::fromBufferSameCompartment no longer creates a singleton object when the constructor was called from Ion-inlined code (because we now have a specific ObjectGroup). This should be okay, because we can only inline a TypedArray constructor call which was already called at least once, which means it's probably not a singleton object anyway when the constructor is called multiple times from a specific callsite.

(*) With this patch, new Int32Array(ab, 0, i & 3) gets emitted as

instruction BitOpI:bitand
andl       $0x3, %edx
instruction Box:Int32
movabsq    $0xfff8800000000000, %rcx
orq        %rdx, %rcx
instruction Value
movabsq    $0xfff8800000000000, %rax
instruction MoveGroup
movq       0x10(%rsp), %rdx
instruction NewTypedArrayFromArrayBuffer
push       %rcx
push       %rax
push       %rdx
movabsq    $0x2c43c3088100, %r11
push       %r11
push       $0x4020
call       .Lfrom373

And when using an ad-hoc patch to switch to BoxExceptPolicy<?, MIRType::Int32>, we'd end up with the following assembly, which looks a bit more efficient w.r.t. register use:

instruction BitOpI:bitand
andl       $0x3, %eax
instruction MoveGroup
movq       0x10(%rsp), %rcx
instruction NewTypedArrayFromArrayBuffer
movabsq    $0xfff8800000000000, %r11
orq        %rax, %r11
push       %r11
movabsq    $0xfff8800000000000, %r11
push       %r11
push       %rcx
movabsq    $0x2ca6fa788100, %r11
push       %r11
push       $0x4020
call       .Lfrom375

Comment 2

[Jan de Mooij [:jandem]]

(In reply to André Bargull [:anba] from comment #1)

to avoid the expensive ObjectGroup::setAllocationSiteObjectGroup call.

Maybe we should consider removing that call.. It's not clear how useful allocation site groups are for typed arrays.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 9041900 [details] [diff] [review]
bug1525352.patch

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

Great! I'm a little concerned about a growing number of (similar) code paths in TypedArrayObject.cpp. However code is mostly shared, there's not too much duplicated *complexity* and these cases are all common.

::: js/src/jit/MCallOptimize.cpp
@@ +2927,5 @@
>  
>    if (getInlineReturnType() != MIRType::Object) {
>      return InliningStatus_NotInlined;
>    }
> +  if (callInfo.argc() == 0) {

Maybe add callInfo.argc() > 3 to be conservative in weird cases.

@@ +2984,5 @@
> +        TemporaryTypeSet::ForAllResult::ALL_FALSE) {
> +      return InliningStatus_NotInlined;
> +    }
> +
> +    // Don't inline if we saw mixed use of (Shared)ArrayBuffers.

I initially misread this comment as "Don't inline if we saw both ArrayBuffers and SharedArrayBuffers."

Maybe add " and other objects." to disambiguate?

Comment 4

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #2)

Maybe we should consider removing that call.. It's not clear how useful allocation site groups are for typed arrays.

It'd be nice to know for which reasons allocation site groups for typed arrays were added in the first place. I think they were added in this changeset, but the bug number (bug 663485) in the changeset seems to be wrong and I wasn't able to find the correct bug report. :-(

Comment 5

[André Bargull [:anba]]

Attachment: bug1525352.patch

Updated per review comments, carrying r+.

Comment 6

[André Bargull [:anba]]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7922211d6dcb
Add VM-call for TypedArray constructor with ArrayBuffer argument. r=anba

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/7922211d6dcb