802869 - IonMonkey: Fast path for LIn for Int32 in dense array

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Bug 801915 implemented support for JSOP_IN using callVM, mirroring existing functionality in JM, with the exception of a single fast-path for int32 lookups in dense arrays.

The original JM bug for this is Bug 664824.

Comment 1

[Sean Stangl [:sstangl]]

Note that such a fast-path for IM would involve MInitializedLength and some new MBoundsCheck-like opcode, not more stuff in LIn codegen as in JM.

Comment 2

[Till Schneidereit [:till]]

Attachment: Test-case: Reduced version of self-hosted Array#forEach

This test-case shows that we need the fast path to efficiently compile the self-hosted Array extras in IM. Once bug 784291 is fixed, we will compile them, and in many cases get slower than we are right now.

Time in the shell: ~295ms
Time with --no-ion: ~62ms

Comment 3

[Tom S]

Hey, I have some free time again. I am going to do this!

Comment 4

[Tom S]

Attachment: v1

This was pretty easy to do with our current infrastructure. Jandem: please check if I got the typeset & resume stuff right, my last ion work has been some time now.
I will have a look into what needs to be test for this.

Times for testcase from comment #2.
JM: 52
JM/Ion before: 274
JM/Ion after: 27

So an easy 2x improvement over JM.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 682781 [details] [diff] [review]
v1

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

Looks great, thanks! r=me with nits fixed. Please make sure the tests below pass and add them to the patch:

function test1() {
    var dense = [1, 2, 3];
    var denseHoles = [1, , 3];
    var result = 0;
    for (var i=0; i<70; i++) {
        if (i in dense) result += 1;
        if (1 in dense) result += 2;
        if (3 in dense) result += 3;
        if (-1000 in dense) result += 4;
        if (i in denseHoles) result += 5;
        if (1 in denseHoles) result += 6;
    }
    assertEq(result, 153);
}
test1();

function test2() {
    var a = [1, 2, 3];
    for (var i=0; i<70; i++) {
        assertEq(-0 in a, true);
        assertEq(1.9 in a, false);
        assertEq(NaN in a, false);
    }
}
test2();

::: js/src/ion/CodeGenerator.cpp
@@ +3907,5 @@
> +    Register initLength = ToRegister(lir->initLength());
> +    Register output = ToRegister(lir->output());
> +
> +    // If the index is out of bounds, load |undefined|. Otherwise, load the
> +    // value.

Nit: please update this comment.

::: js/src/ion/MIR.h
@@ +3516,5 @@
>  
> +// Test whether the index is in the array bounds or a hole.
> +class MInArray
> +  : public MTernaryInstruction,
> +    public SingleObjectPolicy

Nit: InArray doesn't need a type policy.

@@ +3535,5 @@
> +  public:
> +    INSTRUCTION_HEADER(InArray);
> +
> +    static MInArray *New(MDefinition *elements, MDefinition *index,
> +                                 MDefinition *initLength, bool needsHoleCheck) {

Nit: fix indentation so that "MDefinition *initLength" lines up with "MDefinition *elements".

Comment 6

[Tom S]

Thank you, this was fast and on a weekend!
https://hg.mozilla.org/integration/mozilla-inbound/rev/ece6848a46a7

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ece6848a46a7