Nicolas B. Pierron [:nbp] Assignee
	Description • 9 years ago

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.

	Nicolas B. Pierron [:nbp] Assignee
	Comment 1 • 9 years ago

This patch add some spew when IonBailouts is enabled, such that we can see
in the log when we enter/leave Ion.

	Nicolas B. Pierron [:nbp] Assignee
	Comment 2 • 9 years ago

(see comment 0)

	Hannes Verschore [:h4writer]
	Comment 3 • 9 years ago

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()?

	Nicolas B. Pierron [:nbp] Assignee
	Comment 4 • 9 years ago

(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.

	Hannes Verschore [:h4writer]
	Comment 5 • 9 years ago

(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?

	Jan de Mooij [:jandem]
	Comment 6 • 9 years ago

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?

	Nicolas B. Pierron [:nbp] Assignee
	Comment 7 • 9 years ago

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

	Nicolas B. Pierron [:nbp] Assignee
	Comment 8 • 9 years ago

This does what got suggested in comment 6. I verified on benchmarks that
this did not change much of performances on kraken and octane.

	Jan de Mooij [:jandem]
	Comment 9 • 9 years ago

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!

	Pulsebot
	Comment 10 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e1d68784ae3

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 11 • 9 years ago

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

	Nicolas B. Pierron [:nbp] Assignee
	Comment 12 • 9 years ago

(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.

	Nicolas B. Pierron [:nbp] Assignee
	Comment 13 • 9 years ago

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

	Georg Fritzsche [:gfritzsche]
	Comment 14 • 9 years ago

(In reply to Nicolas B. Pierron [:nbp] from comment #13)
> 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

This should be from one of these places:
https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/browser/modules/PluginContent.jsm#928
https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/browser/modules/PluginContent.jsm#984

	Nicolas B. Pierron [:nbp] Assignee
	Comment 16 • 9 years ago

No need for explicit stderr, as this is already the default argument.

	Jan de Mooij [:jandem]
	Comment 17 • 9 years ago

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/

	Mike Conley (:mconley) (:⚙️)
	Comment 18 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=260bf2535e12

	Nicolas B. Pierron [:nbp] Assignee
	Comment 19 • 9 years ago

(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

	Nicolas B. Pierron [:nbp] Assignee
	Comment 20 • 9 years ago

(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]).

	Nicolas B. Pierron [:nbp] Assignee
	Comment 21 • 9 years ago

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.

	Pulsebot
	Comment 22 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/192d80d2d1de
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9026a602088
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ebf44bd3c60

	Carsten Book [:Tomcat]
	Comment 23 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/192d80d2d1de
https://hg.mozilla.org/mozilla-central/rev/a9026a602088
https://hg.mozilla.org/mozilla-central/rev/0ebf44bd3c60

	Nicolas B. Pierron [:nbp] Assignee
	Comment 24 • 8 years ago

This is a feature which is Blocking Bug 1229813, there is no reason to track firefox 46.