Closed Bug 1242462 Opened 4 years ago Closed 4 years ago

Do not prevent OSR as long as we do not reach the frequent bailout limit.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(4 files, 2 obsolete files)

CanEnterAtBranches does not follow the same rule as CheckFrequentBailouts, this causes us to bailout out of hot loops and never attempt to re-enter Ion code until we invalidate or re-execute the function (which might never happen).

This blocks any optimization which is making attempts at optimizing the code based on likely-assumptions, such as Bug 1229813.  As likely assumptions are unlikely to fail, and to remain in baseline if we never invalidate / nor re-execute the functions.
This patch add some spew when IonBailouts is enabled, such that we can see
in the log when we enter/leave Ion.
Attachment #8711724 - Flags: review?(hv1989)
(see comment 0)
Attachment #8711725 - Flags: review?(jdemooij)
Comment on attachment 8711724 [details] [diff] [review]
Add spew on MOSREntry and MBail.

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

::: js/src/jit/CodeGenerator.cpp
@@ +2327,5 @@
>      setOsrEntryOffset(masm.size());
>  
> +    if (JitSpewEnabled(JitSpew_IonBailouts)) {
> +        masm.move32(Imm32(lir->mirRaw()->block()->getSuccessor(0)->id()), temp);
> +        masm.printf("[IonBailouts] OSR Into block %d\n", temp);

Don't think we need to spew this on JitSpew_IonBailouts. Might be good to add a JitSpew_Osr, which also reports if we reach into the normal graph, or if we bail during the OSR block?

@@ +3830,5 @@
> +        AllocatableGeneralRegisterSet regs(GeneralRegisterSet::All());
> +        Register temp = regs.takeAny();
> +        masm.Push(temp);
> +        masm.move32(Imm32(lir->mirRaw()->block()->id()), temp);
> +        masm.printf("[IonBailouts] Bailout from block %d\n", temp);

Why not adding info about the instruction id()?
Attachment #8711724 - Flags: review?(hv1989)
(In reply to Hannes Verschore [:h4writer] from comment #3)
> Comment on attachment 8711724 [details] [diff] [review]
> Add spew on MOSREntry and MBail.
> 
> Review of attachment 8711724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/CodeGenerator.cpp
> @@ +2327,5 @@
> >      setOsrEntryOffset(masm.size());
> >  
> > +    if (JitSpewEnabled(JitSpew_IonBailouts)) {
> > +        masm.move32(Imm32(lir->mirRaw()->block()->getSuccessor(0)->id()), temp);
> > +        masm.printf("[IonBailouts] OSR Into block %d\n", temp);
> 
> Don't think we need to spew this on JitSpew_IonBailouts. Might be good to
> add a JitSpew_Osr, which also reports if we reach into the normal graph, or
> if we bail during the OSR block?

I did not add JitSpew_Osr because I figured that this would be the only instances of it.
I found it quite useful to know when we enter, to know if we bailed out unexpectedly.

Should I add a JitSpew_Entry / JitSpew_Return ?
Should I split that and only submit the MBail part of the patch?

> @@ +3830,5 @@
> > +        AllocatableGeneralRegisterSet regs(GeneralRegisterSet::All());
> > +        Register temp = regs.takeAny();
> > +        masm.Push(temp);
> > +        masm.move32(Imm32(lir->mirRaw()->block()->id()), temp);
> > +        masm.printf("[IonBailouts] Bailout from block %d\n", temp);
> 
> Why not adding info about the instruction id()?

Because we already have this information for every bailout, and not only MBail instruction.  The only missing info is the block number, which is quite faster to lookup in iongraph than the instruction number.
(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> (In reply to Hannes Verschore [:h4writer] from comment #3)
> > Comment on attachment 8711724 [details] [diff] [review]
> > Add spew on MOSREntry and MBail.
> > 
> > Review of attachment 8711724 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/jit/CodeGenerator.cpp
> > @@ +2327,5 @@
> > >      setOsrEntryOffset(masm.size());
> > >  
> > > +    if (JitSpewEnabled(JitSpew_IonBailouts)) {
> > > +        masm.move32(Imm32(lir->mirRaw()->block()->getSuccessor(0)->id()), temp);
> > > +        masm.printf("[IonBailouts] OSR Into block %d\n", temp);
> > 
> > Don't think we need to spew this on JitSpew_IonBailouts. Might be good to
> > add a JitSpew_Osr, which also reports if we reach into the normal graph, or
> > if we bail during the OSR block?
> 
> I did not add JitSpew_Osr because I figured that this would be the only
> instances of it.
> I found it quite useful to know when we enter, to know if we bailed out
> unexpectedly.
> 
> Should I add a JitSpew_Entry / JitSpew_Return ?
> Should I split that and only submit the MBail part of the patch?

I agree that we don't have a lot of OSR spew currently. But the info given here is definitely not part of JitSpew_IonBailouts. Although they were important to see at the same time for this bug. I would be ok with any of the following things:
-> add JitSpew_Osr which gives the above information + script + info about the typebarriers in the osr block + a statement that says we left the osr block into Ion itself.
-> Have JitSpew_Entry and JitSpew_Exit
-> not submit this part


> 
> > @@ +3830,5 @@
> > > +        AllocatableGeneralRegisterSet regs(GeneralRegisterSet::All());
> > > +        Register temp = regs.takeAny();
> > > +        masm.Push(temp);
> > > +        masm.move32(Imm32(lir->mirRaw()->block()->id()), temp);
> > > +        masm.printf("[IonBailouts] Bailout from block %d\n", temp);
> > 
> > Why not adding info about the instruction id()?
> 
> Because we already have this information for every bailout, and not only
> MBail instruction.  The only missing info is the block number, which is
> quite faster to lookup in iongraph than the instruction number.

Oh. It didn't occur to me this was only on the MBail instruction. That seem restrictive? Shouldn't we print the block id for all bailing instruction. I think this information is also interesting for those?
Comment on attachment 8711725 [details] [diff] [review]
Allow OSR until we reached the frequent bailout threshold.

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

::: js/src/jit/Ion.cpp
@@ +2502,5 @@
>  
>      // Skip if the code is expected to result in a bailout.
> +    if (script->hasIonScript()) {
> +        if (script->ionScript()->numBailouts() >= JitOptions.frequentBailoutThreshold)
> +            return Method_Skipped;

There are other places where we call bailoutExpected. What if we add this check to bailoutExpected instead, to fix all callers?
Flags: needinfo?(nicolas.b.pierron)
This sounds like a good idea, I will test and re-ask for review after.  I am a bit worried about [1], as I recall doing a similar modification in the past with terrible benchmark results.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/Ion.cpp#2720
Flags: needinfo?(nicolas.b.pierron)
Attachment #8711725 - Flags: review?(jdemooij)
This does what got suggested in comment 6. I verified on benchmarks that
this did not change much of performances on kraken and octane.
Attachment #8712217 - Flags: review?(jdemooij)
Attachment #8711725 - Attachment is obsolete: true
Comment on attachment 8712217 [details] [diff] [review]
Allow IonMonkey re-entry until we reached the frequent bailout threshold.

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

Thanks!
Attachment #8712217 - Flags: review?(jdemooij) → review+
I had to back this out for apparently breaking browser_CTP_crashreporting.js:

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

https://treeherder.mozilla.org/logviewer.html#?job_id=20514485&repo=mozilla-inbound
Flags: needinfo?(nicolas.b.pierron)
(In reply to Wes Kocher (:KWierso) from comment #11)
> I had to back this out for apparently breaking browser_CTP_crashreporting.js:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b8dce586f413
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=20514485&repo=mozilla-
> inbound

I am able to reproduce it on the try server, but I fail to reproduce it locally so far.
Flags: needinfo?(nicolas.b.pierron)
I have no idea how to reproduce this issue, nor how to investigate what might be going wrong inside browser_CTP_crashreporting.js.  I did a bit of instrumentation, and sent it to try, but this does not help me much in figuring out what might be the issue:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc86620bb234
https://treeherder.mozilla.org/logviewer.html#?job_id=16004620&repo=try

Adding Date.now() in the reported messages show that setInterval is a bit slower compared to the 100ms expected interval, but works.

This issue is probably caused by a function which is being jitted and which bailout in a loop failing to push the state beyond the "submitting" state.

I tried to set the following pref while running the mochitest, but this did not help in reproducing the issue:
  javascript.options.ion.offthread_compilation=false
  javascript.options.ion.unsafe_eager_compilation=true

Georg, Jared, do you have any idea what might set the "status" attribute [1] to "success"?

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/browser_CTP_crashreporting.js#138
Flags: needinfo?(jaws)
Flags: needinfo?(gfritzsche)
Flags: needinfo?(jaws)
No need for explicit stderr, as this is already the default argument.
Attachment #8716352 - Flags: review?(jdemooij)
Attachment #8716348 - Attachment is obsolete: true
Attachment #8716348 - Flags: review?(jdemooij)
Depends on: 1246229
Comment on attachment 8716352 [details] [diff] [review]
Add a newline when calling TypeSet::print from a debugger. r=

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

\o/
Attachment #8716352 - Flags: review?(jdemooij) → review+
Depends on: 929301
(In reply to Wes Kocher (:KWierso) from comment #11)
> I had to back this out for apparently breaking browser_CTP_crashreporting.js:

This issue (Bug 929301) seems to be caused by a problem of handling for-of loops, which might be related to a recent regression of misc-basic-array-forof micro benchmarks [1], which has the same symptoms.

The problems seems to be that we OSR, into the for-of loop but that the iterator object has a different type-object, which causes the type-barrier of the OSR block to cause a bailout.

As we bailout straight from the OSR blocks, we effectively waste time, as we do not benefit from Ion compiled code.  This causes the "parseMultipartForm" [2] function to take much more time to execute, and thus causes a timeout on overloaded systems such as try slaves.

[1] https://arewefastyet.com/#machine=28&view=single&suite=misc&subtest=basic-array-forof&start=1452598325&end=1452647879
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/browser/crashreport.sjs#34
Depends on: 1248412
(In reply to Nicolas B. Pierron [:nbp] from comment #19)
> (In reply to Wes Kocher (:KWierso) from comment #11)
> > I had to back this out for apparently breaking browser_CTP_crashreporting.js:
> 
> This issue (Bug 929301) seems to be caused by a problem of handling for-of
> loops, which might be related to a recent regression of
> misc-basic-array-forof micro benchmarks [1], which has the same symptoms.

And fixed in Bug 1248412 (attachment 8719775 [details] [diff] [review]).
Keywords: leave-open
This is adding some machinery which uses the pid to spew the usual json/c1
output in different files.  This way we can use IONFLAGS=logs
IONFILTER=... when running Firefox with e10s enabled.
Attachment #8720765 - Flags: review?(hv1989)
Attachment #8720765 - Flags: review?(hv1989) → review+
This is a feature which is Blocking Bug 1229813, there is no reason to track firefox 46.
Keywords: leave-open
Target Milestone: --- → mozilla47
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.