664824 - TI+JM: make JSOP_IN fast

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

JSOP_IN currently always takes a stub call in JM. This is causing performance problems in bug 664769. Small testcase:
--
function f(a) {
    var len = a.length;
    var t0 = new Date;
    for (var i=0; i<len; i++) {
        i in a;
    }
    print(new Date - t0);
}
f(new Array(10000000));
--
js -j   :   47 ms
d8      :  798 ms
js -m -n:  836 ms
js -m   :  877 ms
js      : 1210 ms

Looking at record_JSOP_IN, the tracer optimizes at least "int32 in dense_array". For this testcase TM guards that Array.prototype has no indexed properties and index < initializedLength. Especially with TI we should be able to do something similar in JM.

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 591065 [details] [diff] [review]
Patch

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

Marco, this is a great patch, thanks! If you can post another patch to address the comments below, this should be good to go. Just in case you don't know, IonMonkey is eventually going to replace JM. We aren't there yet though, so this patch will still benefit a number of Firefox releases, and we can use the same optimization in IonMonkey, of course.

PS: Would you mind posting results for the micro-benchmark in comment 0 with/without the patch? Should be a nice improvement :)

::: js/src/methodjit/Compiler.cpp
@@ +7441,5 @@
> +{
> +  FrameEntry *obj = frame.peek(-1);
> +  FrameEntry *id = frame.peek(-2);
> +
> +  if (cx->typeInferenceEnabled() && id->isType(JSVAL_TYPE_INT32)) {

Nit: indent this function with 4 spaces instead of 2.

@@ +7452,5 @@
> +        Jump guard = frame.testObject(Assembler::NotEqual, obj);
> +        stubcc.linkExit(guard, Uses(2));
> +      }
> +
> +      RegisterID baseReg = frame.tempRegForData(obj);

The frame.allocReg() call below may get assigned the same register. In this case it's okay (since we don't use baseReg after loading dataReg), but to avoid confusion I'd change this line to:

RegisterID dataReg = frame.copyDataIntoReg(obj);

Then you can also remove the "RegisterID dataReg = frame.allocReg()" line below (it's okay for the loadPtr to use dataReg twice)

@@ +7467,5 @@
> +      Jump initlenGuard = masm.guardArrayExtent(ObjectElements::offsetOfInitializedLength(),
> +                                        dataReg, key, Assembler::BelowOrEqual);
> +
> +      // Guard to make sure we don't have a hole. Skip it if the array is packed.
> +      Jump holeCheck;

Nit: use MaybeJump instead of Jump, and change holeCheck.linkTo(...) to holeCheck.getJump().linkTo(...)

@@ +7474,5 @@
> +          Address slot(dataReg, key.index() * sizeof(Value));
> +          holeCheck = masm.guardNotHole(slot);
> +        } else {
> +          BaseIndex slot(dataReg, key.reg(), masm.JSVAL_SCALE);
> +          holeCheck = masm.guardNotHole(slot);

nit: can you move this if-else to BaseAssembler.h? Something like this "Jump guardElementNotHole(RegisterID elements, const Int32Key &key) { ... }"

@@ +7478,5 @@
> +          holeCheck = masm.guardNotHole(slot);
> +        }
> +      }
> +
> +      masm.move(Imm32(1), Registers::ReturnReg);

We can't overwrite ReturnReg since the register allocator may have assigned it to something else. Since we no longer need dataReg, and it's writable, you can just use that register here (and below) instead of ReturnReg.

@@ +7489,5 @@
> +      masm.move(Imm32(0), Registers::ReturnReg);
> +
> +      done.linkTo(masm.label(), &masm);
> +
> +      frame.freeReg(dataReg);

You can remove this line if you pass dataReg to pushTypedPayload below.

@@ +7495,5 @@
> +      stubcc.leave();
> +      OOL_STUBCALL_USES(stubs::In, REJOIN_FALLTHROUGH, Uses(2));
> +
> +      frame.popn(2);
> +      frame.takeReg(Registers::ReturnReg);

ReturnReg holds the result of the stub call. If we use dataReg for the result of JSOP_IN, you'll have to do something like this instead:

frame.popn(2);
if (dataReg != Registers::ReturnReg)
    stubcc.masm.move(Registers::ReturnReg, dataReg);

frame.pushTypedPayload(JSVAL_TYPE_BOOLEAN, dataReg);

Comment 3

[Marco Castelluccio [:marco]]

Attachment: Patch v2

With the patch ~26 ms, without the patch ~730 ms :D
I've also successfully run the tests under tests/ and jit-test/.

I'd like to help with IonMonkey, also if I haven't so much experience with SpiderMonkey or other compilers.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 591274 [details] [diff] [review]
Patch v2

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

Looks good, r=me with the comments below addressed. I see that you have Try server access so if you can make sure this is green I can land it.

::: js/src/methodjit/BaseAssembler.h
@@ +795,5 @@
> +        Jump jmp;
> +
> +        if (key.isConstant()) {
> +          Address slot(elements, key.index() * sizeof(Value));
> +          jmp = guardNotHole(slot);

Nit: indent with 4 spaces

::: js/src/methodjit/Compiler.cpp
@@ +7469,5 @@
> +
> +    if (cx->typeInferenceEnabled() && id->isType(JSVAL_TYPE_INT32)) {
> +        types::TypeSet *types = analysis->poppedTypes(PC, 0);
> +
> +        if (obj->mightBeType(JSVAL_TYPE_OBJECT) && !types->hasObjectFlags(cx, types::OBJECT_FLAG_NON_DENSE_ARRAY) && !types::ArrayPrototypeHasIndexedProperty(cx, outerScript)) {

Nit: SM style is <= 99 characters per line (80 for comments), so we should break this line in 2 or 3 lines, like this:

if (a &&
    b &&
    c)
{
    ...
}

(If the condition spans multiple lines, we put the brace on a new line to make it clear which lines are part of the condition)

@@ +7481,5 @@
> +            Int32Key key = id->isConstant()
> +                         ? Int32Key::FromConstant(id->getValue().toInt32())
> +                         : Int32Key::FromRegister(frame.tempRegForData(id));
> +
> +            RegisterID dataReg = frame.copyDataIntoReg(obj);

I missed it the first time, but this line should be moved before the previous line. The register assigned by tempRegForData is tracked by the FrameState, and the copyDataIntoReg may clobber it. If we do the copyDataIntoReg first, the register allocator is forced to choose a different register for the tempRegForData since dataReg is "owned" by the compiler and won't be selected.

(Register allocation is too complicated in JM and has caused a lot of problems. Fortunately, regalloc is cleaner in IonMonkey.)

@@ +7506,5 @@
> +
> +            done.linkTo(masm.label(), &masm);
> +
> +            stubcc.leave();
> +            OOL_STUBCALL_USES(stubs::In, REJOIN_FALLTHROUGH, Uses(2));

This should be REJOIN_PUSH_BOOLEAN like below.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Marco Castelluccio from comment #3)
> 
> I'd like to help with IonMonkey, also if I haven't so much experience with
> SpiderMonkey or other compilers.

Cool, feel free to ping one of us on IRC if you have any questions.

Comment 6

[Jan de Mooij [:jandem]]

Marco's latest patch (which addressed the nits in comment 4) was green on try, so I pushed this to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1eee0e95256e

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1eee0e95256e