Closed Bug 802869 Opened 7 years ago Closed 7 years ago

IonMonkey: Fast path for LIn for Int32 in dense array

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: sstangl, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ion:t])

Attachments

(2 files)

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.
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.
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
Hey, I have some free time again. I am going to do this!
Assignee: general → evilpies
Status: NEW → ASSIGNED
Attached patch v1Splinter Review
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.
Attachment #682781 - Flags: review?(jdemooij)
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".
Attachment #682781 - Flags: review?(jdemooij) → review+
Thank you, this was fast and on a weekend!
https://hg.mozilla.org/integration/mozilla-inbound/rev/ece6848a46a7
https://hg.mozilla.org/mozilla-central/rev/ece6848a46a7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.