Closed Bug 1059364 Opened 7 years ago Closed 7 years ago

Don't emit ObjectGroupDispatch fallback path if possible

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Taking this for now as there are a few things I'd like to try.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attached patch WIP (obsolete) — Splinter Review
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.
Attached patch PatchSplinter Review
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)
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)
(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.
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+
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.