Last Comment Bug 678615 - remove ExplainLiveExpectedGarbage
: remove ExplainLiveExpectedGarbage
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
: 523965 (view as bug list)
Depends on:
Blocks: CleanupCC 744103
  Show dependency treegraph
 
Reported: 2011-08-12 13:50 PDT by Andrew McCreight [:mccr8]
Modified: 2012-04-26 14:03 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remove or rename uses of DEBUG_CC outside of nsCycleCollector.cpp (23.30 KB, patch)
2011-08-12 15:16 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Review
remove it (41.20 KB, patch)
2012-04-04 12:13 PDT, Andrew McCreight [:mccr8]
bugs: review+
peterv: superreview+
Details | Diff | Review
remove FinishCycleCollection (4.16 KB, patch)
2012-04-09 13:43 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Review

Description Andrew McCreight [:mccr8] 2011-08-12 13:50:58 PDT
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] 2011-08-12 15:16:43 PDT
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
Comment 2 Andrew McCreight [:mccr8] 2011-08-12 16:09:39 PDT
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] 2011-08-12 16:38:39 PDT
I also removed mBytes from DEBUG_CC PtrInfo because nothing uses it.
Comment 4 Andrew McCreight [:mccr8] 2011-08-12 17:47:08 PDT
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] 2011-08-29 09:14:28 PDT
I'll need to update this Wiki page if I change this: https://wiki.mozilla.org/Performance:Leak_Tools
Comment 6 Andrew McCreight [:mccr8] 2011-09-01 16:39:54 PDT
*** Bug 523965 has been marked as a duplicate of this bug. ***
Comment 7 Andrew McCreight [:mccr8] 2012-04-04 10:45:09 PDT
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] 2012-04-04 12:13:54 PDT
Created attachment 612289 [details] [diff] [review]
remove it
Comment 9 Andrew McCreight [:mccr8] 2012-04-04 12:18:05 PDT
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] 2012-04-09 13:43:04 PDT
Created attachment 613383 [details] [diff] [review]
remove FinishCycleCollection

With the removal of ExplainLiveExpectedGarbage, this doesn't do anything any more.
Comment 11 Olli Pettay [:smaug] 2012-04-09 23:37:58 PDT
Comment on attachment 612289 [details] [diff] [review]
remove it

r=me
Comment 12 Peter Van der Beken [:peterv] 2012-04-18 07:20:33 PDT
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] 2012-04-22 08:01:36 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.