1128535 - Inline getters/setters in Ion even if type information is bad

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Our getter/setter inlining in IonBuilder relies on TI and works great if type information is good, but if type information is bad for some reason we give up. This happens a lot when running deltablue.swf (bug 1103441).

With this patch we will fall back to shape guards in this case (based on the Baseline caches), so that we can still optimize the getter/setter. This improves deltablue.swf from ~600 points to ~2200 points.

(I was wondering if we have any jit-tests hitting this and we do: setting __proto__ marks the ObjectGroup as having unknown properties but with this patch we can still inline __proto__ gets/sets on those objects.)

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> This improves deltablue.swf from ~600 points to ~2200 points.

FWIW, we still spend more than 50% under GetPropertyIC::update, so there's still a lot to improve.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> FWIW, we still spend more than 50% under GetPropertyIC::update, so there's
> still a lot to improve.

I looked into this and I think we need Ion ICs for scripted getters. There are different holder objects with different shapes and different getters, so we can't optimize this in IonBuilder (without adding some complicated polymorphic inlining mechanism).

Comment 3

[Eric Faust [:efaust]]

Comment on attachment 8557932 [details] [diff] [review]
Patch

Review of attachment 8557932 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

::: js/src/jit/BaselineInspector.cpp
@@ +548,5 @@
> +
> +static bool
> +AddReceiverShapeForGetPropFunction(BaselineInspector::ShapeVector &shapes, ICStub *stub)
> +{
> +    if (stub->isGetProp_CallNative())

Why don't we need a guard for own property call natives? Because the holder shape guard is sufficient, right?

@@ +600,2 @@
>              }
> +        } else if (!stub->isGetProp_Fallback() ||

Whoops! Nice catch.

::: js/src/jit/IonBuilder.cpp
@@ +10209,5 @@
>      bool canUseCommonGetter =
>          testCommonGetterSetter(objTypes, name, /* isGetter = */ true,
>                                 foundProto, lastProperty, &guard, globalShape,
>                                 &globalGuard);
> +    if (!canUseCommonGetter) {

This is no longer a great name. canUseTIForGetter, or something?

@@ +10718,5 @@
>      MDefinition *guard = nullptr;
>      bool canUseCommonSetter =
>          testCommonGetterSetter(objTypes, name, /* isGetter = */ false,
>                                 foundProto, lastProperty, &guard);
> +    if (!canUseCommonSetter) {

Same as above.

Comment 4

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d6419079ae1

Thanks for the quick review.

Comment 5

[Jan de Mooij [:jandem]]

Looks like this regressed Droameo dom-query, will investigate this today or tomorrow.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d6419079ae1

Comment 7

[William Lachance (:wlach)]

(In reply to Jan de Mooij [:jandem] from comment #5)
> Looks like this regressed Droameo dom-query, will investigate this today or
> tomorrow.

Yes, this regression is highly reproducible across MacOS X, Windows, and Linux:

http://alertmanager.allizom.org:8080/alerts.html?rev=0d6419079ae1&showAll=1

Comment 8

[Jan de Mooij [:jandem]]

This patch fixed a minor problem but it exposed another (pre-existing) issue. Patch for that coming in a bit.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Follow-up fix

I changed:

if (stub->isGetProp_Fallback() && stub->toGetProp_Fallback()->hadUnoptimizableAccess())

to:

if (!stub->isGetProp_Fallback() || stub->toGetProp_Fallback()->hadUnoptimizableAccess())

But this regressed dom-query because we have a GetName_Fallback stub there and we now |return false| for that. So this patch also adds an unoptimizable-access bit to GetName_Fallback for consistency with GetProp and we check for that case, too.

This fixes the dom-query regression locally.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

if you want to push to try you can do:
try: -b o -p linux,linux64,macosx64,win32,win64 -u none -t dromaeojs

we could retrigger and determine if the results are better.

Comment 11

[Eric Faust [:efaust]]

Comment on attachment 8559370 [details] [diff] [review]
Follow-up fix

Review of attachment 8559370 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry about that. The change looked good to me the first time, as well.

::: js/src/jit/BaselineInspector.cpp
@@ +602,5 @@
> +            // If we have an unoptimizable access, don't try to optimize.
> +            if (stub->toGetProp_Fallback()->hadUnoptimizableAccess())
> +                return false;
> +        } else if (stub->isGetName_Fallback()) {
> +            if (stub->toGetName_Fallback()->hadUnoptimizableAccess())

If this gets more and more prevailing, we should try to common this up in a baseclass for the fallback stubs, somehow. This is fine.

Comment 12

[Jan de Mooij [:jandem]]

Follow-up patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e330b524b9

(In reply to Eric Faust [:efaust] from comment #11)
> Sorry about that. The change looked good to me the first time, as well.

Not your fault; I think everybody would have missed this one.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b2e330b524b9