911246 - Enable incremental cycle collection by default

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

ICC is going to land preffed off by default.

Comment 1

[Andreas Gal :gal]

This is seriously exciting. ++Andrew

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Enable incremental cycle collection by default.

For convenience for anybody who wants to try it when it is ready to be turned on,
here's a patch that will enable it.  It won't do anything yet.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Enable incremental cycle collection by default.

Comment 4

[Andrew McCreight [:mccr8]]

L64 debug run looks green: https://tbpl.mozilla.org/?tree=Try&rev=473197f4723e

Comment 5

[Andrew McCreight [:mccr8]]

Well, BC is quite orange (about 80% of the time).  I'll have to look into those failures.

Comment 6

[Andrew McCreight [:mccr8]]

-p all try run is green: https://tbpl.mozilla.org/?tree=Try&rev=c137fc13f44d
XP M4 looks green: https://tbpl.mozilla.org/?tree=Try&rev=98d6571b6f4c

I'm just waiting on review for one more patch blocking this bug.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8357210 [details] [diff] [review]
Enable incremental cycle collection by default.

Bug 996151 has landed, so this should be ready to land now.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8357210 [details] [diff] [review]
Enable incremental cycle collection by default.

woohoo

Comment 9

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4be5203a3c9

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e4be5203a3c9

Comment 11

[Florian Bender]

Do you want to uplift this so it goes into 31ESR? Or did this land after 31 branching intentionally?

Comment 12

[Andrew McCreight [:mccr8]]

There's no particular reason it needs to go into ESR31.

Comment 13

[Andrew McCreight [:mccr8]]

It looks like this reduced the 95th percentile of CYCLE_COLLECTOR_MAX_PAUSE from 82ms to around 40ms, and the 75th percentile from 26ms to 13ms.

http://telemetry.mozilla.org/#nightly/32/CYCLE_COLLECTOR_MAX_PAUSE

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: Backout bug 911246 from 32 for crashes.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8444597 [details] [diff] [review]
Backout bug 911246 from 32 for crashes.

r+ for Aurora. I think we should not disable icc on trunk.

Comment 16

[Andrew McCreight [:mccr8]]

Comment on attachment 8444597 [details] [diff] [review]
Backout bug 911246 from 32 for crashes.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 911246
User impact if declined: crashes
Testing completed (on m-c, etc.): this puts us back to the state we're in on beta
Risk to taking this patch (and alternatives if risky): should be low, we're just reverted to what we did before
String or IDL/UUID changes made by this patch: none

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 8444597 [details] [diff] [review]
Backout bug 911246 from 32 for crashes.

This approval request appears to have gotten lost, but I have a work in progress patch for the crash, so I'll skip this for now.

Comment 18

[Andrew McCreight [:mccr8]]

Comment on attachment 8444597 [details] [diff] [review]
Backout bug 911246 from 32 for crashes.

I have a patch in progress that fixes ICC, but it is getting a little tricky, so I think it is better to just disable ICC on Aurora and let my ICC fix ride the trains.

Approval Request Comment: See comment 16. This should fix top crashes on Aurora.

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8444597 [details] [diff] [review]
Backout bug 911246 from 32 for crashes.

Approved for Aurora. I look forward to watching ICC ride the 33 train.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/edba30a5adc9