The default bug view has changed. See this FAQ.

remove ExplainLiveExpectedGarbage

RESOLVED FIXED in mozilla15

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Assignee: nobody → continuation
(Assignee)

Comment 1

6 years ago
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
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
I also removed mBytes from DEBUG_CC PtrInfo because nothing uses it.
(Assignee)

Comment 4

6 years ago
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...
(Assignee)

Comment 5

6 years ago
I'll need to update this Wiki page if I change this: https://wiki.mozilla.org/Performance:Leak_Tools
(Assignee)

Updated

6 years ago
Duplicate of this bug: 523965
(Assignee)

Comment 7

5 years ago
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.
Summary: clean up DEBUG_CC uses outside of the cycle collector → remove ExplainLiveExpectedGarbage
(Assignee)

Comment 8

5 years ago
Created attachment 612289 [details] [diff] [review]
remove it
Attachment #552774 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #612289 - Flags: superreview?(peterv)
Attachment #612289 - Flags: review?(bugs)
(Assignee)

Comment 10

5 years ago
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
Attachment #612289 - Flags: review?(bugs) → review+
(Assignee)

Updated

5 years ago
Attachment #613383 - Flags: review?(bugs)

Updated

5 years ago
Attachment #613383 - Flags: review?(bugs) → review+
(Assignee)

Updated

5 years ago
Blocks: 744103
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?
Attachment #612289 - Flags: superreview?(peterv) → superreview+
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a454f62db572
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2bececb50a1
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/a454f62db572
https://hg.mozilla.org/mozilla-central/rev/d2bececb50a1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 749370
You need to log in before you can comment on or make changes to this bug.