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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

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 1

[Till Schneidereit [:till]]

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

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 3

[Till Schneidereit [:till]]

Try-servering here: https://tbpl.mozilla.org/?tree=Try&rev=dadaf183da72

Comment 4

[Till Schneidereit [:till]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/951247ec2044

Comment 5

[Till Schneidereit [:till]]

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.

Comment 6

[Nicholas Nethercote [inactive]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Should this go on esr24 too?

Comment 9

[Till Schneidereit [:till]]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/951247ec2044

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/67beca04cda8
https://hg.mozilla.org/releases/mozilla-beta/rev/891f559ab340
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/52166aee7203

Comment 12

[u279076]

Does this have or need tests?

Comment 13

[Till Schneidereit [:till]]

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?

Comment 14

[Till Schneidereit [:till]]

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.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 16

[Till Schneidereit [:till]]

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

Comment 17

[u279076]

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

Comment 18

[Nicholas Nethercote [inactive]]

Given that this problem (a) occurred, and (b) was bad... I'm in favour of a test.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

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