Closed Bug 994589 Opened 6 years ago Closed 3 years ago

[meta] Generational GC crashes

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: jonco, Assigned: terrence)

References

(Blocks 1 open bug)

Details

(Keywords: meta)

Attachments

(1 file)

Meta bug to track crashes associated with enableing Generational GC.
Depends on: 999158
Depends on: 992535
Keywords: meta
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.
Flags: needinfo?(release-mgmt)
Flags: needinfo?(jcoppeard)
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?
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.
(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.
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.
Flags: needinfo?(jcoppeard)
I've commented in bug 991845 and bug 999158.
(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.
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).
Flags: needinfo?(jcoppeard)
Flags: needinfo?(release-mgmt)
Until we can mitigate the new OOMs or discover (via this patch) that they are simply migrated from another signature.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8452501 - Flags: review?(jcoppeard)
Flags: needinfo?(jcoppeard)
Comment on attachment 8452501 [details] [diff] [review]
backout_ggc_on_beta-v0.diff

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

r=me.
Attachment #8452501 - Flags: review?(jcoppeard) → review+
Terrence, could you checkin this patch and request the uplift? I would like to have this asap in beta. Thanks!
Flags: needinfo?(terrence)
(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.
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.
(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.
(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.
(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.
Flags: needinfo?(terrence)
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.
Attachment #8452501 - Flags: approval-mozilla-beta?
Comment on attachment 8452501 [details] [diff] [review]
backout_ggc_on_beta-v0.diff

Thanks. Hopefully, ggc will be part of 32.
Attachment #8452501 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updating the tracking flags: crashes are fixed by the disabling 
Disabled has been set in bug 619558
No longer depends on: 991847
Blocks: 885550
No longer blocks: GenerationalGC
Blocks: GC.stability
No longer blocks: 885550
Generational GC has been enabled since FF32 and there is no significant crash volume any more.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.