Closed Bug 1248163 Opened 8 years ago Closed 8 years ago

Inline constructor of typed arrays

Categories

(Core :: JavaScript Engine: JIT, defect)

Product:
Core
Component:
JavaScript Engine: JIT
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox47 --- affected
firefox50 --- fixed

People

(Reporter: sandervv, Assigned: sandervv)

References

Details

Attachments

(1 file, 12 obsolete files)

inline-typed-array.patch
35.11 KB, patch
Details | Diff | Splinter Review
Attached patch inline-typed-array.patch (obsolete) — — Splinter Review 
This patch inlines the constructor of small (<= 12 elements) typed arrays. Eventually, this patch should inline typed arrays with a separate buffer as well. Note that this patch is work-in-progress (e.g. some commented code, unimplemented branches).

Simple test case for measuring why inlining is useful:

    var globalB = null;

    function testB() {
        globalB = new Float64Array(10);
    }

    for(var i=0;i<10000000;i++) {
        testB();
    }

Preliminary results of inlining the constructor of small typed arrays:
runtime without patch: 2.930 seconds
runtime with inlining: 0.180 seconds

Currently, this patch works only for float64 typed arrays and typed arrays that do not require a separate array buffer object (= array with <= 12 elements). This patch should in the end also support all other typed array types and typed arrays with array buffer objects.

I'm observing a GC problem that I tracked down to the nursery poisoning JSObjects. I think that has to do with the fact that (probably) the JSObject from the MNewObject instruction is not rooted and therefore marked as not-in-use. What should I do in this case?

Initially, I thought that it was caused by moving the objects from the nursery to the tenured heap. Therefore, I've added `updateTypedArrayAfterMove` as callback that will update the data slot in typed arrays when a typed array is moved by GC. However, this is not implemented/used at the moment due to the problem with rooting/nursery.

All suggestions/ideas/comments are more than welcome!
Attachment #8719189 - Flags: feedback?(terrence)
The attached patch fails to build for me with:

In file included from /home/terrence/moz/branch/w/js/src/_cdD.nightly.noext.shell/js/src/Unified_cpp_js_src15.cpp:11:
/home/terrence/moz/branch/w/js/src/jit/MCallOptimize.cpp:2191:15: error: no member named 'isConstantValue' in 'js::jit::MDefinition'; did you mean 'isConstant'?
    if (!arg->isConstantValue())
              ^~~~~~~~~~~~~~~
              isConstant
/home/terrence/moz/branch/w/js/src/jit/MIR.h:851:21: note: 'isConstant' declared here
    MIR_OPCODE_LIST(OPCODE_CASTS)
                    ^
In file included from /home/terrence/moz/branch/w/js/src/_cdD.nightly.noext.shell/js/src/Unified_cpp_js_src15.cpp:11:
/home/terrence/moz/branch/w/js/src/jit/MCallOptimize.cpp:2194:25: error: no member named 'constantValue' in 'js::jit::MDefinition'
    uint32_t len = arg->constantValue().toInt32();

That said, I'm not entirely clear what the issue is here. You have an object and it's getting collected? What object? When? Presumably you could break where the object is created and set a hardware breakpoint to find when it gets poisoned. What do you find? If the object is collected, how did you expect it to be found by the GC? Why did this mechanism not work?
Attached patch inline-typed-array.patch (obsolete) — — Splinter Review 
Just saw your reply, but I've updated the patch in the mean time.

I fixed the issue! The problem was that the 'slow path' was not handled when nurseryAllocate jumped to fail. It took the Object.create path because of that, which was clearly not the right type.

Currently, I'm seeing these when running the jit-tests:
Assertion failure: !hasLazyGroup(), at /home/smvv/work/leaningtech/gecko-hg/js/src/jsobj.h:135
The failed tests are:
FAILURES:                                                                       
    /home/smvv/work/leaningtech/gecko-hg/js/src/jit-test/tests/SIMD/uconvert.js 
    --no-asmjs /home/smvv/work/leaningtech/gecko-hg/js/src/jit-test/tests/asm.js/testFastHeapAccess.js
    /home/smvv/work/leaningtech/gecko-hg/js/src/jit-test/tests/asm.js/testFastHeapAccess.js
    --no-asmjs /home/smvv/work/leaningtech/gecko-hg/js/src/jit-test/tests/asm.js/testSIMD-load-store.js
    /home/smvv/work/leaningtech/gecko-hg/js/src/jit-test/tests/asm.js/testSIMD-load-store.js
    /home/smvv/work/leaningtech/gecko-hg/js/src/jit-test/tests/asm.js/testSIMD.js
    /home/smvv/work/leaningtech/gecko-hg/js/src/jit-test/tests/basic/bug620532.js
    /home/smvv/work/leaningtech/gecko-hg/js/src/jit-test/tests/basic/spread-array.js
    /home/smvv/work/leaningtech/gecko-hg/js/src/jit-test/tests/basic/testTypedArraySetConversion.js
    /home/smvv/work/leaningtech/gecko-hg/js/src/jit-test/tests/basic/typed-array-sealed-frozen.js
    /home/smvv/work/leaningtech/gecko-hg/js/src/jit-test/tests/basic/typed-array-copyWithin.js
    /home/smvv/work/leaningtech/gecko-hg/js/src/jit-test/tests/ion/bug1107011-2.js
    /home/smvv/work/leaningtech/gecko-hg/js/src/jit-test/tests/ion/bug779595.js 
    /home/smvv/work/leaningtech/gecko-hg/js/src/jit-test/tests/ion/inlining/typedarray-data-inlining-neuter-samedata.js

I'm also seeing exit code -5 on these tests:

Exit code: -5                                                                   
FAIL - basic/typed-array-copyWithin.js                                          
[3424|  11|   0|   0]  68% ============================>              | 198.3s  
Exit code: -5                                                                   
FAIL - ion/bug1107011-2.js                                                      
[3676|  12|   0|   0]  73% ==============================>            | 211.1s  
Exit code: -5                                                                   
FAIL - ion/bug779595.js                                                         

Can we chat on Mozilla IRC? I'm in channel #jsapi
Attachment #8719189 - Attachment is obsolete: true
Attachment #8719189 - Flags: feedback?(terrence)
Attachment #8719832 - Flags: feedback?(terrence)
Attached patch inline-typed-array.patch (obsolete) — — Splinter Review 
This patch passes all jit-tests.

I see a good speed improvement for small typed arrays. Consider the following test case:

    var global = null;

    function test() {   
        global = new Float64Array(10);
        global[0] = 1.2
        //global = [1.2,0.1,0.1,0.1,0.1,0.1,0.1,0.1,0.1,0.1];
    }

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

Without patch: 2.9 sec.
With patch: 0.19 sec.

Changing the length from 10 to 13 (this exceeds the inline elements threshold):

Without patch: 7.9 sec.
With patch: 7.9 sec.

The speed for typed arrays with non-inlined elements are not affected. The normal array with doubles (see commented line in test case) takes 0.10 sec. Compared to the 0.19 sec needed for small typed arrays, I imagine it is possible to optimize typed arrays even further in the future.

As discussed with Terrence, the array buffer objects will be implemented in a separate patch.
Attachment #8719832 - Attachment is obsolete: true
Attachment #8719832 - Flags: feedback?(terrence)
Attachment #8720282 - Flags: review?(jdemooij)
Is this still the latest version? I just want to make sure I don't leave review comments for things you already fixed :)
Flags: needinfo?(sandervv)
Attached patch inline-typed-array.patch (obsolete) — — Splinter Review 
There are still some TODOs left in this patch:
 - Updating the data slot pointer in updateTypedArrayAfterMove seems a bit ugly.

 - I've commented the lines in vm/TypedArrayObject.cpp that sets the SingletonObject kind on TypedArrays. Without these comments, the hasLazyGroup() assertion failure occurs. As discussed on IRC, normally TypedArrays are singletons, but inlined typed arrays are not. (Is this right?)

 - Is it an idea to write a macro list for the Typed Array enum names and types? I'm referring to these macros:
MakeTypedArrayInstance(Int8, int8_t);                                          
MakeTypedArrayInstance(Uint8, uint8_t);                                        
MakeTypedArrayInstance(Int16, int16_t);                                        
MakeTypedArrayInstance(Uint16, uint16_t);                                      
MakeTypedArrayInstance(Int32, int32_t);                                        
MakeTypedArrayInstance(Uint32, uint32_t);                                      
MakeTypedArrayInstance(Float32, float);                                        
MakeTypedArrayInstance(Float64, double);                                       
MakeTypedArrayInstance(Uint8Clamped, uint8_clamped);
In case someone forgets to update one of the macro statements and breaks the code.

- When is MOZ_ASSERT(callInfo.constructing()); necessary to add? I've added it to the beginning of IonBuilder::inlineTypedArray(CallInfo& callInfo) but that gives a assertion failure for SM(e) and SM(r) in https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fbb5bbd4a39.
Attachment #8720282 - Attachment is obsolete: true
Attachment #8720282 - Flags: review?(jdemooij)
Flags: needinfo?(sandervv)
Attachment #8723994 - Flags: review?(jdemooij)
(In reply to Sander Mathijs van Veen from comment #5)
> - When is MOZ_ASSERT(callInfo.constructing()); necessary to add? I've added
> it to the beginning of IonBuilder::inlineTypedArray(CallInfo& callInfo) but
> that gives a assertion failure for SM(e) and SM(r) in
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fbb5bbd4a39.

That's because typed array constructors can be called without 'new': Int32Array(10). Even though that will throw a TypeError, the call *is* possible :) Just change the assert to an if-statement.
(In reply to Sander Mathijs van Veen from comment #5)
>  - I've commented the lines in vm/TypedArrayObject.cpp that sets the
> SingletonObject kind on TypedArrays. Without these comments, the
> hasLazyGroup() assertion failure occurs. As discussed on IRC, normally
> TypedArrays are singletons, but inlined typed arrays are not. (Is this
> right?)

Singletons are used for (1) very large arrays (2) typed arrays created in the global scope (or in some IIFEs in the global scope) and outside a loop (so we expect there will be only one of those). You're likely hitting the second case.

Inlining singleton typed array creation is not worth the trouble, so we should fix the code to not allocate a template object in that case (or, if that's hard, don't use it as template object).
Comment on attachment 8723994 [details] [diff] [review]
inline-typed-array.patch

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

Looks good, sorry for the delay. Posting a first round of comments that will hopefully help you finish this :)

::: js/src/jit/BaselineIC.cpp
@@ +5644,5 @@
>              return true;
>          }
>      }
>  
> +    if (args.length() == 1 && args[0].isInt32() && args[0].toInt32() >= 0) {

Can you check the function's InlinableNative field first (see MCallOptimize for how to do that). That'd avoid the checks for each typed array kind below if it's not a typed array constructor. Later we should convert the other natives here to do that too.

@@ +5978,5 @@
>                  *handled = true;
>                  return true;
>              }
> +            MOZ_ASSERT_IF(templateObject && !templateObject->hasLazyGroup(),
> +                          !templateObject->group()->maybePreliminaryObjects());

We can undo the change to this assert now right?

::: js/src/jit/MCallOptimize.cpp
@@ +2176,5 @@
>  
>  IonBuilder::InliningStatus
> +IonBuilder::inlineTypedArray(CallInfo& callInfo)
> +{
> +    MOZ_ASSERT(callInfo.constructing());

As mentioned, change this to an if to get rid of the assertion failure.

@@ +2182,5 @@
> +    if (callInfo.argc() != 1)
> +        return InliningStatus_NotInlined;
> +    if (callInfo.getArg(0)->type() != MIRType_Int32)
> +        return InliningStatus_NotInlined;
> +    if (getInlineReturnType() == MIRType_ObjectOrNull)

if (getInlineReturnType() != MIRType_Object)

@@ +2246,5 @@
> +        return InliningStatus_NotInlined;
> +
> +    obj->setFixedSlot(TypedArrayObject::BUFFER_SLOT, NullValue());
> +    obj->setFixedSlot(TypedArrayObject::LENGTH_SLOT, Int32Value(len));
> +    obj->setFixedSlot(TypedArrayObject::BYTEOFFSET_SLOT, Int32Value(0));

Why do we have to set those here, can we do this right after we create the template object? If we do it here we may get races: the background compilation thread will read these slots and the main thread may be setting those here.

::: js/src/jit/MIR.h
@@ +3266,5 @@
>    : public MUnaryInstruction,
>      public NoTypePolicy::Data
>  {
>    public:
> +    enum Mode { ObjectLiteral, ObjectCreate, TypedArray };

We should also handle this new TypedArray value in Recover.cpp

To write a test for that, maybe you can put a MOZ_CRASH in that code, run jit-tests and if you get any crashes see if you can write a similar test for typed arrays.

::: js/src/jit/MacroAssembler.cpp
@@ +897,5 @@
>      }
>  
>      allocateObject(obj, temp, allocKind, nDynamicSlots, initialHeap, fail);
>      initGCThing(obj, temp, templateObj, initContents, convertDoubleElements);
> +

Nit: remove blank line.

::: js/src/vm/TypedArrayObject.cpp
@@ +228,5 @@
>          RootedFunction ctorProto(cx, GlobalObject::getOrCreateTypedArrayConstructor(cx, global));
>          if (!ctorProto)
>              return nullptr;
>  
> +        JSFunction *fun = NewFunctionWithProto(cx, class_constructor, 3,

Nit: JSFunction* fun

@@ +235,5 @@
> +                                               ctorProto, gc::AllocKind::FUNCTION,
> +                                               SingletonObject);
> +
> +        if (fun) {
> +            fun->setJitInfo(&js::jit::JitInfo_IntrinsicTypedArray);

Nit: no {}

::: js/src/vm/TypedArrayObject.h
@@ +265,5 @@
>  };
>  
> +template <typename T>
> +js::Native
> +getTypedArrayNativeConstructor();

Nit: functions that are not class methods should start with a capital letter: GetTypedArrayNativeConstructor. Same below.

Also, it'd be great to split the changes to vm/TypedArrayObject* out to a separate patch, then you can ask :Waldo to review that one :)
Attachment #8723994 - Flags: review?(jdemooij) → feedback+
Attached patch inline-typed-array.patch (obsolete) — — Splinter Review 
This patch is passing the jit tests and jsreftests (previous try server job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a82d8468b11), but Jan asked me to run another try server job because I've changed some small parts in the patch in the mean time.

The new test results will be here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f65ea3ac041

Next patch will work on inlining the constructor of large typed arrays and/or with a buffer.
Attachment #8723994 - Attachment is obsolete: true
Attachment #8730643 - Flags: review?(jwalden+bmo)
Attachment #8730643 - Flags: review?(jdemooij)
Attached patch inline-typed-array.patch (obsolete) — — Splinter Review 
Fixed the patch for arm64. I used an assembler instruction in MacroAssembler::initGCThing that was not available for arm64. Tests are now passing for all architectures:
 - https://treeherder.mozilla.org/#/jobs?repo=try&revision=581e401e1955&group_state=expanded
 - https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f8d3d7c0ba4&group_state=expanded

The jit1 failure in the second try job is caused by a timeout.
Attachment #8730643 - Attachment is obsolete: true
Attachment #8730643 - Flags: review?(jwalden+bmo)
Attachment #8730643 - Flags: review?(jdemooij)
Attachment #8730853 - Flags: review?(jwalden+bmo)
Attachment #8730853 - Flags: review?(jdemooij)
Comment on attachment 8730853 [details] [diff] [review]
inline-typed-array.patch

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

Sorry for the delay. This looks great for the most part, but some suggestions below to simplify the code a bit more.

::: js/src/jit/BaselineIC.cpp
@@ +5648,5 @@
>              return true;
>          }
>      }
>  
> +    if (args.length() == 1 && args[0].isInt32() && args[0].toInt32() >= 0) {

As a simple optimization, please test args.isConstructing() before the other checks (most calls are not constructing).

::: js/src/jit/CodeGenerator.cpp
@@ +4996,5 @@
>      // that derives its class from its prototype instead of being
>      // PlainObject::class_'d) from self-hosted code, we need a different init
>      // function.
> +    switch (mode_) {
> +    case MNewObject::ObjectLiteral:

Nit: fix indentation, see other review comment.

::: js/src/jit/InlinableNatives.h
@@ +108,5 @@
>      _(IntrinsicIsMapIterator)       \
>      _(IntrinsicIsStringIterator)    \
>      _(IntrinsicIsListIterator)      \
>                                      \
> +    _(IntrinsicTypedArray)          \

It's not an intrinsic (a helper function for self-hosted code), so let's just call this TypedArray or TypedArrayConstructor

::: js/src/jit/MCallOptimize.cpp
@@ +2216,5 @@
> +    uint32_t len = arg->maybeConstantValue()->toInt32();
> +    JSObject* templateObject = nullptr;
> +
> +    // Large typed arrays have a separate buffer object, while small arrays have their values
> +    // stored inline.

Nit: comments should fit in 80 columns

@@ +2234,5 @@
> +#undef MakeGetTypedArrayConstructor
> +
> +    if (false) {
> +#define MakeGetTypedArrayConstructor(E, T) \
> +    } else if ((templateObject = inspector->getTemplateObjectForNative(pc, native##T))) { \

Hm I think this is more complicated than necessary and it might also be subtly wrong. What I'd do is:

(1) Add a |JSNative native| argument to this function, the caller can pass target->native().

(2) Then do:

JSObject* templateObject = inspector->getTemplateObjectForNative(pc, native);

(3) Below, use obj->bytesPerElement() instead of sizeof(T).

I think that will let you remove the two #defines here.

@@ +2249,5 @@
> +#undef MakeGetTypedArrayConstructor
> +    }
> +
> +    // Negative lengths generate a RangeError, unhandled by the inline path.
> +    if (len > INT32_MAX)

Let's also check that our |len| value matches the template object's length. That's not guaranteed to be true when we're inlining (see IonBuilder::inlineArray comment)

::: js/src/jit/MacroAssembler.cpp
@@ +1109,5 @@
>                      Address(obj, elementsOffset + ObjectElements::offsetOfFlags()));
>              MOZ_ASSERT(!ntemplate->hasPrivate());
>          } else {
> +
> +            // TODO is this comment still accurate?

You could add this assert immediately after this comment:

MOZ_ASSERT(!ntemplate->isSharedMemory());

And fix the comment's last sentence.

@@ +1122,5 @@
>  
> +            if (ntemplate->is<TypedArrayObject>()) {
> +                TypedArrayObject* ttemplate = &ntemplate->as<TypedArrayObject>();
> +
> +                if (ntemplate->hasPrivate()) {

AFAICS this is always true for typed arrays. Can you assert it instead?

@@ +1124,5 @@
> +                TypedArrayObject* ttemplate = &ntemplate->as<TypedArrayObject>();
> +
> +                if (ntemplate->hasPrivate()) {
> +                    uint32_t nfixed = ntemplate->numFixedSlots();
> +                    size_t dataOffset = NativeObject::getPrivateDataOffset(nfixed);

Can you use dataOffset = TypedArrayObject::dataOffset() here?

@@ +1136,5 @@
> +                        MOZ_ASSERT(offset + n <= JSObject::MAX_BYTE_SIZE);
> +
> +                        // Add one more 64-bit store for non 8 bytes aligned allocations
> +                        static_assert(sizeof(HeapSlot) == 8, "Assumed 8 bytes alignment");
> +                        size_t quad_words = n / 8 + ((n & 7) > 0);

I think it'd be simpler to calculate the number of words you have to zero, and then use storePtr(ImmWord(0), Address(...)).

That way we don't need separate code for 32-bit vs 64-bit.

::: js/src/jit/Recover.cpp
@@ +1202,5 @@
>      JSObject* resultObject = nullptr;
>  
>      // See CodeGenerator::visitNewObjectVMCall
> +    switch (mode_) {
> +    case MNewObject::ObjectLiteral:

Nit: indent case labels with 2 spaces, and the case bodies with 2 more, like this:

    switch (mode_) {
      case MNewObject..:
        resultObject = ..;
        break;

Also, we still need a test for this.

::: js/src/jit/VMFunctions.h
@@ +12,5 @@
>  #include "jspubtd.h"
>  
>  #include "jit/CompileInfo.h"
>  #include "jit/JitFrames.h"
> +#include "vm/TypedArrayObject.h"

We don't need this #include if we forward-declare TypedArrayObject (add |class TypedArrayObject;| below, after GeneratorObject).
Attachment #8730853 - Flags: review?(jdemooij)
Comment on attachment 8730853 [details] [diff] [review]
inline-typed-array.patch

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

Sorry for the mega-delay here.  :-(

::: js/src/jit/BaselineIC.cpp
@@ +5655,5 @@
> +        if (native == GetTypedArrayNativeConstructor<T>() &&  \
> +                len <= TypedArrayObject::INLINE_BUFFER_LIMIT / sizeof(T)) { \
> +            res.set(MakeTypedArrayObject<T>(cx, len)); \
> +            return !!res; \
> +        }

This entire block, tests included, seems like it should be an extern bool in TypedArrayObject.* -- cleaner in a number of ways.

::: js/src/jit/CodeGenerator.cpp
@@ +5013,5 @@
>          callVM(ObjectCreateWithTemplateInfo, lir);
> +    break;
> +    case MNewObject::TypedArray:
> +        pushArg(ImmGCPtr(templateObject));
> +        callVM(TypedArrayCreateWithTemplateInfo, lir);

This is dumb, isn't it?  Why not push element kind and length, since those are the things actually used?  Avoid a couple dereferences, one less GC thing to keep alive, etc.

Please improve this in a quick followup patch.  But you can move with this for now, so that we lock in the improvement.

::: js/src/jit/MCallOptimize.cpp
@@ +2207,5 @@
> +        return InliningStatus_NotInlined;
> +    if (getInlineReturnType() != MIRType_Object)
> +        return InliningStatus_NotInlined;
> +
> +    MDefinition *arg = callInfo.getArg(0);

Check return type first, then check argc, then have the decl of getArg(0), then type-check that.

@@ +2212,5 @@
> +
> +    if (!arg->maybeConstantValue())
> +        return InliningStatus_NotInlined;
> +
> +    uint32_t len = arg->maybeConstantValue()->toInt32();

Seems like the exit if this is negative should happen here:

    // Negative lengths must throw a RangeError.  (We don't track that this
    // might have previously thrown, when determining whether to inline, so we
    // have to deal with this error case when inlining.)
    int32_t providedLen = arg->maybeConstantValue()->toInt32();
    if (providedLen < 0)
        return InliningStatus_NotInlined;

    uint32_t len = AssertedCast<uint32_t>(providedLen);

Then |len| can be used normally the rest of the way.

@@ +2234,5 @@
> +#undef MakeGetTypedArrayConstructor
> +
> +    if (false) {
> +#define MakeGetTypedArrayConstructor(E, T) \
> +    } else if ((templateObject = inspector->getTemplateObjectForNative(pc, native##T))) { \

Concur with jandem on the complicatedness and better idea.

For future note, if not here.  If we actually were to do something like this, it'd be better to have a higher-order macro for use both places:

#define JS_FOR_EACH_TYPED_ARRAY(macro) \
  macro(Int8, int8_t) \
  macro(Uint8, uint8_t) \
  ... \
  macro(Uint8Clamped, uint8_clamped)

so that you could have

#define MAKE_CTOR(name, type) \
  Native native##t = GetTypedArrayNativeConstructor<type>();
JS_FOR_EACH_TYPED_ARRAY(MAKE_CTOR)
#undef MAKE_CTOR

or so.

@@ +2249,5 @@
> +#undef MakeGetTypedArrayConstructor
> +    }
> +
> +    // Negative lengths generate a RangeError, unhandled by the inline path.
> +    if (len > INT32_MAX)

This not-guaranteed-to-match thing should have a test that fails without such a check.  Ask jandem for how to write one, as this bit of JIT knowledge isn't my strong suit.

@@ +2262,5 @@
> +    TypedArrayObject* obj = &templateObject->as<TypedArrayObject>();
> +
> +    // Buffers are not supported yet!
> +    if (createBuffer)
> +        return InliningStatus_NotInlined;

This is weird, delaying testing of a computation until after subsequent computation -- compute template object, compute createBuffer, check length, check template object, check createBuffer.  I'd prefer if we computed something, then tested it, computed something else, tested something else, etc.  Not immediately sure how to structure the code to do that.

::: js/src/jit/MacroAssembler.cpp
@@ +1120,5 @@
>  
>              initGCSlots(obj, temp, ntemplate, initContents);
>  
> +            if (ntemplate->is<TypedArrayObject>()) {
> +                TypedArrayObject* ttemplate = &ntemplate->as<TypedArrayObject>();

Could you use templateObj for the name, so it's more readable/less mistypable?

@@ +1129,5 @@
> +                    computeEffectiveAddress(Address(obj, dataOffset + sizeof(HeapSlot)), temp);
> +                    storePtr(temp, Address(obj, dataOffset));
> +
> +                    // Initialise inline data elements to zero
> +                    if (!ttemplate->hasBuffer()) {

If we're here, aren't we inlining creation?  And if we're inlining creation, doesn't that mean we have a template object that we created, and we only create template objects without a buffer?  Seems like this too should be asserted.

@@ +1152,5 @@
> +                            store32(temp, Address(obj, offset + i * sizeof(HeapSlot) + 4));
> +                        }
> +#endif
> +                    } else {
> +                        MOZ_CRASH("unreachable: typed arrays with buffers are not inlined");

Oh, so you have the check because of quasi-paranoia.  Just assert.  We have tests.  We should only be crashing for things that we think we don't understand well enough to assert, and this we understand.

::: js/src/vm/TypedArrayObject.cpp
@@ +437,5 @@
> +    makeTemplateObject(JSContext* cx, uint32_t len)
> +    {
> +        RootedObject proto(cx);
> +        if (!GetPrototypeForInstance(cx, /*newTarget*/nullptr, &proto))
> +            return nullptr;

This is a bit dumb.  GetPrototypeForInstance straightforwardly nulls |proto| when |!newTarget|, so this if (!) ... does nothing.

@@ +440,5 @@
> +        if (!GetPrototypeForInstance(cx, /*newTarget*/nullptr, &proto))
> +            return nullptr;
> +
> +        const Class* clasp = instanceClass();
> +        gc::AllocKind allocKind = AllocKindForLazyBuffer(len * sizeof(NativeType));

This isn't the same as what TypedArrayObjectTemplate::makeInstance does, so I guess this implicitly is asserting there's no |proto|, which is to say the template object *never* uses out-of-line storage.  I'd like to see this explicitly asserted -- that'd also let you remove the assertion of such from at least one caller.

@@ +450,5 @@
> +        if (!GetBuiltinPrototype(cx, JSCLASS_CACHED_PROTO_KEY(clasp), &checkProto))
> +            return nullptr;
> +
> +        AutoSetNewObjectMetadata metadata(cx);
> +        MOZ_ASSERT_IF(proto, proto == checkProto);

And so if |!proto|, this never asserts anything.  (The more I read, the more I suspect you just copied the existing code path for class_constructor and then minimized it.  :-)  It's a good approach [IMO], you just need to do it a little harder.)

And if that never asserts anything, then |checkProto| is never checked.  And since that's the case, the outparam of the |GetBuiltinPrototype| is never used.  And then |checkProto| is a useless variable.

In other words, can't the whole start of this be simplified to:

static TypedArrayObject*
makeTemplateObject(JSContext* cx, uint32_t len)
{
    MOZ_ASSERT(len * sizeof(NativeType) <= inline storage space);
    gc::AllocKind allocKind = AllocKindForLazyBuffer(len * sizeof(NativeType));

    AutoSetNewObjectMetadata metadata(cx);
    MOZ_ASSERT(len * sizeof(NativeType) < TypedArrayObject::SINGLETON_BYTE_LENGTH);

    ....

@@ +454,5 @@
> +        MOZ_ASSERT_IF(proto, proto == checkProto);
> +        MOZ_ASSERT(len * sizeof(NativeType) < TypedArrayObject::SINGLETON_BYTE_LENGTH);
> +
> +        jsbytecode* pc;
> +        RootedScript script(cx, cx->currentScript(&pc));

Erm, |pc| and |script| are both unused other than in these lines, so why have these two lines?

@@ +460,5 @@
> +        if (!tmp)
> +            return nullptr;
> +
> +        Rooted<TypedArrayObject*> obj(cx);
> +        obj = &tmp->as<TypedArrayObject>();

Rooted<TypedArrayObject*> obj(cx, &tmp->as<TypedArrayObject>());

@@ +466,5 @@
> +        void* data = obj->fixedData(FIXED_DATA_START);
> +        obj->initPrivate(data);
> +        memset(data, 0, len * sizeof(NativeType));
> +
> +        obj->setFixedSlot(TypedArrayObject::BUFFER_SLOT, ObjectOrNullValue(nullptr));

NullValue()

@@ +467,5 @@
> +        obj->initPrivate(data);
> +        memset(data, 0, len * sizeof(NativeType));
> +
> +        obj->setFixedSlot(TypedArrayObject::BUFFER_SLOT, ObjectOrNullValue(nullptr));
> +        obj->setFixedSlot(TypedArrayObject::LENGTH_SLOT, Int32Value(len));

Int32Value(AssertedCast<int32_t>(len))

and add #include "mozilla/Casting.h"/using mozilla::AssertedCast; if it's not at the top of this file.

@@ +756,5 @@
>  
>  } /* anonymous namespace */
>  
> +static void
> +updateTypedArrayAfterMove(JSObject* obj, const JSObject* old)

InterCaps for standalone static function naming, please.

I confess that I don't understand what this function does/should do, so unless jandem verified this, you need a GGC person to look at this.

@@ +793,5 @@
> +GetTypedArrayNativeConstructorDefinition(int32_t);
> +GetTypedArrayNativeConstructorDefinition(uint32_t);
> +GetTypedArrayNativeConstructorDefinition(float);
> +GetTypedArrayNativeConstructorDefinition(double);
> +GetTypedArrayNativeConstructorDefinition(uint8_clamped);

Yeah, you're doing enough repeat-for-all-typed-array things you really should add that JS_FOR_EACH macro I mentioned elsewhere here, to TypedArrayObject.h.

@@ +798,5 @@
> +#undef GetTypedArrayNativeConstructorDefinition
> +
> +template <typename T>
> +TypedArrayObject*
> +js::MakeTypedArrayObject(JSContext* cx, size_t len)

This method name doesn't communicate that it creates a *template* object, which confused me briefly for various reasons.  Make this MakeTypedArrayTemplateObject to clarify that.

@@ +800,5 @@
> +template <typename T>
> +TypedArrayObject*
> +js::MakeTypedArrayObject(JSContext* cx, size_t len)
> +{
> +    MOZ_ASSERT(len <= TypedArrayObject::INLINE_BUFFER_LIMIT / sizeof(T));

Let's add a reason-string to this:

  MOZ_ASSERT(...,
             "typed array template objects can only be used to allocate typed "
             "array objects with inline elements");

@@ +827,5 @@
> +    MOZ_ASSERT(templateObj->is<TypedArrayObject>());
> +    TypedArrayObject* obj = &templateObj->as<TypedArrayObject>();
> +    size_t len = obj->length();
> +
> +    switch (obj->type()) {

I'm not really a fan at all of using template objects as infrastructure for this.  :-\  The relevant information to save, to inline typed array creation, is the element-kind and the length.  We only really need one template object per element kind, not one per inlined allocation site.

But it doesn't appear simple to disentangle this from that, so okay for now.  Would be nice if we followed up to make this simplification, tho.

@@ +2083,4 @@
>  }
>  
> +
> +

We generally put a single blank line between things, so don't do this.

::: js/src/vm/TypedArrayObject.h
@@ +263,5 @@
>  
>      static bool set(JSContext* cx, unsigned argc, Value* vp);
>  };
>  
> +template <typename T>

Making this instead be js::Scalar might be better, as that would limit to *only* types that are permissible typed array element types.  And for the rest of these.
Attachment #8730853 - Flags: review?(jwalden+bmo)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #18)
> > +static void
> > +updateTypedArrayAfterMove(JSObject* obj, const JSObject* old)
> 
> I confess that I don't understand what this function does/should do, so
> unless jandem verified this, you need a GGC person to look at this.

This is called when an object is moved by the GC so that any pointers that the GC doesn't know about can be updated.

This is the right thing to do here so that the internal data pointer is updated.
Attached patch inline-typed-array.patch (obsolete) — — Splinter Review 
I've updated the patch with :Waldo's suggestions. 

I had to return GenericObject in useSingletonForAllocationSite() for Typed Arrays. Otherwise, a SingletonObject was returned that in turn will have a lazy group which causes problems in the MCallOptimize and MacroAssembler (e.g. setting the group in the generated code, since the group is not constructed). There was a comment in that function that said:

| Objects created outside loops in global and eval scripts should have
| singleton types. For now this is only done for plain objects and typed
| arrays, but not normal arrays.

I changed the last sentence to "For now this is only done for plain objects, but not for typed arrays nor normal arrays." since I'm not understanding why normal arrays are omitted while some typed arrays are supposed to be singletons.

The disadvantage could be that there is a performance hit caused by changing to the generic object newkind, but I think it is good to measure this instead of assuming it is. Is it possible to land this patch and see if it regresses any performance benchmark?
Attachment #8730853 - Attachment is obsolete: true
Attachment #8744854 - Flags: review?(jdemooij)
FYI All jit-tests and jsreftests for linux {opt,debug} and {arm,x86,x86-64} have passed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=329d32ed0779
Comment on attachment 8744854 [details] [diff] [review]
inline-typed-array.patch

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

Looks great, r=me (with nits below addressed) on everything except vm/TypedArrayObject.* Let's ask Waldo to look at that, as he reviewed the previous version.

::: js/src/jit/BaselineIC.cpp
@@ +5647,5 @@
>              return true;
>          }
>      }
>  
> +    if (args.length() == 1 && args[0].isInt32() && args[0].toInt32() >= 0) {

As I mentioned in the previous review, as a simple optimization, check args.isConstructing() first (most calls are not constructing).

::: js/src/jit/MCallOptimize.cpp
@@ +2363,5 @@
> +        return InliningStatus_NotInlined;
> +    if (callInfo.getArg(0)->type() != MIRType_Int32)
> +        return InliningStatus_NotInlined;
> +
> +    MDefinition *arg = callInfo.getArg(0);

Nit: * goes next to the type, MDefinition* arg

@@ +2368,5 @@
> +
> +    if (!arg->maybeConstantValue())
> +        return InliningStatus_NotInlined;
> +
> +    JSObject *templateObject = inspector->getTemplateObjectForNative(pc, native);

And here.

::: js/src/jit/MacroAssembler.cpp
@@ +1122,5 @@
> +                MOZ_ASSERT(ntemplate->hasPrivate());
> +                MOZ_ASSERT(!ttemplate->hasBuffer());
> +
> +                size_t dataOffset = TypedArrayObject::dataOffset();
> +                computeEffectiveAddress(Address(obj, dataOffset + sizeof(HeapSlot)), temp);

It can't hurt to add:

static_assert(TypedArrayObject::FIXED_DATA_START == TypedArrayObject::DATA_SLOT + 1,
              "Fixed data must be stored after the dataOffset slot");

@@ +1136,5 @@
> +                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)));
> +                }

Nit: no {}. This code looks very nice btw!

::: js/src/vm/ObjectGroup.cpp
@@ +210,1 @@
>       */

I think we should revert the changes to this file and not optimize when we see a template object with a singleton type (optimizing that is not interesting because it hits at most once).

You could check template->isSingleton() in BaselineIC.cpp or in MCallOptimize.cpp
Attachment #8744854 - Flags: review?(jwalden+bmo)
Attachment #8744854 - Flags: review?(jdemooij)
Attachment #8744854 - Flags: review+
Comment on attachment 8744854 [details] [diff] [review]
inline-typed-array.patch

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

It appears that at least some of my review comments on the last patch weren't dealt with -- please do so and request review on the updated patch.  Various other parts of the patch look fine, but it's certainly not ready as-is.

::: js/src/vm/TypedArrayObject.cpp
@@ +440,5 @@
> +    makeTemplateObject(JSContext* cx, uint32_t len, NewObjectKind newKind)
> +    {
> +        RootedObject proto(cx);
> +        if (!GetPrototypeForInstance(cx, /*newTarget*/nullptr, &proto))
> +            return nullptr;

My last review commented that this was unnecessary -- GPFI would do |proto.set(nullptr)| and return true for these arguments, so no reason to call it.  Looking further on, I see (at least some of the) other comments I had on this method also weren't covered.  Please address those comments and request review on an updated patch.

@@ +838,5 @@
> +MakeTypedArrayDefinition(int8_t);
> +MakeTypedArrayDefinition(uint8_t);
> +MakeTypedArrayDefinition(int16_t);
> +MakeTypedArrayDefinition(uint16_t);
> +MakeTypedArrayDefinition(int32_t);

As noted before, I see enough cases of a single macro enumerating all typed array element types that you should add a higher-order macro for it.  See my last review for how.

::: js/src/vm/TypedArrayObject.h
@@ +275,5 @@
> +TypedArrayObject*
> +MakeTypedArrayObject(JSContext* cx, size_t len);
> +
> +TypedArrayObject*
> +TypedArrayCreateWithTemplate(JSContext* cx, HandleObject templateObj);

extern TypedArrayObject*
TypedArrayCreateWithTemplate...
Attachment #8744854 - Flags: review?(jwalden+bmo) → review-
Attached patch inline-typed-array.patch (obsolete) — — Splinter Review 
Changed the patch based on your suggestions. Looks much cleaner now!

I have two patches for the large and non-constant sized typed arrays, which I'll attach to two separate new bug reports.
Attachment #8744854 - Attachment is obsolete: true
Attachment #8758194 - Flags: review?(jwalden+bmo)
Blocks: 1276955
Attached patch inline-typed-array.patch (obsolete) — — Splinter Review 
Rebased patch to trunk
Attachment #8758194 - Attachment is obsolete: true
Attachment #8758194 - Flags: review?(jwalden+bmo)
Attachment #8762539 - Flags: review?(jwalden+bmo)
Blocks: 1279992
Waldo, review ping.
Bah, I was hoping to review this today but somewhat ran out of time -- and I seem to have picked up a headache as well (probably dehydration).  I'll review this (and the other typed array inlining patches) on trains tomorrow to Paris.
Comment on attachment 8762539 [details] [diff] [review]
inline-typed-array.patch

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

I'm not sure whether I'll get to the other two reviews today or not -- if not, it's happening Monday.

::: js/src/jit/MacroAssembler.cpp
@@ +1135,5 @@
>              initGCSlots(obj, temp, ntemplate, initContents);
>  
> +            if (ntemplate->is<TypedArrayObject>()) {
> +                TypedArrayObject* ttemplate = &ntemplate->as<TypedArrayObject>();
> +                MOZ_ASSERT(ntemplate->hasPrivate());

Maybe rather than assert this here...

@@ +1138,5 @@
> +                TypedArrayObject* ttemplate = &ntemplate->as<TypedArrayObject>();
> +                MOZ_ASSERT(ntemplate->hasPrivate());
> +                MOZ_ASSERT(!ttemplate->hasBuffer());
> +
> +                size_t dataOffset = TypedArrayObject::dataOffset();

Have

  size_t dataSlotOffset = TypedArrayObject::dataOffset();

  static_assert(TypedArrayObject::FIXED_DATA_START == TypedArrayObject::DATA_SLOT + 1,
                "fixed inline element data assumed to begin after the data slot");
  size_t dataOffset = dataSlotOffset + sizeof(HeapSlot);

and then

  computeEffectiveAddress(Address(obj, dataOffset), temp);
  storePtr(temp, Address(obj, dataSlotOffset));

  // Initialize inline data elements to zero.
  ...

@@ +1145,5 @@
> +
> +                static_assert(TypedArrayObject::FIXED_DATA_START == TypedArrayObject::DATA_SLOT + 1,
> +                              "Fixed data must be stored after the dataOffset slot");
> +
> +                // Initialise inline data elements to zero

Period at end of complete sentence.

@@ +1146,5 @@
> +                static_assert(TypedArrayObject::FIXED_DATA_START == TypedArrayObject::DATA_SLOT + 1,
> +                              "Fixed data must be stored after the dataOffset slot");
> +
> +                // Initialise inline data elements to zero
> +                size_t offset = dataOffset + sizeof(HeapSlot);

|offset| can be replaced by |dataOffset|, given the changes above.

@@ +1150,5 @@
> +                size_t offset = dataOffset + sizeof(HeapSlot);
> +                size_t n = ttemplate->length() * ttemplate->bytesPerElement();
> +                MOZ_ASSERT(offset + n <= JSObject::MAX_BYTE_SIZE);
> +
> +                // Add one more 64-bit store for non 8 bytes aligned allocations

Maybe instead,

                // Write enough zero pointers into fixed data to zero every
                // element.  (This zeroes past the end of a byte count that's
                // not a multiple of pointer size.  That's okay, because fixed
                // data is a count of 8-byte HeapSlots (i.e. <= pointer size),
                // and we won't inline unless the desired memory fits in that
                // space.)
                static_assert(..., "...");

And *please* don't use "word" as a name, because of Intel making word not mean "pointer-sized".  (Yes, our JIT is bad about this.  *grmbls*)  Maybe instead,

    size_t numZeroPointers = ((n + 7) & ~0x7) / sizeof(char*);
    for (size_t i = 0; i < numZeroPointers; i++)
        storePtr(ImmWord(0), Address(obj, dataOffset + i * sizeof(char*)));

And speaking of which: this loop's zeroing "words", i.e. pointers, sufficient to cover all elements, but you're looping over sizeof(HeapSlot) in the code in the patch, which is *double* pointer size on 32-bit.  So isn't this code wrong on 32-bit, and don't you need to use sizeof(char*) instead as my replacement above does?  Please do a 32-bit build and verify we have a test that (usually) crashes with the code as currently/wrongly written.

(Also, don't brace single-line loops.)

::: js/src/vm/TypedArrayObject.cpp
@@ +186,5 @@
> +macro(int32_t, Int32) \
> +macro(uint32_t, Uint32) \
> +macro(float, Float32) \
> +macro(double, Float64) \
> +macro(uint8_clamped, Uint8Clamped) \

#define JS_FOR_EACH_TYPED_ARRAY(macro) \
    macro(int8_t, Int8) \
    macro(uint8_t, Uint8) \
    ....
    macro(double, Float64) \
    macro(uint8_clamped, Uint8Clamped)

@@ +245,5 @@
> +                                               ctorProto, gc::AllocKind::FUNCTION,
> +                                               SingletonObject);
> +
> +        if (fun)
> +            fun->setJitInfo(&js::jit::JitInfo_TypedArrayConstructor);

You could leave off js:: here, I think.

@@ +479,5 @@
> +        {
> +            return nullptr;
> +        }
> +
> +        Rooted<TypedArrayObject*> obj(cx, &tmp->as<TypedArrayObject>());

Possibly just

  TypedArrayObject* tarray = &tmp->as<TypedArrayObject>();

would be better than rooting this against no actions that can GC.  (Definitely change the name, to clarify the object type in all the subsequent code.)

@@ +489,5 @@
> +        obj->setFixedSlot(TypedArrayObject::BUFFER_SLOT, NullValue());
> +        obj->setFixedSlot(TypedArrayObject::LENGTH_SLOT, Int32Value(AssertedCast<int32_t>(len)));
> +        obj->setFixedSlot(TypedArrayObject::BYTEOFFSET_SLOT, Int32Value(0));
> +
> +        // Verify that the private slot is at the expected place

Period at end of complete sentence.

@@ +786,5 @@
>      static Value getIndexValue(JSObject* tarray, uint32_t index);
>  };
>  
> +#define CREATE_TYPE_FOR_TYPED_ARRAY(T, N) \
> +typedef TypedArrayObjectTemplate<T> N##Array;

Indent this four spaces.

@@ +805,5 @@
> +    // supported yet!
> +    if (oldObj->hasBuffer())
> +        return;
> +
> +    // Update the data slot pointer if it points to the old JSObject

Period at end of complete sentence.

@@ +824,5 @@
> +    switch (obj->type()) {
> +#define CREATE_TYPED_ARRAY(T, N) \
> +    case Scalar::N: \
> +        return TypedArrayObjectTemplate<T>::makeTemplateObject(cx, len, GenericObject); \
> +    break;

Remove the break; here (no need for it after a return), and indent the |case| by a further two spaces, to be consistent with SpiderMonkey's normal switch-formatting.

@@ +827,5 @@
> +        return TypedArrayObjectTemplate<T>::makeTemplateObject(cx, len, GenericObject); \
> +    break;
> +JS_FOR_EACH_TYPED_ARRAY(CREATE_TYPED_ARRAY)
> +#undef CREATE_TYPED_ARRAY
> +    default:

default: should also be indented two spaces.

@@ +1120,5 @@
> +{
> +#define CHECK_TYPED_ARRAY_CONSTRUCTOR(T, N) \
> +    if (native == &TypedArrayObjectTemplate<T>::class_constructor &&  \
> +            len <= TypedArrayObject::INLINE_BUFFER_LIMIT / sizeof(T)) { \
> +        res.set(TypedArrayObjectTemplate<T>::makeTemplateObject(cx, len, TenuredObject)); \

Like so:

#define ...(T, N) \
    if (native == .... && \
        len <= ... / ...)) \
    { \
        res.set(...); \
        return true; \
    }

@@ +2303,5 @@
>      nullptr,                 /* construct   */
>      TypedArrayObject::trace, /* trace  */
>  };
>  
> +static const ClassExtension TypedArrayClassExtention = {

TypedArrayClassExtension

@@ +2341,5 @@
>      JSCLASS_HAS_CACHED_PROTO(JSProto_##_type##Array) |                         \
>      JSCLASS_DELAY_METADATA_BUILDER,                                            \
>      &TypedArrayClassOps,                                                       \
> +    &TypedArrayObjectClassSpecs[Scalar::Type::_type],                          \
> +    &TypedArrayClassExtention                                                  \

TypedArrayClassExtension
Attachment #8762539 - Flags: review?(jwalden+bmo) → review+
Attached patch inline-typed-array.patch (obsolete) — — Splinter Review 
Fixed Waldo's comments.

The jit-tests runs successfully on 32-bit with the |sizeof(HeapSlot)| to |sizeof(char *)| changed address increment:
$ python ./jit-test/jit_test.py --tbpl -j 10 ~/work/leaningtech/gecko-dev-opt-32/js/src/js
[25962|    0|    0|    0] 100% ======================================>| 282.6s
PASSED ALL
Attachment #8762539 - Attachment is obsolete: true
Attached patch inline-typed-array.patch (obsolete) — — Splinter Review 
Added reviewer names to commit message.
Attachment #8765828 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48e767f0a73f
Inline typed array constructors r=jandem r=Waldo
Keywords: checkin-needed
sorry backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=30779268&repo=mozilla-inbound
Flags: needinfo?(sandervv)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/346559e02035
Backed out changeset 48e767f0a73f for SM-Tc failures
Attached patch inline-typed-array.patch (obsolete) — — Splinter Review 
I've backported the fix for ObjectGroup::useSingletonForNewObject that is causing the assertion error |!hasLazyGroup()|. That fix was already in use for the large and non-compile-time known sized typed arrays, and therefore did not fail during the try jobs.

A new try build is currently pending for the backported fix.
Attachment #8765854 - Attachment is obsolete: true
Flags: needinfo?(sandervv)
All tests have passed with the backported fix (see last URL for treeherder results). I'm adding the 'checkin-needed' keyword now.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c34ec3e0884c
Inline typed array constructors r=jandem r=Waldo
Keywords: checkin-needed
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/633d41ede644
Backed out changeset c34ec3e0884c for hazard failures
This is the correct patch. I accidentally attached a previous version of the patch that did not contain the fix for the hazard failure.
Attachment #8765913 - Attachment is obsolete: true
Flags: needinfo?(sandervv)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4af8f332bc98
Inline typed array constructors r=jandem r=Waldo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4af8f332bc98
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Seeing an improvement on AWFY:

misc 0.7 bugs-608733-interpreter linux32 opt shell    8.33% 	
misc 0.7 bugs-608733-interpreter linux64 opt shell    6.77%
misc 0.7 f32-exp linux32 opt shell                    3.15%
misc 0.7 f32-exp linux64 opt shell                    3.28%

(These small tests are no official tests, but I don't think we have 'official' benchmarks that use typed array.)
Depends on: 1305948
Depends on: 1369994
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: