Jon Coppeard (:jonco) (PTO until 14th September) Reporter
	Description • 10 years ago

Meta bug to track crashes associated with enableing Generational GC.

	u279076
	Comment 1 • 10 years ago

The following crashes are currently known to affect Beta:
 * Bug 1028972 - 925 crashes in the last week on Beta
 * Bug 999158 - 17314 crashes in the last week on Beta
 * Bug 991845 - 6721 crashes in the last week on Beta
 * Bug 991752 - 90 crashes in the last week on Beta
 * Bug 991746 - 82 crashes in the last week on Beta
 * Bug 991694 - Unsure if this is a concern for Beta

Total volume of unfixed GGC crashes currently stands at 25132 crashes in the last week on Beta. This accounts for about 9.11% of our crashes on Beta currently. Based on this I'm inclined to nominate GGC to be delayed to Firefox 32 (or later) and thus pref'd off for Firefox 31.

That said, I'm open to be convinced otherwise if someone has a solid argument.

	Andrew McCreight [:mccr8]
	Comment 2 • 10 years ago

About 95% of those crashes are Bug 991845 and Bug 999158, which look like OOM crashes.  Now, it is certainly possible that GGC is causing an increase in OOM crashes, because it requires more contiguous space than before, but maybe it is just causing OOM crashes to happen slightly sooner in a different place.  Is there any kind of comparison we can make about the overall "OOMiness" of beta vs release?

	u279076
	Comment 3 • 10 years ago

Doing a search for signatures containing OOM:
 * Firefox 31 -> 59915 crashes in 7 days with 468 signatures
 * Firefox 30 -> 104658 crashes in 7 days with 694 signatures

I know that's completely unscientific so maybe someone more experienced can look at the data.

	u279076
	Comment 4 • 10 years ago

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #3)
> Doing a search for signatures containing OOM:
>  * Firefox 31 -> 59915 crashes in 7 days with 468 signatures
>  * Firefox 30 -> 104658 crashes in 7 days with 694 signatures

Note, I think the numbers look worse on Beta after factoring in that ADIs on Release are much higher than Beta.

	Robert Kaiser
	Comment 5 • 10 years ago

The problem is that we have no good way of actually comparing there, as not all the OOM cases actually contain OOM in the signature - and we changed OOM reporting early in the the 31 beta cycle.

	Jon Coppeard (:jonco) (PTO until 14th September) Reporter
	Comment 6 • 10 years ago

I've commented in bug 991845 and bug 999158.

	u279076
	Comment 7 • 10 years ago

(In reply to Jon Coppeard (:jonco) from comment #6)
> I've commented in bug 991845 and bug 999158.

How reasonable is it that those can be addressed in this Beta cycle? I think we're pretty close to the end and we need to make a decision about whether GGC should ship or sit on Beta for another 6 weeks.

	Sylvestre Ledru [:Sylvestre]
	Comment 8 • 10 years ago

In case both fixes are not ready to land by beta 8 (tomorrow Monday), Jon, could you prepare a patch to disable GGC? Thanks. We will evaluate soon if we disable the feature for 31 or not.
(I am afraid beta 9 won't give us enough time for a proper coverage).

	Terrence Cole [:terrence] Assignee
	Comment 9 • 10 years ago

Until we can mitigate the new OOMs or discover (via this patch) that they are simply migrated from another signature.

	Jon Coppeard (:jonco) (PTO until 14th September) Reporter
	Comment 10 • 10 years ago

Comment on attachment 8452501 [details] [diff] [review]
backout_ggc_on_beta-v0.diff

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

r=me.

	Sylvestre Ledru [:Sylvestre]
	Comment 11 • 10 years ago

Terrence, could you checkin this patch and request the uplift? I would like to have this asap in beta. Thanks!

	Robert Kaiser
	Comment 12 • 10 years ago

(In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> Terrence, could you checkin this patch and request the uplift? I would like
> to have this asap in beta. Thanks!

I think it's only requesting uplift, this is targeted to land *only* on beta from what I understand.

	Sylvestre Ledru [:Sylvestre]
	Comment 13 • 10 years ago

Kairo, the patch is doing a bit more than disabling the feature. It is updating the configure argument. Since 31 is an ESR, I guess we want to remain consistent over the configure arguments over the various version.

	u279076
	Comment 14 • 10 years ago

(In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> Kairo, the patch is doing a bit more than disabling the feature. It is
> updating the configure argument. Since 31 is an ESR, I guess we want to
> remain consistent over the configure arguments over the various version.

Oh geez, I completely forgot that 31 is an ESR. That makes me even more sure that we should disable this, and happy to see that we are.

	Robert Kaiser
	Comment 15 • 10 years ago

(In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> Kairo, the patch is doing a bit more than disabling the feature. It is
> updating the configure argument. Since 31 is an ESR, I guess we want to
> remain consistent over the configure arguments over the various version.

The whole patch is only deactivating from what I see. As this is a build-time flag, the configure argument also needs to be reversed to deactivate by default for anyone else who build it.

	Terrence Cole [:terrence] Assignee
	Comment 16 • 10 years ago

(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #12)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> > Terrence, could you checkin this patch and request the uplift? I would like
> > to have this asap in beta. Thanks!
> 
> I think it's only requesting uplift, this is targeted to land *only* on beta
> from what I understand.

That is correct.

(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #15)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> > Kairo, the patch is doing a bit more than disabling the feature. It is
> > updating the configure argument. Since 31 is an ESR, I guess we want to
> > remain consistent over the configure arguments over the various version.
> 
> The whole patch is only deactivating from what I see. As this is a
> build-time flag, the configure argument also needs to be reversed to
> deactivate by default for anyone else who build it.

That is correct. This patch was generated with |hg backout <commit-enabling-ggc>|. In this case we are remaining consistent with the /prior/ version, which had no GGC. This configuration is still, of course, extremely well tested -- it is what is currently shipping in FF30.

	Terrence Cole [:terrence] Assignee
	Comment 17 • 10 years ago

Comment on attachment 8452501 [details] [diff] [review]
backout_ggc_on_beta-v0.diff

Approval Request Comment
[Feature/regressing bug #]: GGC
[User impact if declined]: Extra crashes under OOM situations.
[Describe test coverage new/current, TBPL]: Extensive.
[Risks and why]: None.
[String/UUID change made/needed]: None.

	Sylvestre Ledru [:Sylvestre]
	Comment 18 • 10 years ago

Comment on attachment 8452501 [details] [diff] [review]
backout_ggc_on_beta-v0.diff

Thanks. Hopefully, ggc will be part of 32.

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 10 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/c4249580b856

	Sylvestre Ledru [:Sylvestre]
	Comment 20 • 10 years ago

Updating the tracking flags: crashes are fixed by the disabling 
Disabled has been set in bug 619558

	Jon Coppeard (:jonco) (PTO until 14th September) Reporter
	Comment 21 • 8 years ago

Generational GC has been enabled since FF32 and there is no significant crash volume any more.