837312 - IonMonkey: Inline a strict subset of known targets

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

This is a follow-up to Bug 837014 to handle interference between the halves of DeltaBlue.

Both chainTest() and projectionTest() perform their work in plan.execute(), but projectionTest() introduces a fourth constraint type, "ScaleConstraint". The body of its execute function is as follows:

> ScaleConstraint.prototype.execute = function () {
>   if (this.direction == Direction.FORWARD) {
>     this.v2.value = this.v1.value * this.scale.value + this.offset.value;
>   } else {
>     this.v1.value = (this.v2.value - this.offset.value) / this.scale.value;
>   }
> }

The addition of this extra target from plan.execute()'s |c.execute()| causes an invalidation, which is fine -- but ScaleConstraint is relatively rare. After invalidation, we reach a point where we would like to inline that callsite, but useCounts are as follows:

> EditConstraint: 122 // empty evaluate(), always inlined
> EqualityConstraint: 9905
> StayConstraint: 0 // empty evaluate(), always inlined
> ScaleConstraint: 0 // Never run, and the function has no type information.

In this case, we would like to inline the .evaluate() functions for EditConstraint, EqualityConstraint, and StayConstraint, but allow ScaleConstraint's to fall back to the generic MCall path.

That path already exists, but we make inlining decisions on an all-or-nothing-basis: we should instead gain the ability to inline only a strict subset of the known targets.

Based on conservative testing, this is a performance gain of >3%.

Comment 1

[Brian Hackett [Laid off!]]

I think this is what we want to do for the run() call in v8-richards (in TaskControlBlock.prototype.run).  Right now the only thing blocking inlining of this call is that one of the callees (WorkerTask.prototype.run) contains a loop.  If I enable inlining of functions with loops (per bug 768288) then richards' score improves by 50%, but only when not using parallel compilation.  Only 7% of the calls are on WorkerTask.prototype.run, so when compiling off thread it is often the case that its use count is not high enough when compiling the caller.  So it'd be nice if the callees which are hit 93% of the time were inlined, and the one hit 7% of the time was not (though it should still use a direct call rather than an MCallGeneric).

Comment 2

[Sean Stangl [:sstangl]]

The only benchmarks in {SS, v8, octane} that would be affected by this change are in v8's deltablue (3 callsites) and v8's richards (1 callsite).

Comment 3

[Sean Stangl [:sstangl]]

Attachment: WIP patch

Work in progress. Everything is done, but TypeObject-based inlining is incorrect and slows down DeltaBlue and Regexp.

Richards, which uses JSFunction*-based inlining, is sped up 17.6%, ~17000 -> ~20000.

Working on this patch in (scarce) free time.

Comment 4

[Sean Stangl [:sstangl]]

Note that this still uses an MCallGeneric for the fallback case. That could be changed to MCallKnown very easily.

Comment 5

[Hannes Verschore [:h4writer]]

Since I found the free time, I'll finish this. This is already discussed with Sean.

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: inline subset

- Unbitrot
- Fixed all TODO's

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Inline a subset of targets

This changes inlining a lot. Therefore fuzzing this patch would be very welcome. It is build on top of the patch in bug 849781 . Both should apply on a recent revision (I tried 44cf42a8e6e5)

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 729053 [details] [diff] [review]
Inline a subset of targets

Diff from your patch:
- Enable native inlining
- Clean-up graph when native inlining fails
- Preallocating length of phi. Note that I made it possible to overallocate. Therefore the change from initLength to reserveLength.
- Improved spewing of inlining
- Enabled LCallKnown to be used
- Removed the legacy function
- Removed the indicated rooting

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 729053 [details] [diff] [review]
Inline a subset of targets

Since Sean made the patch himself, I would like to have a secondary set of eyes to review this patch. This fully enables inlining a subset of functions.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 729053 [details] [diff] [review]
Inline a subset of targets

Adding myself for fuzzing.

Comment 11

[Christian Holler (:decoder)]

function bind(f) {
  return f.call.apply(f.bind, arguments);
}
function g(a, b) {}
for(var i=0; i<20; ++i) {
  g.call(undefined, {}, bind(function(){}));
}


Assertion failure: target->isInterpreted(), at ion/IonBuilder.cpp:2938

Arch: 32-bit, OS: Linux, Options: --ion-eager

Comment 12

[Hannes Verschore [:h4writer]]

Attachment: Additional patch for bug decoder found

Comment 13

[Hannes Verschore [:h4writer]]

Attachment: Inline a subset of targets

Combined both patches (upon request from Gary). From revision f14e176de069 onwards this should just apply to m-i. (No need for other patches anymore.)

Comment 14

[Kannan Vijayan [:djvj]]

Comment on attachment 729894 [details] [diff] [review]
Inline a subset of targets

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

Nice work by all involved.  This feels like a good clean-up of the hacked up mess that was there previously.

::: js/src/ion/CodeGenerator.cpp
@@ +475,5 @@
> +    // Generate a fallback path if required.
> +    if (mir->hasFallback()) {
> +        LBlock *fallback = mir->getFallback()->lir();
> +        masm.jump(fallback->label());
> +    }

Couple notes here about this method's implementation:

1. It seems like at least one of |hasFallback()| or |mir->numCases() > 0| should be true (otherwise the code emission would be a no-op).  Should ASSERT this.

2. The code seems be shorter and clearer if it switches on the hasFallback() first:

JS_ASSERT(mir->hasFallback() || mir->numCases() > 0);
if (mir->hasFallback() {
    for (int i = 0; i < mir->numCases(); i++) {
        LBlock *target = ...;
        masm.branchPtr(Assembler::Equal, input, ImmGCPtr(mir->getCase(i)), target->label());
    }
    masm.jump(fallback->label());
} else {
    for (int i = 0; i < mir->numCases() - 1; i++) {
        LBlock *target = ...;
        masm.branchPtr(Assembler::Equal, input, ImmGCPtr(mir->getCase(i)), target->label());
    }
    LBlock *lastBlock = ...;
    masm.jump(lastBlock->label());
}
return true;

::: js/src/ion/IonBuilder.cpp
@@ +3262,4 @@
>          }
> +
> +        choiceSet.append(inlineable);
> +        numInlineable += uint32_t(inlineable);

It's clearer to just do:
  if(inlineable)
    numInlineable++;

@@ +3344,1 @@
>  IonBuilder::makePolyInlineDispatch(JSContext *cx, CallInfo &callInfo, 

Nit: pre-existing whitespace at end of line.

::: js/src/ion/MIR.cpp
@@ +607,5 @@
> +{
> +    // This can only been done if the length was reserved through reserveLength,
> +    // else the slower addInputSlow need to get called.
> +#if DEBUG
> +    JS_ASSERT(inputs_.length() < capacity_);

Nit: Does not need to be warpped in #if DEBUG.

::: js/src/ion/MIR.h
@@ +5083,5 @@
> +
> +// Polymorphic dispatch for inlining, keyed off incoming TypeObject.
> +class MTypeObjectDispatch : public MDispatchInstruction
> +{
> +    // Map from JSFunction* -> TypeObject.

The mapping is actually from TypeObject -> JSFunction *
Something like the following might be more appropriate:
  // Map TypeObject of CallProp's Target Object -> JSFunction yielded by the CallProp.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 729894 [details] [diff] [review]
Inline a subset of targets

I had a round of overnight fuzzing on a Linux machine, nothing terribly wrong found. ->feedback+

Comment 16

[Christian Holler (:decoder)]

Comment on attachment 729894 [details] [diff] [review]
Inline a subset of targets

No more patch-specific issues found :)

Comment 17

[Hannes Verschore [:h4writer]]

Attachment: Inline a subset of targets

Updated with the comments. Only need approval of Sean ;)

Comment 18

[Sean Stangl [:sstangl]]

Comment on attachment 730690 [details] [diff] [review]
Inline a subset of targets

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

When you commit the patch, make sure to edit out the 3000 line commit message!

::: js/src/ion/CodeGenerator.cpp
@@ +466,5 @@
> +        lastLabel = mir->getFallback()->lir()->label();
> +    }
> +
> +    // Compare function pointers, except for the last case.
> +    for (size_t i = 0; i < casesWithFallback - 1; i++) {

It may be interesting to order these cases by target useCount.

@@ +499,5 @@
> +        DebugOnly<bool> found = false;
> +        for (size_t j = 0; j < propTable->numEntries(); j++) {
> +            if (propTable->getFunction(j) != func)
> +                continue;
> +            types::TypeObject *typeObj = propTable->getTypeObject(j);

JS_ASSERT(!found) here? Or "break;" after "found = true;".

::: js/src/ion/IonBuilder.cpp
@@ +3732,5 @@
> +        // Note that guarding is on the original function pointer even
> +        // if there is a clone, since cloning occurs at the callsite.
> +        JSFunction *original = originals[i]->toFunction();
> +        MConstant *funcDef = MConstant::New(ObjectValue(*original));
> +        funcDef->setFoldedUnchecked();

We don't set the Folded bit on every constant -- and it looks like the original code occasionally (but not always!) uses that bit on the original function, before replacing it with an MConstant.

It looks like the Folded bit is only used to skip MInstructions from elimination phases in IonAnalysis.cpp, which seems like fairly scary behavior -- it may be worthwhile to track down the person who added it to find out whether we're using the bit in the intended way.

@@ +3823,5 @@
> +                    break;
> +                }
> +            }
> +
> +            if (!inlineGenericFallback(remaining, callInfo, dispatchBlock, clonedAtCallsite))

nice

Comment 19

[Hannes Verschore [:h4writer]]

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

Comment 20

[Hannes Verschore [:h4writer]]

For the record:

(In reply to Sean Stangl [:sstangl] from comment #18)
> When you commit the patch, make sure to edit out the 3000 line commit
> message!
booo'h

> It may be interesting to order these cases by target useCount.
Added follow-up bug

> @@ +499,5 @@
> > +        DebugOnly<bool> found = false;
> > +        for (size_t j = 0; j < propTable->numEntries(); j++) {
> > +            if (propTable->getFunction(j) != func)
> > +                continue;
> > +            types::TypeObject *typeObj = propTable->getTypeObject(j);
> 
> JS_ASSERT(!found) here? Or "break;" after "found = true;".
No, that would be wrong. Different TypeObjects can have the same target function.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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