1276955 - Inline constructor of large typed arrays

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Sander Mathijs van Veen]

This is a follow-up bug for 'inline constructors of typed arrays'. This bug report is used to implement constructor inlining of large typed arrays. In this case, large means that the elements of the typed array do not fit in the maximum sized JSObject and therefore require another memory allocation.

Comment 1

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcc2cee17422

Comment 2

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ea492a2658f

Comment 3

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2608597a0e8

Comment 4

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d4dad3ff2fd

Comment 5

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0900f4106ba

Comment 6

[Sander Mathijs van Veen]

Attachment: inline-large-typed-arrays.patch

This patch implements small and large typed arrays using an MIR and LIR opcode: NewTypedArray. Non-constant sized typed arrays will be implemented in a separate bug report.

This patch includes the small patch(https://bug1279898.bmoattachments.org/attachment.cgi?id=8762527) of bug 1279898, because it is not possible to run a try job successfully without that code. I'll remove that from the final patch after review. (I assume that that patch has landed in the mean time.)

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8762552 [details] [diff] [review]
inline-large-typed-arrays.patch

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

> This patch includes the small patch

Generally it's preferable in this case not to produce one intermingled patch, but just to comment saying that the patch for *this* bug builds upon the patch for *that* bug.  It's common for reviewers to have to deal with a patch that can't land on its own, particularly within a single bug.  A dependency across bugs isn't that much more unusual.

Alternatively, I hear there's this newfangled Mozreview thing that might help with multi-part patches like this.  But I'm somewhat negative on its UI versus Splinter, so I'm not *actually* suggesting it, just noting its existence as a possible solution.  ;-)  And if you used it, I'd adapt.

...or actually, as I look at this, isn't it a patch atop that one?  If this patch "included" that one I'd be complaining somewhat, but it looks like it just builds on that one, exactly as I describe above as being cool.

r+ as far as that goes, but certainly someone else should be looking at various parts of this, as the various comments note.

::: js/src/gc/Nursery.h
@@ +137,5 @@
>      }
>  
> +    /* Common internal allocator function. */
> +    void* allocate(size_t size);
> +

Doesn't look like it's necessary to move this, is it?  (If it moves from private to public, ignore me.  :-) )

::: js/src/jit/AliasAnalysisShared.cpp
@@ +54,5 @@
>  
> +    if (object->isTypedArrayElements())
> +        return nullptr;
> +    if (object->isTypedObjectElements())
> +        return nullptr;

I don't understand what this code is doing to know whether these changes make sense.

::: js/src/jit/MCallOptimize.cpp
@@ +2332,5 @@
>      MConstant* templateConst = MConstant::NewConstraintlessObject(alloc(), obj);
>      current->add(templateConst);
>  
> +    MInstruction* ins = MNewTypedArray::New(alloc(), constraints(), obj,
> +                                            obj->group()->initialHeap(constraints()));

Um, it looks like |templateConst| is now sort of dangling, isn't it?  It's not passed to the MNewTypedArray, certainly, even tho it's added.  Should it be removed?

::: js/src/jit/MIR.h
@@ +3347,5 @@
> +    CompilerGCPointer<TypedArrayObject*> templateObject_;
> +    gc::InitialHeap initialHeap_;
> +
> +    MNewTypedArray(CompilerConstraintList* constraints, TypedArrayObject* templateObject,
> +              gc::InitialHeap initialHeap)

Align gc:: directly underneath CompilerConstraintList.

@@ +3360,5 @@
> +  public:
> +    INSTRUCTION_HEADER(NewTypedArray)
> +
> +    static MNewTypedArray* New(TempAllocator& alloc,
> +                                     CompilerConstraintList* constraints,

CCL and subsequent arguments should align directly under TempAllocator.

(Is it maybe the case there are tab characters throwing off alignment here?  I thought Splinter highlighted those, but I might be wrong.)

::: js/src/jit/MacroAssembler.cpp
@@ +21,5 @@
>  #include "js/Conversions.h"
>  #include "vm/TraceLogging.h"
>  
>  #include "jsobjinlines.h"
> +#include "gc/Nursery-inl.h"

Blank line before this added line, please.  (You probably want to verify |make check-style|, as well -- I had thought it verified blank lines like this, but if there wasn't one before, it probably doesn't.)

@@ +1016,5 @@
>      }
>      *startOfUndefined = 0;
>  }
>  
> +/*static*/ void*

Why can't this be actually static?  And why not void for the return type?

@@ +1022,5 @@
> +{
> +    MOZ_ASSERT(count != 0);
> +
> +    Nursery& nursery = cx->asJSContext()->runtime()->gc.nursery;
> +    void *buf = (void*)nursery.allocate(count * sizeof(HeapSlot));

void* buf, and don't cast to void* if it's already void*.

@@ +1027,5 @@
> +    if (buf) {
> +        obj->initPrivate(buf);
> +        memset(buf, 0, count * sizeof(HeapSlot));
> +    } else {
> +        obj->initPrivate(nullptr);

I'd prefer if we had an accessor like initViewData or something, that clearly was consistent with the TypedArray::dataOffset() in the next function, but I couldn't find one right now.  So I guess this does for now.

@@ +1046,5 @@
> +    size_t offset = dataOffset + sizeof(HeapSlot);
> +    size_t n = templateObj->length() * templateObj->bytesPerElement();
> +
> +    static_assert(TypedArrayObject::FIXED_DATA_START == TypedArrayObject::DATA_SLOT + 1,
> +                    "Fixed data must be stored after the dataOffset slot");

This assertion seems better in the <= branch below, as it doesn't appear anything in the |else| depends on it.

@@ +1060,5 @@
> +        static_assert(sizeof(HeapSlot) == 8, "Assumed 8 bytes alignment");
> +        size_t words = ((n + 7) & ~0x7) / sizeof(char *);
> +
> +        for (size_t i = 0; i < words; i++)
> +            storePtr(ImmWord(0), Address(obj, offset + i * sizeof(HeapSlot)));

Make sure to apply the modifications mentioned in the previous review here, most especially the s/HeapSlot/char*/ change.

@@ +1066,5 @@
> +        move32(Imm32(n), lengthReg);
> +
> +        // TODO verify that this check is required
> +        size_t max_buf_size = Nursery::MaxNurseryBufferSize / sizeof(HeapSlot);
> +        branch32(Assembler::Above, lengthReg, Imm32(max_buf_size), fail);

This check should just be asserted.  We shouldn't have a template typed array object created if its element memory would be too big to fit in the nursery.  (As for who verifies, I think I have comments elsewhere on this patch for chokepoints that should ensure this.)

@@ +1069,5 @@
> +        size_t max_buf_size = Nursery::MaxNurseryBufferSize / sizeof(HeapSlot);
> +        branch32(Assembler::Above, lengthReg, Imm32(max_buf_size), fail);
> +
> +        // Store the length of the buffer in the object
> +        store32(lengthReg, Address(obj, TypedArrayObject::lengthOffset()));

If you're going to go to the trouble of storing the length here, seems like you might as well not pass it to AllocateObjectBufferWithInit, right?  Or eliminate this store and make that method do it.  One or the other.

@@ +1074,5 @@
> +
> +        // Allocate a buffer on the heap to store the data elements
> +        PushRegsInMask(liveRegs);
> +        setupUnalignedABICall(temp);
> +        loadJSContext(temp);

I had a recollection that you couldn't reuse the register passed to setupUnalignedABICall as an argument, but looking at the backends' code I can't see it any more.  Someone double-check this?

@@ +1204,5 @@
>              storePtr(ImmPtr(emptyObjectElements), Address(obj, NativeObject::offsetOfElements()));
>  
>              initGCSlots(obj, temp, ntemplate, initContents);
>  
> +            if (!ntemplate->is<TypedArrayObject>() && ntemplate->hasPrivate()) {

Flip the conditions around -- hasPrivate() is the broader test and will rule out more things, is-typed-array the narrower test.

::: js/src/jit/Recover.cpp
@@ -1209,5 @@
>        case MNewObject::ObjectCreate:
>          resultObject = ObjectCreateWithTemplate(cx, templateObject.as<PlainObject>());
>          break;
> -      case MNewObject::TypedArray:
> -        resultObject = TypedArrayCreateWithTemplate(cx, templateObject.as<TypedArrayObject>());

So if this disappears, does MNewTypedArray need recovering?  (I don't quite know enough about the JITs to be sure of the answer here.)

::: js/src/jsobj.cpp
@@ +3694,5 @@
>       */
>      if (is<TypedArrayObject>() && !as<TypedArrayObject>().hasBuffer()) {
>          size_t nbytes = as<TypedArrayObject>().byteLength();
> +        if (nbytes >= TypedArrayObject::INLINE_BUFFER_LIMIT) {
> +            const Class* clasp = as<TypedArrayObject>().getClass();

The as<>(). part of this shouldn't be needed, I think.  And really, I think you could just simplify all this to

    if (nbytes >= TypedArrayObject::INLINE_BUFFER_LIMIT)
        return GetGCObjectKind(getClass());

::: js/src/vm/ObjectGroup.cpp
@@ +205,5 @@
>  
>      /*
>       * Objects created outside loops in global and eval scripts should have
> +     * singleton types. For now this is only done for plain objects, but not
> +     * typed arrays nor normal arrays.

but not typed arrays or normal arrays.  (That is, double negatives of not...nor are bad, and "not" is assumed to apply to both of the "or" cases, not just the first.)

::: js/src/vm/TypedArrayObject.cpp
@@ +55,5 @@
>  using JS::CanonicalizeNaN;
>  using JS::ToInt32;
>  using JS::ToUint32;
>  
> +#define JS_FOR_EACH_TYPED_ARRAY(macro) \

Actually, it's dumb having this in the .cpp at all -- please move it into vm/TypedArrayObject.h (with the formatting from the other review).

We have a ton of places that should use this, once it's in place, but that should be separate patches and at least a separate bug.

@@ +163,5 @@
> +        return;
> +
> +    // Update the data slot pointer if it points to the old JSObject
> +    if (oldObj->hasInlineElements())
> +        newObj->setInlineElements();

Oh boy howdy is this better than the monstrosity in the other patch!

@@ +168,5 @@
> +}
> +
> +/* static */ size_t
> +TypedArrayObject::objectMovedDuringMinorGC(JSTracer* trc, JSObject* obj, const JSObject* old,
> +                                           gc::AllocKind allocKind)

Could I convince you to name allocKind, newAllocKind or something, so it's clear which way the meaning goes?

@@ +174,5 @@
> +    TypedArrayObject* newObj = &obj->as<TypedArrayObject>();
> +    const TypedArrayObject* oldObj = &old->as<TypedArrayObject>();
> +    MOZ_ASSERT(newObj->elements() == oldObj->elements());
> +
> +    // Typed arrays with a buffer object do not need an update

Period at end of complete sentence.

@@ +197,5 @@
> +        nbytes = oldObj->length() * sizeof(T); \
> +    break;
> +JS_FOR_EACH_TYPED_ARRAY(OBJECT_MOVED_TYPED_ARRAY)
> +#undef OBJECT_MOVED_TYPED_ARRAY
> +    default:

default/break/case/etc. should be indented as noted in the earlier review.

@@ +200,5 @@
> +#undef OBJECT_MOVED_TYPED_ARRAY
> +    default:
> +        MOZ_CRASH("Unsupported TypedArray type");
> +    }
> +    if (dataOffset() + nbytes <= GetGCKindBytes(allocKind)) {

Blank line before this.

@@ +209,5 @@
> +
> +        AutoEnterOOMUnsafeRegion oomUnsafe;
> +        uint8_t* data = newObj->zone()->pod_malloc<uint8_t>(nbytes);
> +        if (!data)
> +            oomUnsafe.crash("Failed to allocate typed array elements while tenuring.");

*grmbls about our GC getting worse and worse about being infallible*

@@ +223,5 @@
> +    return newObj->hasInlineElements() ? 0 : nbytes;
> +}
> +
> +bool
> +TypedArrayObject::hasInlineElements() const {

{ on new line, and for TypedArrayObject::setInlineElements.

@@ +229,5 @@
> +}
> +
> +void
> +TypedArrayObject::setInlineElements() {
> +    *(void **)((((char *)this) + this->dataOffset())) = this->fixedData(TypedArrayObject::FIXED_DATA_START);

Eeeeugh.  Split this up into a couple more variables, and use C++-style casting to make things more readable/clearer.

  char* dataPointerLocation = static_cast<char*>(this) + this->dataOffset();
  *reinterpret_cast<void**>(dataPointerLocation) = this->fixedData(...);

@@ +550,5 @@
> +                                  : AllocKindForLazyBuffer(len * sizeof(NativeType));
> +        MOZ_ASSERT(CanBeFinalizedInBackground(allocKind, clasp));
> +        allocKind = GetBackgroundAllocKind(allocKind);
> +
> +        void *buf = nullptr;

void* buf

@@ +553,5 @@
> +
> +        void *buf = nullptr;
> +
> +        if (!fitsInline) {
> +            buf = allocateTypedArrayElementsBuffer(cx, len);

Also regarding the TODO mentioned later in this file's comments, we could/should assert we don't allocate elements too large to fit in the nursery here.

@@ +597,5 @@
>          MOZ_ASSERT(obj->numFixedSlots() == TypedArrayObject::DATA_SLOT);
>  
> +        if (buf)
> +            obj->initPrivate(buf);
> +        else {

Braces around the if-consequent, because the else-alternative is braced:

        if (buf) {
            obj->initPrivate(buf);
        } else {

@@ +618,5 @@
> +        if (len == 0)
> +            return nullptr;
> +
> +        void* buf = nullptr;
> +        buf = cx->runtime()->pod_callocCanGC<HeapSlot>(len);

void* buf = cx->runtime()->pod_callocCanGC<HeapSlot>(len);

@@ +622,5 @@
> +        buf = cx->runtime()->pod_callocCanGC<HeapSlot>(len);
> +        if (!buf)
> +            ReportOutOfMemory(cx);
> +        if (!buf)
> +            return nullptr;

Uh, surely

         if (!buf) {
             ReportOutOfMemory(cx);
             return nullptr;
         }

?

@@ +643,5 @@
> +        RootedObjectGroup group(cx, templateObj->group());
> +
> +        NewObjectKind newKind = GenericObject;
> +
> +        void *buf = nullptr;

void* buf

@@ +646,5 @@
> +
> +        void *buf = nullptr;
> +        if (!fitsInline) {
> +            buf = allocateTypedArrayElementsBuffer(cx, len);
> +

No blank line here.

@@ +660,5 @@
> +        if (!tmp)
> +            return nullptr;
> +
> +        Rooted<TypedArrayObject*> obj(cx, &tmp->as<TypedArrayObject>());
> +        initTypedArraySlots(obj, len, buf, allocKind);

initTypedArraySlots(&tmp->as<TypedArrayObject>(), len, buf, allocKind);

@@ +1266,5 @@
>  TypedArrayObject::GetTemplateObjectForNative(JSContext* cx, Native native, uint32_t len,
>                                               MutableHandleObject res)
>  {
>  #define CHECK_TYPED_ARRAY_CONSTRUCTOR(T, N) \
> +    if (native == &TypedArrayObjectTemplate<T>::class_constructor) { \

Seems to me, regarding the TODO about nursery size, we should add that check in here -- only create the template object if its elements would fit in the max nursery size.

@@ +2443,5 @@
>      nullptr,                 /* setProperty */
>      nullptr,                 /* enumerate   */
>      nullptr,                 /* resolve     */
>      nullptr,                 /* mayResolve  */
> +    TypedArrayObject::finalize, /* finalize    */

Someone GC should probably look at this, 'cause last I heard adding finalizers to classes was Bad.  And we wouldn't want to be Bad.

@@ +2487,5 @@
>      JSCLASS_HAS_PRIVATE |                                                      \
>      JSCLASS_HAS_CACHED_PROTO(JSProto_##_type##Array) |                         \
> +    JSCLASS_DELAY_METADATA_BUILDER |                                           \
> +    JSCLASS_SKIP_NURSERY_FINALIZE |                                            \
> +    JSCLASS_BACKGROUND_FINALIZE,                                               \

I am not going to claim to be able to review these additions, except that they seem perfectly cromulent to me.

Comment 8

[Jan de Mooij [:jandem]]

smvv, it might be best to land the first part now, then rebase the other two patches, fix Waldo's nits, and request review from me?

Sorry for the delay, I can review quickly when you have updated patches.

Comment 9

[Sander Mathijs van Veen]

Attachment: inline-typed-array.patch

Added reviewer names to commit message.

Comment 10

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18f07cb2ccd9

Comment 11

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=644d6696265d

Comment 12

[Sander Mathijs van Veen]

Attachment: inline-large-typed-arrays.patch

I've changed the patch based on Waldo's comments.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8768371 [details] [diff] [review]
inline-large-typed-arrays.patch

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

Looks great! Some issues/questions below, once that's addressed I can review quickly.

::: js/src/jit/CodeGenerator.cpp
@@ +5429,5 @@
> +    JSObject* templateObject = lir->mir()->templateObject();
> +    gc::InitialHeap initialHeap = lir->mir()->initialHeap();
> +
> +    TypedArrayObject* ttemplate = &templateObject->as<TypedArrayObject>();
> +    size_t n = ttemplate->length();

uint32_t n

@@ +5432,5 @@
> +    TypedArrayObject* ttemplate = &templateObject->as<TypedArrayObject>();
> +    size_t n = ttemplate->length();
> +
> +    OutOfLineCode* ool = oolCallVM(TypedArrayConstructorOneArgInfo, lir,
> +                                   ArgList(ImmGCPtr(templateObject), ImmWord(n)),

Nit: Imm32(n), since it's an int32 argument.

::: js/src/jit/MacroAssembler.cpp
@@ +1017,5 @@
>      *startOfUndefined = 0;
>  }
>  
> +static void
> +AllocateObjectBufferWithInit(ExclusiveContext* cx, TypedArrayObject* obj, uint32_t count)

s/ExclusiveContext/JSContext/ (ExclusiveContext suggests it can be called on background threads).

@@ +1023,5 @@
> +    obj->setFixedSlot(TypedArrayObject::LENGTH_SLOT, Int32Value(count));
> +
> +    // Typed arrays with a non-compile-time known size that have a count of zero eventually are
> +    // essentially typed arrays with inline elements. The bounds check will make sure that no
> +    // elements are read or written to that memory.

Nit: comments should fit in 80 columns.

@@ +1087,5 @@
> +        loadJSContext(temp);
> +        passABIArg(temp);
> +        passABIArg(obj);
> +        passABIArg(lengthReg);
> +        callWithABI(reinterpret_cast<void*>(&AllocateObjectBufferWithInit));

masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, AllocateObjectBufferWithInit));

::: js/src/vm/TypedArrayObject.cpp
@@ +135,5 @@
> +
> +    Nursery& nursery = fop->runtime()->gc.nursery;
> +
> +    // Free the data slot pointer if it does not point into the old JSObject
> +    // nor to a buffer allocated on the nursery.

Nit: s/on/in/

@@ +136,5 @@
> +    Nursery& nursery = fop->runtime()->gc.nursery;
> +
> +    // Free the data slot pointer if it does not point into the old JSObject
> +    // nor to a buffer allocated on the nursery.
> +    if (!curObj->hasInlineElements() && !nursery.isInside(curObj->elements()))

I think the elements can never be in the nursery if the object itself isn't. Please remove the |nursery = ...| line above, and change this to:

if (!curObj->hasInlineElements()) {
    MOZ_ASSERT(!fop->runtime()->gc.nursery.isInside(curObj->elements()));
    js_free(curObj->elements());
}

@@ +146,5 @@
> +{
> +    TypedArrayObject* newObj = &obj->as<TypedArrayObject>();
> +    const TypedArrayObject* oldObj = &old->as<TypedArrayObject>();
> +
> +    // Typed arrays with a buffer object do not need an update

Nit: period at the end, and below.

@@ +177,5 @@
>      }
> +
> +    // Determine if we can use inline data for the target array. If this is
> +    // possible, the nursery will have picked an allocation size that is large
> +    // enough.

Hm this part I don't understand. It seems to me we should be able to say: if the old array used inline elements, the new one will use them too, and same for non-inline elements. Why isn't that true?

@@ +535,4 @@
>      {
> +        NewObjectKind newKind = TenuredObject;
> +        const Class* clasp = instanceClass();
> +        bool fitsInline = len * sizeof(NativeType) <= INLINE_BUFFER_LIMIT;

Two concerns:

(1) This multiplication can overflow, right? Use CalculateAllocSize to check for that.

(2) When someone allocates a huge typed array, will we create a template object with that huge size? That seems undesirable because it could easily introduce new OOMs for code that ran fine before.

It seems we should either add a constant to cap the length, or to store the length separately from the template object and have the template object use a smaller length. What do you think?

@@ +607,5 @@
> +        if (!buf) {
> +            ReportOutOfMemory(cx);
> +            return nullptr;
> +        }
> +        memset(buf, 0, len * sizeof(HeapSlot));

The pod_callocCanGC call will zero the elements (it's the difference between calloc and malloc), so we can remove this memset.

@@ +616,5 @@
> +    makeTypedArrayWithTemplate(JSContext* cx, TypedArrayObject* templateObj, uint32_t len)
> +    {
> +        AutoSetNewObjectMetadata metadata(cx);
> +
> +        bool fitsInline = len * sizeof(NativeType) <= INLINE_BUFFER_LIMIT;

Same overflow comment here.

Comment 14

[Sander Mathijs van Veen]

Attachment: inline-large-typed-arrays.patch

I've addressed the nits.

> @@ +177,5 @@
> >      }
> > +
> > +    // Determine if we can use inline data for the target array. If this is
> > +    // possible, the nursery will have picked an allocation size that is large
> > +    // enough.
> Hm this part I don't understand. It seems to me we should be able to say: if the old array used inline elements, the new one will use them too, and same for non-inline elements. Why isn't that true?

I've added |MOZ_ASSERT(oldObj->hasElementsInside());| when |dataOffset() + nbytes <= GetGCKindBytes(newAllocKind)| but that assertion failed. I did not look into it any further to see what caused the problem.

> (2) When someone allocates a huge typed array, will we create a template object with that huge size? That seems undesirable because it could easily introduce new OOMs for code that ran fine before.
> 
> It seems we should either add a constant to cap the length, or to store the length separately from the template object and have the template object use a smaller length. What do you think?

I agree. The template object doesn't need any memory since there won't be any elements that are stored in that memory. It is simply unused. Now, I point it where the inline elements would have been stored.

Comment 15

[Sander Mathijs van Veen]

s/hasElementsInside/hasInlineElements/

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8768764 [details] [diff] [review]
inline-large-typed-arrays.patch

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

Sorry, I really wanted to r+ this but I noticed some other things I forgot earlier. This stuff is complicated :/

::: js/src/jit/MacroAssembler.cpp
@@ +1020,5 @@
>  
> +static void
> +AllocateObjectBufferWithInit(JSContext* cx, TypedArrayObject* obj, uint32_t count)
> +{
> +    obj->setFixedSlot(TypedArrayObject::LENGTH_SLOT, Int32Value(count));

Sorry, I forgot this earlier, but we should add an |AutoCheckCannotGC nogc(cx);| at the start of this function (we don't want to GC here, for instance because obj is unrooted).

@@ +1026,5 @@
> +    // Typed arrays with a non-compile-time known size that have a count of zero
> +    // eventually are essentially typed arrays with inline elements. The bounds
> +    // check will make sure that no elements are read or written to that memory.
> +    if (count == 0) {
> +        obj->setInlineElements();

Is this case actually possible, considering the dataOffset + nbytes <= JSObject::MAX_BYTE_SIZE check we do before emitting a call to this? It'd be nice if we could assert it instead.

@@ +1030,5 @@
> +        obj->setInlineElements();
> +        return;
> +    }
> +
> +    void* buf = AllocateObjectBuffer<HeapSlot>(cx, obj, count);

Doesn't this allocate too much memory? sizeof(HeapSlot) is 8, so for an Int8Array with length 100, this will allocate at least 800 bytes instead of 100.

We should also assert nbytes < SINGLETON_BYTE_LENGTH here.

::: js/src/vm/TypedArrayObject.cpp
@@ +123,5 @@
>      ArrayBufferViewObject::trace(trc, objArg);
> +}
> +
> +void
> +TypedArrayObject::finalize(FreeOp* fop, JSObject* obj)

Please file a follow-up bug/patch to update maybeCreateArrayBuffer? IIUC we no longer need to create an array buffer there when byteLength > INLINE_BUFFER_LIMIT.

That'd be another nice optimization and it would let us test this code better.

@@ +534,4 @@
>      {
> +        size_t nbytes;
> +        if (!js::CalculateAllocSize<NativeType>(len, &nbytes))
> +            return nullptr;

Hm, I didn't realize this earlier, but we should also return nullptr here when nbytes >= TypedArrayObject::SINGLETON_BYTE_LENGTH. We shouldn't create non-singletons in that case.

@@ +613,5 @@
> +    static TypedArrayObject*
> +    makeTypedArrayWithTemplate(JSContext* cx, TypedArrayObject* templateObj, uint32_t len)
> +    {
> +        size_t nbytes;
> +        if (!js::CalculateAllocSize<NativeType>(len, &nbytes))

This can be MOZ_ALWAYS_TRUE(CalculateAllocSize(...)); right? We should also assert < SINGLETON_BYTE_LENGTH here.

@@ +1253,1 @@
>          return true; \

I think here we should |return !!res;|, or else the caller will propagate false up the stack. Please also write a jit-test for this case (new Int32Array($largeNumber) inside a loop, and then test it throws the right exception and doesn't abort execution).

Comment 17

[Sander Mathijs van Veen]

Attachment: inline-large-typed-arrays.patch

I've updated the patch using your suggestions.

Comment 18

[Jan de Mooij [:jandem]]

Comment on attachment 8769623 [details] [diff] [review]
inline-large-typed-arrays.patch

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

Great, thanks again!

::: js/src/jit/MacroAssembler.cpp
@@ +1023,5 @@
> +AllocateObjectBufferWithInit(JSContext* cx, TypedArrayObject* obj, uint32_t count)
> +{
> +    JS::AutoCheckCannotGC nogc(cx);
> +
> +    obj->setFixedSlot(TypedArrayObject::LENGTH_SLOT, Int32Value(count));

As discussed on IRC, make sure the data slot is initialized on all return paths, to make sure GC won't crash.

::: js/src/vm/TypedArrayObject.cpp
@@ +534,4 @@
>      {
> +        size_t nbytes;
> +        if (!js::CalculateAllocSize<NativeType>(len, &nbytes))
> +            return nullptr;

Because we also do this check in the caller, you can change this to something like:

size_t nbytes;
MOZ_ALWAYS_TRUE(CalculateAllocSize<NativeType>(len, &nbytes));

Comment 19

[Sander Mathijs van Veen]

Attachment: inline-large-typed-arrays.patch

Fixed the remaining two comments.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/480e54e0ae6c
Inline constructor of large typed arrays. r=jandem, r=Waldo

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/480e54e0ae6c

Comment 22

[Nicolas B. Pierron [:nbp]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> Comment on attachment 8762552 [details] [diff] [review]
> 
> ::: js/src/jit/Recover.cpp
> @@ -1209,5 @@
> >        case MNewObject::ObjectCreate:
> >          resultObject = ObjectCreateWithTemplate(cx, templateObject.as<PlainObject>());
> >          break;
> > -      case MNewObject::TypedArray:
> > -        resultObject = TypedArrayCreateWithTemplate(cx, templateObject.as<TypedArrayObject>());
> 
> So if this disappears, does MNewTypedArray need recovering?  (I don't quite
> know enough about the JITs to be sure of the answer here.)

Yes, it should have.  Unfortunately an optimization disabled the assertion which was checking that we do use this code path.  Thus, this did not appear in the test suite as a regression while this question did pin-point a real problem.

I will fix this specific issue as part of Bug 1132888.