Closed
Bug 837312
Opened 12 years ago
Closed 12 years ago
IonMonkey: Inline a strict subset of known targets
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: sstangl, Assigned: h4writer)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
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%.
Reporter | ||
Updated•12 years ago
|
Summary: IonMonkey: Permit fallback path for polymorphic inlining → IonMonkey: Inline a strict subset of known targets
Reporter | ||
Updated•12 years ago
|
Assignee: general → sstangl
Comment 1•12 years ago
|
||
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).
Reporter | ||
Comment 2•12 years ago
|
||
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).
Reporter | ||
Comment 3•12 years ago
|
||
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.
Reporter | ||
Comment 4•12 years ago
|
||
Note that this still uses an MCallGeneric for the fallback case. That could be changed to MCallKnown very easily.
Assignee | ||
Comment 5•12 years ago
|
||
Since I found the free time, I'll finish this. This is already discussed with Sean.
Assignee: sstangl → hv1989
Assignee | ||
Comment 6•12 years ago
|
||
- Unbitrot
- Fixed all TODO's
Attachment #722013 -
Attachment is obsolete: true
Assignee | ||
Comment 7•12 years ago
|
||
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)
Assignee | ||
Comment 8•12 years ago
|
||
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)
Assignee | ||
Comment 9•12 years ago
|
||
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 10•12 years ago
|
||
Comment on attachment 729053 [details] [diff] [review]
Inline a subset of targets
Adding myself for fuzzing.
Attachment #729053 -
Flags: feedback?(gary)
Comment 11•12 years ago
|
||
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
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #729821 -
Flags: review?(sstangl)
Assignee | ||
Comment 13•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #729894 -
Flags: review?(kvijayan)
Attachment #729894 -
Flags: feedback?(gary)
Comment 14•12 years ago
|
||
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 15•12 years ago
|
||
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 16•12 years ago
|
||
Comment on attachment 729894 [details] [diff] [review]
Inline a subset of targets
No more patch-specific issues found :)
Attachment #729894 -
Flags: feedback?(choller) → feedback+
Assignee | ||
Comment 17•12 years ago
|
||
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)
Reporter | ||
Comment 18•12 years ago
|
||
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+
Assignee | ||
Comment 19•12 years ago
|
||
Blocks: 855897
Assignee | ||
Comment 20•12 years ago
|
||
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•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Updated•11 years ago
|
Depends on: CVE-2013-1728
You need to log in
before you can comment on or make changes to this bug.
Description
•