678615 - remove ExplainLiveExpectedGarbage

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Outside of nsCycleCollector.cpp, DEBUG_CC is now only used in a few places, for explainLiveExpectedGarbage (eLEG).  We can clean up a few of those places, remove outdated references to it in comments, and rename it to something more specific.  This will make it clear that to enable eg cycle collector logging, you don't really need to touch nsCycleCollectionParticipant.h, which is a pain because it is included everywhere and causes huge rebuilds.  There's probably room for a followup that splits out DEBUG_CC within nsCycleCollector.cpp into multiple compile options.

Here is where it appears outside of nsCycleCollector.cpp according to MXR:
- nsCycleCollector.h (nsCC.h): defines DEBUG_CC, uses it for eLEG
- nsCycleCollectionParticipant (nsCCP.h): defines DEBUG_CC, does not use it
- xpcprivate.h: used for eLEG, includes nsCCP.h and nsCC.h
- nsXPConnect: used for eLEG, includes xpcprivate.h

Here's the steps I am thinking of:
- nsCPP.h: remove DEBUG_CC entirely (just in a comment)
- nsCC.h, xpcprivate.h, nsXPConnect: rename DEBUG_CC to something like CC_EXPLAIN_LIVE
- nsXPConnect: fix outdated comments about DEBUG_CC
- nsCC.cpp: if CC_EXPLAIN_LIVE is defined, define DEBUG_CC.  alternatively, replace uses of DEBUG_CC with CC_EXPLAIN_LIVE where appropriate, though they may be a bit tangled up.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: remove or rename uses of DEBUG_CC outside of nsCycleCollector.cpp

WIP

This renames DEBUG_CC outside of nsCycleCollector.cpp to CC_EXPLAIN_LIVE, which is what it is used for.  Within nsCC.cpp, uses of DEBUG_CC are replaced with CC_EXPLAIN_LIVE as appropriate.  For simplicity, CC_EXPLAIN_LIVE implies DEBUG_CC.

My testing plan is to make sure the browser builds and runs for a few minutes with these combinations:
1. as is (should be identical to current)
2. DEBUG_CC set in nsCycleCollector.cpp
3. EXPLAIN_LIVE_CC set in nsCycleCollector.h

Comment 2

[Andrew McCreight [:mccr8]]

Briefly, the advantage of this patch is that most of the features of DEBUG_CC can be enabled only be recompiling nsCycleCollector.cpp, instead of causing a ton of things to need to be rebuilt.  The disadvantage is mainly that explainLiveExpectedGarbage will probably bitrot more because it will be getting built and run less.

I tried it in combination 1 and 2 described above.  Still need to try EXPLAIN_LIVE_CC.

Comment 3

[Andrew McCreight [:mccr8]]

I also removed mBytes from DEBUG_CC PtrInfo because nothing uses it.

Comment 4

[Andrew McCreight [:mccr8]]

Does this sound like a reasonable thing to do, Peter?

Currently my patch hangs almost right away with EXPLAIN_LIVE_CC set (the third condition above).  I need to figure out if that is due to my patch, or if DEBUG_CC is just broken on trunk right now...

Comment 5

[Andrew McCreight [:mccr8]]

I'll need to update this Wiki page if I change this: https://wiki.mozilla.org/Performance:Leak_Tools

Comment 7

[Andrew McCreight [:mccr8]]

With the state of CC logging, it probably makes more sense to just remove ExplainLiveExpectedGarbage.  It adds a lot of complexity, and nobody seems to be using it.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: remove it

Comment 9

[Andrew McCreight [:mccr8]]

The miscellaneous code moving around I did was to eliminate compiler warnings with DEBUG_CC about initialization order and an unused variable.  I built and ran this with and without DEBUG_CC.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: remove FinishCycleCollection

With the removal of ExplainLiveExpectedGarbage, this doesn't do anything any more.

Comment 11

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

Comment on attachment 612289 [details] [diff] [review]
remove it

r=me

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 612289 [details] [diff] [review]
remove it

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

::: js/xpconnect/src/nsXPConnect.cpp
@@ -675,5 @@
>  }
>  
> -#ifdef DEBUG_CC
> -void
> -nsXPConnect::PrintAllReferencesTo(void *p)

I use this sometimes to print traces. Maybe move it to XPCDebug?

Comment 13

[Andrew McCreight [:mccr8]]

(In reply to Peter Van der Beken [:peterv] from comment #12)
> I use this sometimes to print traces. Maybe move it to XPCDebug?

Sure, I'll move it to XPCDebug and rename it to xpc_PrintAllReferencesTo, if that sounds okay to you.

Comment 14

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a454f62db572
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2bececb50a1

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/a454f62db572
https://hg.mozilla.org/mozilla-central/rev/d2bececb50a1