Don't emit ObjectGroupDispatch fallback path if possible

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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

3 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

3 years ago
Created attachment 8480450 [details] [diff] [review]
WIP

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

3 years ago
Created attachment 8571519 [details] [diff] [review]
Patch

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

3 years ago
Summary: Don't emit TypeObjectDispatch fallback path if possible → Don't emit ObjectGroupDispatch fallback path if possible
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

3 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

3 years ago
Comment on attachment 8571519 [details] [diff] [review]
Patch

See comment 5.
Attachment #8571519 - Flags: review?(bhackett1024)
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

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/57ddae8223f9

Added the comment.

Comment 9

3 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
https://hg.mozilla.org/mozilla-central/rev/57ddae8223f9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 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.