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.
Created attachment 552774 [details] [diff] [review] 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
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.
I also removed mBytes from DEBUG_CC PtrInfo because nothing uses it.
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...
I'll need to update this Wiki page if I change this: https://wiki.mozilla.org/Performance:Leak_Tools
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.
Created attachment 612289 [details] [diff] [review] remove it
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.
Created attachment 613383 [details] [diff] [review] remove FinishCycleCollection With the removal of ExplainLiveExpectedGarbage, this doesn't do anything any more.
Comment on attachment 612289 [details] [diff] [review] remove it r=me
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?
(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.