Closed Bug 837312 Opened 10 years ago Closed 10 years ago

IonMonkey: Inline a strict subset of known targets

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: sstangl, Assigned: h4writer)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 5 obsolete files)

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%.
Summary: IonMonkey: Permit fallback path for polymorphic inlining → IonMonkey: Inline a strict subset of known targets
Assignee: general → sstangl
Depends on: 838469
Depends on: 839727
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).
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).
Depends on: 846539
Attached patch WIP patch (obsolete) — Splinter Review
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.
Note that this still uses an MCallGeneric for the fallback case. That could be changed to MCallKnown very easily.
Since I found the free time, I'll finish this. This is already discussed with Sean.
Assignee: sstangl → hv1989
Attached patch inline subset (obsolete) — Splinter Review
- Unbitrot
- Fixed all TODO's
Attachment #722013 - Attachment is obsolete: true
Attached patch Inline a subset of targets (obsolete) — Splinter Review
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)
Attachment #729032 - Attachment is obsolete: true
Attachment #729053 - Flags: feedback?(choller)
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
Attachment #729053 - Flags: review?(sstangl)
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.
Attachment #729053 - Flags: review?(kvijayan)
Comment on attachment 729053 [details] [diff] [review]
Inline a subset of targets

Adding myself for fuzzing.
Attachment #729053 - Flags: feedback?(gary)
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
Attachment #729821 - Flags: review?(sstangl)
Attached patch Inline a subset of targets (obsolete) — Splinter Review
Combined both patches (upon request from Gary). From revision f14e176de069 onwards this should just apply to m-i. (No need for other patches anymore.)
Attachment #729053 - Attachment is obsolete: true
Attachment #729821 - Attachment is obsolete: true
Attachment #729053 - Flags: review?(sstangl)
Attachment #729053 - Flags: review?(kvijayan)
Attachment #729053 - Flags: feedback?(gary)
Attachment #729053 - Flags: feedback?(choller)
Attachment #729821 - Flags: review?(sstangl)
Attachment #729894 - Flags: review?(sstangl)
Attachment #729894 - Flags: feedback?(choller)
Attachment #729894 - Flags: review?(kvijayan)
Attachment #729894 - Flags: feedback?(gary)
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.
Attachment #729894 - Flags: review?(kvijayan) → review+
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+
Attachment #729894 - Flags: feedback?(gary) → feedback+
Comment on attachment 729894 [details] [diff] [review]
Inline a subset of targets

No more patch-specific issues found :)
Attachment #729894 - Flags: feedback?(choller) → feedback+
Updated with the comments. Only need approval of Sean ;)
Attachment #729894 - Attachment is obsolete: true
Attachment #729894 - Flags: review?(sstangl)
Attachment #730690 - Flags: review?(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
Attachment #730690 - Flags: review?(sstangl) → review+
Blocks: 855852
Blocks: 855854
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.
https://hg.mozilla.org/mozilla-central/rev/a92c968b29ae
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 861904
Depends on: 904759
Depends on: CVE-2013-1728
You need to log in before you can comment on or make changes to this bug.