Perform shrinking GC if nsJSContext::GarbageCollectNow is called with aShrinking == ShrinkingGC.

RESOLVED FIXED in Firefox 27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: till, Assigned: till)

Tracking

unspecified
mozilla29
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox27 fixed, firefox28 fixed, firefox29 fixed, firefox-esr24 wontfix, b2g18 wontfix, b2g-v1.1hd wontfix, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment)

In bug 758034, the browser's GC api was refactored. That regressed the memory-pressure GCs, which now don't do shrinking GCs anymore.
Comment on attachment 8358643 [details] [diff] [review]
Perform shrinking GC if nsJSContext::GarbageCollectNow is called with aShrinking == ShrinkingGC.

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

Thanks. We should backport this.
Attachment #8358643 - Flags: review?(wmccloskey) → review+
Comment on attachment 8358643 [details] [diff] [review]
Perform shrinking GC if nsJSContext::GarbageCollectNow is called with aShrinking == ShrinkingGC.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 758034
User impact if declined: potentially more OOM crashes
Testing completed: landed on m-i, full try run
Risk to taking this patch (and alternatives if risky): extremely low. Makes shrinking GCs more likely to happen, nothing else
String or UUID changes made by this patch: none

Given that this might reduce OOM crashes and is extremely low-risk, I think it should be uplifted to various b2g branches, too. I don't know how the process for that works, though.
Attachment #8358643 - Flags: approval-mozilla-beta?
Attachment #8358643 - Flags: approval-mozilla-aurora?
> Given that this might reduce OOM crashes and is extremely low-risk, I think
> it should be uplifted to various b2g branches, too. I don't know how the
> process for that works, though.

khuey knows...
Flags: needinfo?(khuey)
Comment on attachment 8358643 [details] [diff] [review]
Perform shrinking GC if nsJSContext::GarbageCollectNow is called with aShrinking == ShrinkingGC.

V1.3 is covered by the Aurora nom. For v1.2, you want b2g26 approval

More info:
https://wiki.mozilla.org/Release_Management/B2G_Landing
Attachment #8358643 - Flags: approval-mozilla-b2g26?
Flags: needinfo?(khuey)
Should this go on esr24 too?
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #8)
> Should this go on esr24 too?

Hmm, maybe. Let's wait what the drivers say about the other uplifts, first?
https://hg.mozilla.org/mozilla-central/rev/951247ec2044
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Attachment #8358643 - Flags: approval-mozilla-beta?
Attachment #8358643 - Flags: approval-mozilla-beta+
Attachment #8358643 - Flags: approval-mozilla-b2g26?
Attachment #8358643 - Flags: approval-mozilla-b2g26+
Attachment #8358643 - Flags: approval-mozilla-aurora?
Attachment #8358643 - Flags: approval-mozilla-aurora+
Keywords: checkin-needed
Does this have or need tests?
Flags: needinfo?(till)
Flags: in-testsuite?
The reason this wasn't caught any earlier is that it's not really testable, afaik. ni?'ing billm for verification, though. Bill, could we somehow test that sending a memory-pressure event causes a shrinking instead of a normal gc?
Flags: needinfo?(till) → needinfo?(wmccloskey)
Comment on attachment 8358643 [details] [diff] [review]
Perform shrinking GC if nsJSContext::GarbageCollectNow is called with aShrinking == ShrinkingGC.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: might reduce crash rates, extremely low risk
User impact if declined: pontially more OOM crashes
Fix Landed on Version: first on m-c, uplifted to aurora and beta
Risk to taking this patch (and alternatives if risky): extremely now
String or UUID changes made by this patch: none

Given that this patch, as expected, didn't cause any problems on any branch, we might as well uplift it to ESR in the hopes of reducing crashes with pretty much no downsides.
Attachment #8358643 - Flags: approval-mozilla-esr24?
(In reply to Till Schneidereit [:till] from comment #13)
> The reason this wasn't caught any earlier is that it's not really testable,
> afaik. ni?'ing billm for verification, though. Bill, could we somehow test
> that sending a memory-pressure event causes a shrinking instead of a normal
> gc?

We could fire an observer notification when a GC happens and then a test could listen for that. The notification would include details like whether it's a shrinking GC. I'm afraid I don't have time to work on this though.
Flags: needinfo?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #15)
> We could fire an observer notification when a GC happens and then a test
> could listen for that. The notification would include details like whether
> it's a shrinking GC. I'm afraid I don't have time to work on this though.

While that sounds good, I'm not sure the effort is warranted. The regression this bug fixes isn't something that we should expect to happen again soon, so I don't think investing quite a bit of work into preventing the reoccurrence is worth it.
(In reply to Till Schneidereit [:till] from comment #16)
> (In reply to Bill McCloskey (:billm) from comment #15)
> > We could fire an observer notification when a GC happens and then a test
> > could listen for that. The notification would include details like whether
> > it's a shrinking GC. I'm afraid I don't have time to work on this though.
> 
> While that sounds good, I'm not sure the effort is warranted. The regression
> this bug fixes isn't something that we should expect to happen again soon,
> so I don't think investing quite a bit of work into preventing the
> reoccurrence is worth it.

Based on the choices in front of us:
A) Investigate and develop a test to ensure this doesn't regress ever again
 - or -
B) Investigate, develop, and test a fix for a shipped regression

...it seems like you think A is more work and B is unlikely to happen. Isn't it more work in the long run if B happens though?
Given that this problem (a) occurred, and (b) was bad... I'm in favour of a test.
Comment on attachment 8358643 [details] [diff] [review]
Perform shrinking GC if nsJSContext::GarbageCollectNow is called with aShrinking == ShrinkingGC.

That's not how the ESR branch works. This isn't a security fix or an exceptionally high volume crash that we can attribute to on that channel so there's actually no reason to take extra churn on this branch. 

see: https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
Attachment #8358643 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24-
You need to log in before you can comment on or make changes to this bug.