836005 - BaselineCompiler: Optimize typed array getelem & length

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

Attachment: typed array length

I think the length patch is simple enough. For the getelem patch, there is one annoyance with Uint32Arrays. Because I can't know if TI has seen a double type or just int, i just bail out for values that would require a double to represent. (This is the false in loadFromTypedArray). I am giving the later patch to djvj so we have some fair split.

Comment 1

[Tom S [:evilpie]]

Attachment: typed array element

Comment 2

[Kannan Vijayan [:djvj]]

Comment on attachment 707800 [details] [diff] [review]
typed array element

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

Nice work.

::: js/src/ion/BaselineIC.cpp
@@ +1644,5 @@
>              return false;
>  
>          stub->addNewStub(denseStub);
>      }
> +    if (obj->isTypedArray() && rhs.isInt32() && res.isNumber()) {

Nit: Make this into an "else if" attached to the previous if.  Makes it clear that these two cases are exclusive.

@@ +1742,5 @@
> +    Register key = masm.extractInt32(R1, ExtractTemp1);
> +
> +    // Bounds check.
> +    masm.unboxInt32(Address(obj, TypedArray::lengthOffset()), scratchReg);
> +    masm.branch32(Assembler::BelowOrEqual, scratchReg, key, &failure);

Need to check if (key >= 0) here.

@@ +1751,5 @@
> +    // Load the value.
> +    BaseIndex source(scratchReg, key, ScaleFromElemWidth(TypedArray::slotWidth(type_)));
> +    masm.loadFromTypedArray(type_, source, R0, false, scratchReg, &failure);
> +
> +    // Todo: type check for uint32 array

Nit: Can remove this TODO.  The GetElementMonitored will take care of the typeset updates.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 707798 [details] [diff] [review]
typed array length

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

::: js/src/ion/BaselineIC.cpp
@@ +1988,5 @@
>          *attached = true;
>          stub->addNewStub(newStub);
>          return true;
>      }
> +    if (obj->isTypedArray() && res.isInt32()) {

Nit: JS_ASSERT(res.isInt32()) inside the |if| instead (unlike the array length, it can never be a double)

Comment 4

[Tom S [:evilpie]]

https://hg.mozilla.org/projects/ionmonkey/rev/e9961afdfcaf
https://hg.mozilla.org/projects/ionmonkey/rev/71e998c2430f

I am going to keep this open, because I need a similar patch like bug 836953.

Comment 5

[Tom S [:evilpie]]

Attachment: don't attach multiple stub

Comment 6

[Tom S [:evilpie]]

Great!
https://hg.mozilla.org/projects/ionmonkey/rev/b7e4d01b541e