Closed Bug 1128535 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Keywords: perf, regression, Whiteboard: [talos_regression])

Attachments

(2 files)

Attached patch PatchSplinter Review
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.)
Attachment #8557932 - Flags: review?(efaustbmo)
(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.
(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 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.
Attachment #8557932 - Flags: review?(efaustbmo) → review+
Looks like this regressed Droameo dom-query, will investigate this today or tomorrow.
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/0d6419079ae1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(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
This patch fixed a minor problem but it exposed another (pre-existing) issue. Patch for that coming in a bit.
Attached patch Follow-up fixSplinter Review
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.
Flags: needinfo?(jdemooij)
Attachment #8559370 - Flags: review?(efaustbmo)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 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.
Attachment #8559370 - Flags: review?(efaustbmo) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/b2e330b524b9
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Keywords: perf, regression
Whiteboard: [talos_regression]
Depends on: 1155788
You need to log in before you can comment on or make changes to this bug.