The default bug view has changed. See this FAQ.

Implement a version of nsICycleCollectorListener for devtools

RESOLVED FIXED

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 11 obsolete attachments)

(Assignee)

Description

5 years ago
It would be great to have some tool to analyze possible runtime leaks and mem usage a page 
can cause. Currently there is just, AFAIK, one implementation of nsICycleCollectorListener, 
but maybe there could be another one for devtools.
nice idea!

What are you thinking for a display? Maybe an event in the web console?
(Assignee)

Comment 2

5 years ago
event?

I'm not quite sure how CC could be utilized. Would need to brainstorm this a bit :)
Perhaps the log could be created using allTraces()
https://wiki.mozilla.org/Performance:Leak_Tools#Cycle_collector_heap_dump
and then JS could visualize the graph. Basically trying to show what all objects are
alive (in CC terms) in a tab.
(Assignee)

Updated

5 years ago
Blocks: 722750
(Assignee)

Updated

5 years ago
Assignee: nobody → bugs
(Assignee)

Comment 3

5 years ago
Created attachment 597515 [details] [diff] [review]
wip
(Assignee)

Comment 4

5 years ago
Created attachment 597532 [details]
CC log printer

about:cc
(Assignee)

Updated

5 years ago
Attachment #597532 - Attachment is patch: false
Attachment #597532 - Attachment mime type: text/plain → application/zip
(Assignee)

Comment 5

5 years ago
Created attachment 597657 [details]
some more functionality

Ugly UI, but it has most of the functionality I use from my perl script.
Attachment #597532 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #597515 - Flags: review?(continuation)
I tried the extension with Firefox Nightly:
Built from http://hg.mozilla.org/mozilla-central/rev/d45c7d7b0079

and I am seeing an exception, whenever I click "Run cycle collector"

Timestamp: 16.2.2012 10:05:48
Error: uncaught exception: [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: resource://cclog-addon/aboutcc.js :: cc :: line 64"  data: no]

Clicking "Number of garbage objects" or "Number of root objects" says 0
(but I guess it could be related to that exception)

(running on Win Vista)

Honza
(Assignee)

Comment 7

5 years ago
Created attachment 597819 [details]
faster graph analysis

Honza, the addon requires the core patch (which is waiting for review).
Attachment #597657 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 597845 [details]
Some tweaks
Attachment #597819 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #597845 - Attachment mime type: application/octet-stream → application/x-xpinstall
(Assignee)

Comment 9

5 years ago
Created attachment 597868 [details]
don't hang so easily with *huge* graphs
Attachment #597845 - Attachment is obsolete: true
Blocks: 723783
No longer blocks: 722750
(Assignee)

Comment 10

5 years ago
Created attachment 598079 [details]
Some tweaks
Attachment #597868 - Attachment is obsolete: true
Comment on attachment 597515 [details] [diff] [review]
wip

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

Well, I'm not sure if I'm on board with mashing two different listeners together, if only because of all the getter/setter stuff it requires, but here are some comments on what you have.  Thanks a lot for doing this, I've been meaning to look at it, but I haven't gotten around to it.

I wonder if it would make sense/is possible to split this off into another file.

::: xpcom/base/nsCycleCollector.cpp
@@ +1424,5 @@
> +
> +  enum Type
> +  {
> +    eRefCountedObject,
> +    eGCedObject,

you could have a type for marked and unmarked GCed objects, then avoid the bool mGCMarked.  Not a big deal.

@@ +1444,5 @@
>  {
>  public:
> +    nsCycleCollectorLogger() :
> +      mStream(nsnull), mWantAllTraces(false),
> +      mDisableLog(false), mWantAfterProcessing(false),

These two flags should be consistent, so you don't check one for true and one for false all over the place.  Maybe mWantLogToFile and mWantSaveLog or something?

@@ +1649,5 @@
>  
> +    NS_IMETHOD ProcessNext(nsICycleCollectorHandler* aHandler,
> +                           bool* aCanContinue)
> +    {
> +        NS_ENSURE_STATE(aHandler);

Do we maybe want an assert here for mWantAfterProcessing?
(Assignee)

Comment 12

5 years ago
> > +  enum Type
> > +  {
> > +    eRefCountedObject,
> > +    eGCedObject,
> 
> you could have a type for marked and unmarked GCed objects, then avoid the
> bool mGCMarked.  Not a big deal.
Ah, yes.

> 
> @@ +1444,5 @@
> >  {
> >  public:
> > +    nsCycleCollectorLogger() :
> > +      mStream(nsnull), mWantAllTraces(false),
> > +      mDisableLog(false), mWantAfterProcessing(false),
> 
> These two flags should be consistent, so you don't check one for true and
> one for false all over the place.  Maybe mWantLogToFile and mWantSaveLog or
> something?
Well, I wanted to be consistent with the IDL attribute names

> 
> @@ +1649,5 @@
> >  
> > +    NS_IMETHOD ProcessNext(nsICycleCollectorHandler* aHandler,
> > +                           bool* aCanContinue)
> > +    {
> > +        NS_ENSURE_STATE(aHandler);
> 
> Do we maybe want an assert here for mWantAfterProcessing?
No. It is perfectly legal for JS to call the method whenever it wants.
But, perhaps the method should throw if !mWantAfterProcessing
I could change to NS_ENSURE_STATE(aHandler && mWantAfterProcessing);
(In reply to Olli Pettay [:smaug] from comment #12)
> > Do we maybe want an assert here for mWantAfterProcessing?
> No. It is perfectly legal for JS to call the method whenever it wants.
> But, perhaps the method should throw if !mWantAfterProcessing
> I could change to NS_ENSURE_STATE(aHandler && mWantAfterProcessing);

Ah, good point it will be JS calling it.  I think it is safe to run it without that, but people may want to know more strongly that they aren't going to get anything.  Though I guess the "JS way" is to just keep plowing ahead and ignore errors as long as you can, so maybe it is fine as is...
(Assignee)

Comment 14

5 years ago
Well, throwing (NS_ENSURE_*) gives a good hint that something is wrong :)
(In reply to Olli Pettay [:smaug] from comment #12)
> Well, I wanted to be consistent with the IDL attribute names

Sure, but you could change the IDL attribute name, too. Not a big deal, I guess.
Comment on attachment 597515 [details] [diff] [review]
wip

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

I still think it is a little gross to mash together two listeners like this, but it really isn't a huge deal, so okay.
Attachment #597515 - Flags: review?(continuation) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 598261 [details] [diff] [review]
+comments
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/mozilla-central/rev/eb85fbbeb6d9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

5 years ago
Created attachment 598347 [details]
cc analyzer
Attachment #598079 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 598349 [details]
cc analyzer
Attachment #598347 - Attachment is obsolete: true
Created attachment 598896 [details]
CCDump - better UI for CC object graph analysis

Great progress here, Olli!

I created another extension that provides more interactive UI (the graph is an expandable tree). It doesn't have all the logic yet, but already shows the object graph, roots and documents.

The source is available here:
https://github.com/janodvarko/ccdump

Honza
(Assignee)

Comment 22

5 years ago
Does(In reply to Jan Honza Odvarko from comment #21)
> Created attachment 598896 [details]
Does this end up creating/deleting dom nodes?
For an addon like about:cc it is pretty important to not create
any extra cycle collectable garbage. That would reduce the reliability.
(Assignee)

Comment 23

5 years ago
Though, I guess one could make things more reliable if after deleting some parts of a 
dom tree you run cycle collector few times.
(Assignee)

Comment 24

5 years ago
Honza, I get the following when loading about:ccdump.

Timestamp: 02/20/2012 07:49:15 PM
Error: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.import]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://ccdump/content/lib/trace.js :: <TOP_LEVEL> :: line 17"  data: no]
Source File: resource://ccdump/content/lib/trace.js
Line: 22
Created attachment 598913 [details]
CCDump 2 (one bug fixed)

One bug fixed in this version

Honza
Attachment #598896 - Attachment is obsolete: true
Created attachment 598919 [details]
CCDump 3 (yet one problem fixed)
Attachment #598913 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
Comment on attachment 598919 [details]
CCDump 3 (yet one problem fixed)

Marking this obsolete since Honza has better version in 
https://github.com/janodvarko/ccdump
Attachment #598919 - Attachment is obsolete: true
nsICycleCollectorListener.idl needs comments about the new interface and methods.
(Assignee)

Comment 29

5 years ago
Created attachment 602281 [details]
cc analyzer
(Assignee)

Comment 30

5 years ago
Comment on attachment 598349 [details]
cc analyzer

The latest version should work with about:telemetry
Attachment #598349 - Attachment is obsolete: true

Updated

5 years ago
Blocks: 728568
(Assignee)

Comment 31

2 years ago
Created attachment 8601578 [details]
+e10s support
You need to log in before you can comment on or make changes to this bug.