Closed
Bug 1059364
Opened 10 years ago
Closed 10 years ago
Don't emit ObjectGroupDispatch fallback path if possible
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
Probably the hottest function in Octane-deltablue is Plan.prototype.execute. We use polymorphic inlining with MTypeObjectDispatch for the c.execute() call, but unfortunately IonBuilder always adds a fallback path that has a GetPropertyCache and CallGeneric and this blocks LICM and other optimizations from optimizing the rest of the code.
It's tricky to fix because for the c.execute() call, c's TypeSet contains 5 TypeObjects but in practice we only see 3, so somehow we need to narrow it down to those 3 and then emit MTypeObjectDispatch without the fallback path.
Based on some local hacks this could win at least a few thousand points. Unfortunately the polymorphic inlining code is really complicated, so I've no idea if this is correct.
Assignee | ||
Comment 1•10 years ago
|
||
Taking this for now as there are a few things I'd like to try.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
A hackish WIP patch that wins about 10% on deltablue on 32-bit. I don't see any difference with a 64-bit build though because type information is different somehow.
Assignee | ||
Comment 3•10 years ago
|
||
With this patch we don't emit a fallback path if the incoming object types are all handled by the ObjectGroupDispatch instruction. This improves alias analysis: the fallback path if present blocks LICM/GVN. It should also help regalloc.
This wins at least 1000 points or so when I run deltablue locally, both with and without --unboxed-objects.
Attachment #8480450 -
Attachment is obsolete: true
Attachment #8571519 -
Flags: review?(bhackett1024)
Assignee | ||
Updated•10 years ago
|
Summary: Don't emit TypeObjectDispatch fallback path if possible → Don't emit ObjectGroupDispatch fallback path if possible
Comment 4•10 years ago
|
||
Comment on attachment 8571519 [details] [diff] [review]
Patch
Review of attachment 8571519 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/IonBuilder.cpp
@@ +5477,5 @@
> + MOZ_ASSERT(barrier->type() == MIRType_Object);
> + MOZ_ASSERT(barrier->input()->isGetPropertyCache());
> + MOZ_ASSERT(barrier->input()->toGetPropertyCache() == maybeCache);
> +
> + barrier->block()->discard(barrier);
I don't understand why it's ok to remove this barrier. We are removing the fallback path if the dispatch handles all incoming objects, but those objects are produced by the barrier instruction and the barrier is in place because it is possible to read other objects. Is there another place where we'll ensure that other incoming objects are caught and trigger a bailout?
@@ +5479,5 @@
> + MOZ_ASSERT(barrier->input()->toGetPropertyCache() == maybeCache);
> +
> + barrier->block()->discard(barrier);
> + MOZ_ASSERT(!maybeCache->hasUses());
> + maybeCache->block()->discard(maybeCache);
The last two lines here can be commoned with the end of the |if| block and moved after the |else| block.
Attachment #8571519 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Brian Hackett (:bhackett) from comment #4)
> I don't understand why it's ok to remove this barrier. We are removing the
> fallback path if the dispatch handles all incoming objects, but those
> objects are produced by the barrier instruction and the barrier is in place
> because it is possible to read other objects.
We have:
objectDef -> MGetPropertyCache -> MTypeBarrier
objectDef -> MObjectGroupDispatch
The patch is checking whether objectDef's types are all handled by the ObjectGroupDispatch. The GetPropertyCache and TypeBarrier are unrelated to this; right now we move them to the fallback path but if we don't have a fallback path we can eliminate them.
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8571519 -
Flags: review?(bhackett1024)
Comment 7•10 years ago
|
||
Comment on attachment 8571519 [details] [diff] [review]
Patch
Review of attachment 8571519 [details] [diff] [review]:
-----------------------------------------------------------------
OK, but can you add a comment describing why it's OK to remove the type barrier? Just because the type barrier doesn't feed into the dispatch doesn't mean it's required for correctness (i.e. it's a guard). But something like:
// The object group dispatch handles all possible incoming objects, so the cache and barrier will not be reached and can be eliminated.
Attachment #8571519 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/57ddae8223f9
Added the comment.
Comment 9•10 years ago
|
||
Deltablue on AWFY:
Mac 32-bit machine:
without unboxed-objects - 8% regression
with unboxed objects - 8% improvement
Mac 64-bit machine:
on both cases - 3% improvement
Windows 32-bit (browser) - no difference
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•