Bailouts when calling performance.now() if the window's shape has changed since we compiled the ion code

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: marco, Assigned: bzbarsky)

Tracking

unspecified
mozilla36
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8503357 [details]
RmLxa9ir.dms

To reproduce, clone https://github.com/andreasgal/j2me.js and open index.html?profile=1

There are bailouts on each line containing a call to performance.now
(Assignee)

Updated

4 years ago
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 1

4 years ago
> To reproduce, clone https://github.com/andreasgal/j2me.js and open index.html?profile=1

I just get am exception:

  TypeError: MIDP.nativeEventQueues[isolateId] is undefined
Flags: needinfo?(mar.castelluccio)
(Assignee)

Comment 2

4 years ago
And in particular, I'm not seeing any bailouts, when running a tip Firefox on that page with IONFLAGS=bailouts.

I'd really appreciate some non-cryptic steps to reproduce.
(Reporter)

Comment 3

4 years ago
To reproduce:
> git clone https://github.com/andreasgal/j2me.js.git
> cd j2me.js
> make
> Open index.html?profile=1 in Firefox

I'm using the Gecko Profiler add-on to get a profile and I'm searching for the word 'Bailout' and 'instrument' (like this: "tr ',' '\n' < aprofile.dms | grep 'Bailout' | grep 'instrument'").
Flags: needinfo?(mar.castelluccio)
(In reply to Boris Zbarsky [:bz] from comment #1)
> I just get am exception:
> 
>   TypeError: MIDP.nativeEventQueues[isolateId] is undefined

This is an unrelated error that shouldn't affect the index.html page load and the unit tests it runs.
(Assignee)

Comment 5

4 years ago
Created attachment 8505935 [details]
Testcase that shows the problem pretty clearly

Marco, thanks.

Here's a minimal testcase that reproduces the issue.

What happens is that when we first compile f we call IonBuilder::getPropTryCommonGetter which calls testCommonGetterSetter.  This ends up adding a shape guard for "foundProto" using the passed-in lastProperty (which came from the baseline IC).

In our case, foundProto is the Window object.

Then we resolve "Date" on the window, which changes its shape.

The next time we call f, the shape guard fails, since the shape of the window has in fact changed.  So we take a bailout.  So far so good.

But now we try to recompile the ioncode.  We end up in testCommonGetterSetter again and lastProperty has the same value as the previous time.  That is, lastProperty no longer matches foundProto->lastProperty().  So we end up compiling exactly the same code as last time, which immediately bails out when run.  That's not terribly useful.

If I just change testCommonGetterSetter to use foundProto->lastProperty() in the guard, the problem goes away: I get one bailout, and then life is good.

So how come we end up with the old "lastProperty" value the second time around?  This comes from BaselineInspector::commonGetPropFunction which sniffs the baseline ICs.  The second time around, we have _two_ ICStub::GetProp_CallNative stubs at this pc.  The first one is the old one: it has the old holderShape() value and in particular its holderShape() doesn't match its holder()->lastProperty() anymore.  The second one is the stub baseline added when we ran it after the bailout: it has the same holder() but the new holderShape().

It seems to me like testCommonGetterSetter should simply ignore stubs for which the holder()->lastProperty() doesn't match holderShape(), since compiling ion code based on those is bound to fail.  Or something.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 6

4 years ago
Created attachment 8505945 [details] [diff] [review]
When looking for a common getter/setter function via sniffing baseline stubs, skip over the stubs which have a holder object whose shape doesn't match the cached shape in the stub: trying to compile code based on those is likely to just lead to immediate

Does this seem like a reasonable thing to do?
Attachment #8505945 - Flags: review?(jdemooij)
(Assignee)

Updated

4 years ago
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
(Assignee)

Comment 7

4 years ago
Created attachment 8505953 [details]
Testcase that shows a nice speedup from the patch
(Assignee)

Updated

4 years ago
Summary: Bailouts when calling performance.now() → Bailouts when calling performance.now() if the window's shape has changed since we compiled the ion code
(Assignee)

Comment 8

4 years ago
Eric, feel free to steal the review here.  ;)
Flags: needinfo?(efaustbmo)
(Assignee)

Comment 9

4 years ago
Though.... it's weird to leave the old stub in place at all, since it has a "holder" that has a known-bad shape.  Baseline doesn't use the "holder" I guess, but is there any situation in which that old stub is still useful?  Or should we either try to mutate it in place or do something similar to RemoveExistingGetElemNativeStubs when adding our new stub?
Flags: needinfo?(jdemooij)
And in particular, it seems like we could blow out the max stub count pretty easily as people resolve more things no Window.
Comment on attachment 8505945 [details] [diff] [review]
When looking for a common getter/setter function via sniffing baseline stubs, skip over the stubs which have a holder object whose shape doesn't match the cached shape in the stub: trying to compile code based on those is likely to just lead to immediate

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

::: js/src/jit/BaselineInspector.cpp
@@ +494,5 @@
>              ICGetPropCallGetter *nstub = static_cast<ICGetPropCallGetter *>(stub);
> +            if (nstub->holder()->lastProperty() != nstub->holderShape()) {
> +                // The shape of nstub->holder() changed since we generated this
> +                // stub.  Keep going, in hopes of finding a stub for which the
> +                // shape matches the current holder shape.

As discussed on IRC, I think we should change this code to ignore sites that (1) have the hadUnoptimizableAccess() flag, or (2) have multiple (conflicting) stubs attached. With these two checks we should avoid the repeated-bailout issue (we've had this issue before with Baseline IC sniffing).

Then we can change the BaselineIC code to update the old holder shape, or unlink the old stub, so that Ion can still use it.
Attachment #8505945 - Attachment description: When looking for a common getter/setter function via sniffing baseline stubs, skip over the stubs which have a holder object whose shape doesn't match the cached shape in the stub: trying to compile code based on those is likely to just lead to immediate → When looking for a common getter/setter function via sniffing baseline stubs, skip over the stubs which have a holder object whose shape doesn't match the cached shape in the stub: trying to compile code based on those is likely to just lead to immediate
Attachment #8505945 - Flags: review?(jdemooij)
Created attachment 8506589 [details] [diff] [review]
part 1.  Change BaselineInspector to not attempt to optimize to a common getter/setter if either we have getter/setter stubs with different holder shapes or have had an unoptimizable access
Attachment #8506589 - Flags: review?(efaustbmo)
Created attachment 8506590 [details] [diff] [review]
part 2.  Change baseline ICs to update getter/setter stubs in place instead of adding new stubs if the stub kind and holder match an existing stub (but the shape does not)
Attachment #8506590 - Flags: review?(efaustbmo)
(Assignee)

Updated

4 years ago
Attachment #8505945 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Flags: needinfo?(jdemooij)
Flags: needinfo?(efaustbmo)
Comment on attachment 8506589 [details] [diff] [review]
part 1.  Change BaselineInspector to not attempt to optimize to a common getter/setter if either we have getter/setter stubs with different holder shapes or have had an unoptimizable access

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

Othe than style, this looks fine. r=me

::: js/src/jit/BaselineInspector.cpp
@@ +498,5 @@
> +            if (!holder) {
> +                holder = nstub->holder();
> +                holderShape = nstub->holderShape();
> +                getter = nstub->getter();
> +            } else if (nstub->holderShape() != holderShape)

nit: SM style: If one if else half needs braces, all should have them. Here, both the else if and the else should be braced.

@@ +503,5 @@
> +                return nullptr;
> +            else
> +                MOZ_ASSERT(getter == nstub->getter());
> +        } else if (stub->isGetProp_Fallback() &&
> +                   stub->toGetProp_Fallback()->hadUnoptimizableAccess()) {

nit: SM style: Multi-line if conditions put the opening brace on the next line

@@ +515,4 @@
>  }
>  
>  JSObject *
>  BaselineInspector::commonSetPropFunction(jsbytecode *pc, Shape **lastProperty, JSFunction **commonSetter)

see above for similar nits.
Attachment #8506589 - Flags: review?(efaustbmo) → review+
Comment on attachment 8506590 [details] [diff] [review]
part 2.  Change baseline ICs to update getter/setter stubs in place instead of adding new stubs if the stub kind and holder match an existing stub (but the shape does not)

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

r=me with comments addressed.

::: js/src/jit/BaselineIC.cpp
@@ +5704,5 @@
> +                Shape *cachedReceiverShape;
> +                if (kind == ICStub::GetProp_CallNative) {
> +                    cachedReceiverShape = nullptr;
> +                } else {
> +                    ICGetPropCallPrototypeGetter *stubWithReceiver = 

nit: space after =

@@ +5712,5 @@
> +                // We would like to assert that either
> +                // getPropStub->holderShape() != holder->lastProperty() or
> +                // receiverShape != cachedReceiverShape, but that assertion can
> +                // fail if there is something in a getter that changes something
> +                // that we guard on in our stub but don't check for before/after

This is bug 1084150, right? If that lands, does this need to be changed? Can we add that assertion back?

@@ +5724,5 @@
> +                // have changed which getter we want to use.
> +                getPropStub->getter() = getter;
> +                if (receiverShape == cachedReceiverShape)
> +                    foundMatchingStub = true;
> +            }

OK, there needs to be a comment in here somewhere (either here, or at the top) stating that we have the side effect that we update all such stubs in place, not only the one we were looking for. I came into this expecting a |break| here and was surpised not to find it until the grand scheme made sense. Perhaps this function should even be called Update..Stubs

@@ +5734,5 @@
> +
> +// Try to update an existing SetProp setter call stub in place with a new shape
> +// and setter.
> +static bool
> +UpdateExistingSetPropCallStub(ICSetProp_Fallback* fallbackStub,

The same comments from above apply again in the trivial ways.
Attachment #8506590 - Flags: review?(efaustbmo) → review+
(In reply to Eric Faust [:efaust] from comment #15)
> r=me with comments addressed.

Come to think of it, this needs tests. Can we whip up something that hits this and put it in jit-tests? Including the multiple stub change case?
> This is bug 1084150, right?

Sadly, not quite.  The comment explains this, in fact: Bug 1084150 makes sure this doesn't happen due to changes in the receiver's shape, but it can still happen due to changes in the _holder_'s shape.

> Perhaps this function should even be called Update..Stubs

Yes.  Done.  Also updated the comment for the function:

  // Try to update all existing GetProp/GetName getter call stubs that match the
  // given holder in place with a new shape and getter. 

> Can we whip up something that hits this and put it in jit-tests?

What would it test?  The failure mode here is not generating optimal ion code, not actual correctness issues.  Unless you mean add a test that will just ensure we exercise these new codepaths in some way?
Flags: needinfo?(efaustbmo)
> but it can still happen due to changes in the _holder_'s shape.

Which, note, should be much rarer, since doing "this.foo = something" and thus adding a property to the receiver in a setter is a much more sane thing to do than modifying the prototype the setter lives on.  Of course Firefox front-end code does the latter too.  ;)
(In reply to Boris Zbarsky [:bz] from comment #17) 
> What would it test?  The failure mode here is not generating optimal ion
> code, not actual correctness issues.  Unless you mean add a test that will
> just ensure we exercise these new codepaths in some way?

Yeah, I would like to see the shape replacement code exercised with some proof that the resulting thing isn't functionally wrong? Maybe that's too paranoid.
Flags: needinfo?(efaustbmo)
Yes, that seems fair.  Let me see what I can do.
Created attachment 8511475 [details] [diff] [review]
test.patch

This at least catches it if I comment out the "setPropStub->setter() = setter;" and "getPropStub->getter() = getter;" lines...
Attachment #8511475 - Flags: review?(efaustbmo)
Comment on attachment 8511475 [details] [diff] [review]
test.patch

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

Yeah, this makes me feel better about our general coverage. r=me. Thanks for doing this.
Attachment #8511475 - Flags: review?(efaustbmo) → review+
Looks like either this or bug 1088002 regressed octane-box2d by around 40% on awfy.
This one would be more likely than the other...  Is there a way to run box2d locally?
Yes, go to js/src/octane and run   js run-box2d.js   .
OK, so doing this on inbound rev c30b79646349 (that's without this bug or bug 1088002) I get about 50k runs/s.

With rev 233974607185 I get about 30k runs/s.

I tried replacing the "return nullptr" in the holder shape mismatch cases with "continue", and that did not affect things: still 30k runs/s.

However if I replace the "return nullptr" in the hadUnoptimizableAccess() cases with "continue", performance comes back to what it used to be.  Both the setter and getter case matter; if I leave one of them but not the other returning null, I get about 40k runs/s.

Jan, should I just take those hadUnoptimizableAccess() checks out?  It's not quite obvious to me why we want them...
Flags: needinfo?(jdemooij)
(In reply to Boris Zbarsky [:bz] from comment #27)
> Jan, should I just take those hadUnoptimizableAccess() checks out?  It's not
> quite obvious to me why we want them...

You know why the access is unoptimizable?

The check is used to avoid this situation: we attach a Baseline stub and also see some unoptimizable cases, then we optimize in Ion based on the attached Baseline stub but will bailout each time we hit the unoptimizable cases.

If that's not a risk here we can remove the check, or not set the hadUnoptimizableAccess flag in this case?
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/233974607185
https://hg.mozilla.org/mozilla-central/rev/9d1f171c9c07
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
I filed bug 1091795 on the perf regression.
Depends on: 1091795
You need to log in before you can comment on or make changes to this bug.