1187511 - IonMonkey: Improve refinement of |this| types on GET_PROP operations

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Spenser Andrew Bauman [:sbauman]]

Attachment: example.js

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150721080904

Steps to reproduce:

The benchmark attached exhibits a serious performance hit due to this issue.

Consider the bytecode sequence generated by SpiderMonkey for `a.foo()`

```
// load |a| onto the stack
DUP
CALL_PROP "foo"
SWAP
CALL
```

IonMonkey attempts to unbox its copy of |a| produced by the DUP instruction in
that a get property instruction is emitted, but this information is not
reflected in the second copy on the stack which flows into the |this| slot of the
CALL operation. This leads to additional COMPUTE_THIS operations when the call
is inlined.

Comment 1

[Spenser Andrew Bauman [:sbauman]]

Attachment: getprop-type-refinement.patch

Comment 2

[Nicolas B. Pierron [:nbp]]

This sounds to me to be more general than the |thisSlot| or any getprop.  I guess every time we refine the type of an input for the input of an operation, we could refine it as-well for all the following operations.

This makes me think that what we are really looking for is to exhibit a property on the input, which holds to all later point in the workflow, and not insert an unboxed statement.  Much more like what a guard should really be.

Also, couldn't this problem be solved by GVN?

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8638815 [details] [diff] [review]
getprop-type-refinement.patch

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

Good find.

A while ago I added IonBuilder::improveThisTypesForCall, it's supposed to solve the same problem. Can you find out why that code doesn't work here? Maybe because the this-typeset is not object/null/undefined but includes other types?

If there's a good reason we can't fix this issue in improveThisTypesForCall, we can probably take this patch though.

Comment 4

[Spenser Andrew Bauman [:sbauman]]

Upon reflection, it seems we can do this in |improveThisTypesForCall| and with even less effort, so I've updated the patch.

As to the use of GVN, it may be possible to get the GVN pass to handle some of this, but it would then have to contend with the bad code generated by previous passes due to less precise type information.
For the attached example, the apply types pass ends up inserting box operations on each iteration to ensure a phi node input is of type Value, to unbox it again on the next iteration.

Comment 5

[Spenser Andrew Bauman [:sbauman]]

Attachment: getprop-type-refinement.patch

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8639620 [details] [diff] [review]
getprop-type-refinement.patch

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

::: js/src/jit/IonBuilder.cpp
@@ -10253,5 @@
>      MDefinition* thisDef = current->peek(-2);
>      if (thisDef->type() != MIRType_Value ||
>          !thisDef->mightBeType(MIRType_Object) ||
> -        !thisDef->resultTypeSet() ||
> -        !thisDef->resultTypeSet()->objectOrSentinel())

Hm it's not valid to remove the objectOrSentinel check. The idea is that null.foo() or undefined.foo() is guaranteed to throw, so once we get past the CALLPROP we *know* `this` must be non-null/undefined.

For other primitives, that's not true though:

  "use strict";
  String.prototype.foo = function() { assertEq(typeof this, "string"); };
  "bla".foo();   // foo is called with `this` not an object

I looked at your testcase and I see the problem now; it's a numer-or-object. Maybe your original patch was the best way to fix this, sorry for the bogus suggestion...

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8638815 [details] [diff] [review]
getprop-type-refinement.patch

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

I'll r+ this patch to save us another round trip :) r=me with comments addressed.

::: js/src/jit/IonBuilder.cpp
@@ +10101,5 @@
> +    //      a.foo()
> +    // ================= Compiles to ================
> +    //      LOAD "a"
> +    //      DUP
> +    //      CALL_PROP "foo"

Nit: `CALLPROP` here and below.

@@ +10111,5 @@
> +    // stack as well.
> +    if (current->stackDepth() > 0) {
> +        uint32_t idx = current->stackDepth() - 1;
> +        if (current->getSlot(idx) == def)
> +            current->setSlot(idx, unbox);

I'd change this to

if (*pc == JSOP_CALLPROP || *pc == JSOP_CALLELEM) {
    MOZ_ASSERT(current->getSlot(idx) == def);
    current->setSlot(idx, unbox);
}

Comment 8

[Spenser Andrew Bauman [:sbauman]]

Attachment: getprop-type-refinement.patch

Comment 9

[Spenser Andrew Bauman [:sbauman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e5b639e454f

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a27930f420b1

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Hannes Verschore [:h4writer]]

A regression/improvement was reported on AWFY:
- slave: Mac OS X 10.10 32-bit (Mac Pro, shell)
- mode: Ion

Regression(s)/Improvement(s):
- dart: DeltaBlue: 2.83% (regression)

Recorded range:
- http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=677efdac9819&tochange=a27930f420b1

More details: http://arewefastyet.com/regressions/#/regression/1793434

Comment 13

[Hannes Verschore [:h4writer]]

Would it be possible to have a look at the dart2js deltablue benchmark (https://github.com/h4writer/arewefastyet/tree/master/benchmarks/dart) and look if the changes in "maybeUnboxForPropertyAccess" do something bad for this benchmark?

Comment 14

[Spenser Andrew Bauman [:sbauman]]

It looks like the patch for |maybeUnboxForPropertyAccess| is preventing |improveThisTypeForCall| from firing, which results in sloppier code generation down the line. The unbox operation introduced by |maybeUnboxForPropertyAccess| changed the result type of the operation but not the result type set.

Comment 15

[Spenser Andrew Bauman [:sbauman]]

Attachment: getprop-regression.patch

Inspect the |resultTypeSet| rather than |type| in |improveThisTypeForCall|.

Comment 16

[Hannes Verschore [:h4writer]]

Comment on attachment 8648076 [details] [diff] [review]
getprop-regression.patch

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

Nice find!

Comment 17

[Spenser Andrew Bauman [:sbauman]]

Attachment: filter-types.patch

Make use of the unbox type to refine the result type set for unbox operations.
This fixes the performance regression on the Dart DeltaBlue benchmark in a more general way than the previous patch.

Comment 18

[Jan de Mooij [:jandem]]

Where is this MUnbox causing trouble?

Most MIR instructions with a primitive MIRType don't have a resultTypeSet (because the resultTypeSet doesn't give us more information in that case and does require a memory allocation), so it seems a MIRType_Boolean Unbox for instance could/should just have a nullptr resultTypeSet...

Comment 19

[Spenser Andrew Bauman [:sbauman]]

(In reply to Jan de Mooij [:jandem] from comment #18)
> Where is this MUnbox causing trouble?
> 
> Most MIR instructions with a primitive MIRType don't have a resultTypeSet
> (because the resultTypeSet doesn't give us more information in that case and
> does require a memory allocation), so it seems a MIRType_Boolean Unbox for
> instance could/should just have a nullptr resultTypeSet...

The MUnbox operation is causing problems for code in improveThisTypeForCall
which inspects the MIRType of the instruction to decide whether to refine the
resultTypeSet of the |this| parameter. The MUnbox operation uses the supplied
MIRType for its MIRType but copies the resultTypeSet of its input. This results
in improveThisTypeForCall not firing due to the refined MIRType.

I was thinking this would be a more robust fix than tweaking improveThisTypeForCall
to look at the resultTypeSet since some instructions don't have a resultTypeSet.

I see that this patch is a little overzealous, and we probably only really need the
case for MIRType_Object, which is sufficient to fix this regression.

Comment 20

[Jan de Mooij [:jandem]]

(In reply to Spenser Andrew Bauman [:sbauman] from comment #19)
> The MUnbox operation is causing problems for code in improveThisTypeForCall
> which inspects the MIRType of the instruction to decide whether to refine the
> resultTypeSet of the |this| parameter.

Hm I see, but improveThisTypeForCall doesn't need to do anything here because MIRType_Object doesn't include null/undefined anyway right? Where down the line do we rely on the (MUnbox) resultTypeSet without checking its MIRType?

Comment 21

[Spenser Andrew Bauman [:sbauman]]

(In reply to Jan de Mooij [:jandem] from comment #20)
> Hm I see, but improveThisTypeForCall doesn't need to do anything here
> because MIRType_Object doesn't include null/undefined anyway right? Where
> down the line do we rely on the (MUnbox) resultTypeSet without checking its
> MIRType?

The problem manifests in code for getting/setting unboxed properties, which
computes the offset using the resultTypeSet. This procedure also checks the
MIRType to determine that the input definition is an object, but with
MUnbox<MIRType_Object> the MIRType and resultTypeSet are slightly out of sync.

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 8649032 [details] [diff] [review]
filter-types.patch

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

Makes sense, thanks for the explanation! r=me with comment below addressed.

::: js/src/jit/MIR.h
@@ +4395,5 @@
>                     type == MIRType_Object);
>  
> +        TemporaryTypeSet* resultSet = ins->resultTypeSet();
> +        if (resultSet)
> +            resultSet = TypeSet::filterByMIRType(resultSet, type, alloc.lifoAlloc());

Please change this to:

if (resultSet && type == MIRType_Object)
    resultSet = resultSet->cloneObjectsOnly(alloc.lifoAlloc());

And then we don't need the vm/TypeInference.{h,cpp} changes :)

Comment 23

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #22)
> if (resultSet && type == MIRType_Object)
>     resultSet = resultSet->cloneObjectsOnly(alloc.lifoAlloc());

You could also move the setResultTypeSet call into the `if` so we only set the resultTypeSet if we have an object. Either way is fine with me.

Comment 24

[Spenser Andrew Bauman [:sbauman]]

Attachment: filter-types.patch

Comment 25

[Spenser Andrew Bauman [:sbauman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f02fa8f98428

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7e30cbf82c1

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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