Closed Bug 1279992 Opened 4 years ago Closed 3 years ago

Inline constructor of typed arrays with non-compile-time known size

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: sandervv, Assigned: sandervv)

References

Details

Attachments

(1 file, 9 obsolete files)

This is a follow-up bug for 'inline constructors of typed arrays' and 'inline constructors of large typed arrays'. This bug report is used to implement constructor inlining of typed arrays that do not have a compile-time known size. It is not known in advance if the elements of the typed array fit in the maximum sized JSObject, and therefore require another memory allocation for the buffer.
Attachment #8762635 - Flags: review?(jwalden+bmo)
Attachment #8762635 - Flags: review?(jdemooij)
Assignee: nobody → sandervv
Benchmark:

    var global = null;

    function test(i) {
        global = new Float64Array(i);
        global[0] = 1.2;
    }

    for(var i=0;i<10000000;i++) {
        test((i & 0xff) + 2);
        if (global[0] === global[1])
            print('fail');
    }

Benchmark results without patch:

    $ for i in 1 2 3; do time ~/work/leaningtech/sm-js ~/work/leaningtech/tests/typed-arrays-dynamic.js; done
    elapsed: 0m12.552s (usr 0m8.872s; sys 0m3.864s; cpu 101.47%)
    elapsed: 0m12.823s (usr 0m8.900s; sys 0m4.104s; cpu 101.41%)
    elapsed: 0m12.395s (usr 0m8.772s; sys 0m3.808s; cpu 101.50%)

Benchmark results with patch:

    $ for i in 1 2 3; do time ~/work/leaningtech/sm-js ~/work/leaningtech/tests/typed-arrays-dynamic.js; done
    elapsed: 0m5.451s (usr 0m3.296s; sys 0m2.268s; cpu 102.06%)
    elapsed: 0m5.322s (usr 0m3.192s; sys 0m2.244s; cpu 102.14%)
    elapsed: 0m5.268s (usr 0m3.188s; sys 0m2.192s; cpu 102.12%)
Comment on attachment 8762635 [details] [diff] [review]
inline-dynamic-typed-arrays.patch

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

r+ as far as it goes, but this JIT stuff is beyond my core competence, so I'd have thrown for a second r+ at a JIT person even if you hadn't requested it.  Sorry for the delay responding!

::: js/src/jit/BaselineIC.cpp
@@ +5511,1 @@
>          if (TypedArrayObject::GetTemplateObjectForNative(cx, native, len, res))

I'm a bit unsure as to the consequences of a template object whose length doesn't match that of the typed array ultimately created.  Someone else should be looking at this.

::: js/src/jit/Lowering.cpp
@@ +251,5 @@
> +{
> +    MDefinition* length = ins->length();
> +    MOZ_ASSERT(length->type() == MIRType::Int32);
> +
> +    LNewTypedArrayDynamicLength* lir = new(alloc()) LNewTypedArrayDynamicLength(useRegister(length), temp());

I vaguely remember that useRegisterAtStart would be better/faster, if it could be used.  It seems to me that the length here is generally not going to be necessary after this instruction.  Could it be *AtStart?  (I assume this would require a change elsewhere.)

The last time I tried to understand this was years ago, and I failed at it and we had a fuzzbug complaining about exactly what I thought I'd understood enough to grok for reviewing purposes.  (I wasn't the only reviewer of it, either.)  So don't count on anything I just said without jandem taking it on as well.

::: js/src/jit/MIR.h
@@ +3388,5 @@
> +    CompilerObject templateObject_;
> +    gc::InitialHeap initialHeap_;
> +
> +    MNewTypedArrayDynamicLength(CompilerConstraintList* constraints, JSObject* templateObject,
> +                           gc::InitialHeap initialHeap, MDefinition* length)

Align gc:: with CCL.

@@ +3396,5 @@
> +    {
> +        setGuard(); // Need to throw if length is negative.
> +        setResultType(MIRType::Object);
> +        if (!templateObject->isSingleton())
> +            setResultTypeSet(MakeSingletonTypeSet(constraints, templateObject));

I don't understand these two lines.

::: js/src/jit/MacroAssembler.cpp
@@ +1063,5 @@
>          for (size_t i = 0; i < words; i++)
>              storePtr(ImmWord(0), Address(obj, offset + i * sizeof(HeapSlot)));
>      } else {
> +        if (!dynamicallySized)
> +            move32(Imm32(templateObj->length()), lengthReg);

...okay, here's where lengthReg becomes necessary.  I guess given that, it makes sense not to initialize length from JIT code in the prior patch, but to have AllocateObjectBufferWithInit do it, right?

@@ +1075,5 @@
>  
>          // Allocate a buffer on the heap to store the data elements
> +        liveRegs.addUnchecked(temp);
> +        liveRegs.addUnchecked(obj);
> +        liveRegs.addUnchecked(lengthReg);

My recollection of how to use this interface is near nil, so jandem should be checking this, not me.

::: js/src/jit/MacroAssembler.h
@@ +1473,5 @@
>      void initGCThing(Register obj, Register temp, JSObject* templateObj,
>                       bool initContents = true, bool convertDoubleElements = false);
>      void initTypedArraySlots(Register obj, Register temp, Register lengthReg,
>                               LiveRegisterSet liveRegs, Label* fail,
> +                             TypedArrayObject* templateObj, bool dynamicallySized);

Use an enum to indicate known-size versus dynamically-sized, please, something like so:

   enum class TypedArrayLength { Fixed, Dynamic };

::: js/src/vm/TypedArrayObject.cpp
@@ +42,5 @@
>  #include "vm/WrapperObject.h"
>  
>  #include "jsatominlines.h"
>  
> +//#include "gc/Nursery-inl.h"

Presumably this change is partial -- remove entirely or leave it alone.
Attachment #8762635 - Flags: review?(jwalden+bmo) → review+
Rebased patch. I still have to change the bool |dynamicallySized| to an enum.
Attachment #8762635 - Attachment is obsolete: true
Attachment #8762635 - Flags: review?(jdemooij)
Attachment #8768404 - Flags: review?(jdemooij)
I've rebased on top of the updated large typed array patch. This patch includes the enum class proposed by Waldo.
Attachment #8768404 - Attachment is obsolete: true
Attachment #8768404 - Flags: review?(jdemooij)
Attachment #8768770 - Flags: review?(jdemooij)
Rebased on top of large typed array patch.
Attachment #8768770 - Attachment is obsolete: true
Attachment #8768770 - Flags: review?(jdemooij)
Attachment #8769624 - Flags: review?(jdemooij)
Comment on attachment 8769624 [details] [diff] [review]
inline-dynamic-typed-arrays.patch

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

Nice!
Attachment #8769624 - Flags: review?(jdemooij) → review+
Attachment #8769624 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4197ec30757a
Inline constructor of typed arrays with non-compile-time known size r=jandem,Waldo
Keywords: checkin-needed
had to back this out for frequent memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=31718055&repo=mozilla-inbound#L523 that i guess are caused by this push
Flags: needinfo?(sandervv)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b00ee999a591
Backed out changeset 4197ec30757a for frequent memory leaks
I've fixed the memory leak. The leak was in |makeTypedArrayWithTemplate| and happpened when |tmp| was invalid and returned |nullptr| without free()ing |buf| which was allocated using |allocateTypedArrayElementsBuffer|.
Attachment #8770287 - Attachment is obsolete: true
Flags: needinfo?(sandervv)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1c5f34ad31b
Inline constructor of typed arrays with non-compile-time known size r=jandem,Waldo
Keywords: checkin-needed
sorry had to back this out again for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=31804330&repo=mozilla-inbound
Flags: needinfo?(sandervv)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/481501a8258f
Backed out changeset f1c5f34ad31b for memory leaks
The memory leak mentioned above by Carsten Book [:Tomcat] occurs when:
1) a typed array is allocated in the nursery.
2) the typed array is moved to the tenured heap, and the typed array data does not fit in the jsobject. This requires a malloc call for the data.
3) the method TypedArrayObject::ensureHasBuffer() is called (probably because the typed array's buffer object is used), which wrote a pointer (to the buffer elements) over the typed array's data pointer.
4) the method TypedArrayObject::finalize() is called, and the typed array has a buffer (hasBuffer() returns true) which will exit the method since typed arrays with a buffer object do not need to be free'd. 

The memory leak is fixed using the following patch:

@@ -101,16 +102,23 @@ TypedArrayObject::ensureHasBuffer(JSCont
[...]
     // tarray is not shared, because if it were it would have a buffer.
     memcpy(buffer->dataPointer(), tarray->viewDataUnshared(), tarray->byteLength());
+
+    // Free the data slot pointer if has no inline data
+    if (!tarray->hasInlineElements() && !cx->runtime()->gc.nursery.isInside(tarray->elements())) {
+        js_free(tarray->elements());
+        tarray->setInlineElements();
+    }
     tarray->setPrivate(buffer->dataPointer());
                                                                                
     tarray->setFixedSlot(TypedArrayObject::BUFFER_SLOT, ObjectValue(*buffer));

The fix will free the typed array's elements before that pointer is overwritten in ensureHasBuffer().

This patch does also include the fix for bug 1287412.
Attachment #8770723 - Attachment is obsolete: true
Flags: needinfo?(sandervv)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3feee73843
Inline constructor of typed arrays with non-compile-time known size. r=jandem, r=Waldo
Keywords: checkin-needed
Had similar crashes in other test suites as well.
Fixed try jobs issues (confirmed in last try job results).
Attachment #8773310 - Attachment is obsolete: true
Keywords: checkin-needed
has problems to apply: 

patching file js/src/vm/TypedArrayObject.cpp
Hunk #7 FAILED at 577
1 out of 8 hunks FAILED -- saving rejects to file js/src/vm/TypedArrayObject.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh inline-dynamic-typed-arrays.patch
Flags: needinfo?(sandervv)
Keywords: checkin-needed
Rebased on mozilla-central.
Attachment #8776928 - Attachment is obsolete: true
Flags: needinfo?(sandervv)
Keywords: checkin-needed
sorry seems a new conflict:

renamed 1279992 -> inline-dynamic-typed-arrays.patch
applying inline-dynamic-typed-arrays.patch
patching file js/src/gc/Nursery.h
Hunk #2 FAILED at 349
1 out of 2 hunks FAILED -- saving rejects to file js/src/gc/Nursery.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh inline-dynamic-typed-arrays.patch
Flags: needinfo?(sandervv)
Rebased on top of mozilla-inbound.
Attachment #8777730 - Attachment is obsolete: true
Flags: needinfo?(sandervv)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1f1638d126
Inline constructor of typed arrays with non-compile-time known size r=jandem,Waldo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cb1f1638d126
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1292858
Depends on: 1293258
Depends on: 1295031
Depends on: 1296015
Depends on: 1302682
Depends on: 1313807
Depends on: 1312480
You need to log in before you can comment on or make changes to this bug.