Closed Bug 1187511 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

40 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: sbauman, Assigned: sbauman)

Details

Attachments

(3 files, 4 obsolete files)

Attached file 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.
Attached patch getprop-type-refinement.patch (obsolete) — Splinter Review
Attachment #8638815 - Flags: review?(jdemooij)
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Target Milestone: --- → mozilla42
Version: 38 Branch → 40 Branch
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 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.
Attachment #8638815 - Flags: review?(jdemooij)
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.
Attached patch getprop-type-refinement.patch (obsolete) — Splinter Review
Attachment #8638815 - Attachment is obsolete: true
Attachment #8639620 - Flags: review?(jdemooij)
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...
Attachment #8639620 - Flags: review?(jdemooij)
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);
}
Attachment #8638815 - Flags: review+
Attachment #8638815 - Attachment is obsolete: false
Attachment #8638815 - Attachment is obsolete: true
Attachment #8639620 - Attachment is obsolete: true
Attachment #8639966 - Flags: review+
Keywords: checkin-needed
Assignee: nobody → sabauma
https://hg.mozilla.org/mozilla-central/rev/a27930f420b1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
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?
Flags: needinfo?(sabauma)
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.
Flags: needinfo?(sabauma)
Attached patch getprop-regression.patch (obsolete) — Splinter Review
Inspect the |resultTypeSet| rather than |type| in |improveThisTypeForCall|.
Attachment #8648076 - Flags: feedback?(hv1989)
Comment on attachment 8648076 [details] [diff] [review]
getprop-regression.patch

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

Nice find!
Attachment #8648076 - Flags: feedback?(hv1989) → feedback+
Attached patch filter-types.patch (obsolete) — Splinter Review
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.
Attachment #8648076 - Attachment is obsolete: true
Attachment #8649032 - Flags: review?(jdemooij)
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...
(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.
(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?
(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 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 :)
Attachment #8649032 - Flags: review?(jdemooij) → review+
(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.
Attachment #8649032 - Attachment is obsolete: true
Attachment #8649503 - Flags: review+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.