Last Comment Bug 726346 - Implement a version of nsICycleCollectorListener for devtools
: Implement a version of nsICycleCollectorListener for devtools
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on:
Blocks: 723783 728568
  Show dependency treegraph
 
Reported: 2012-02-11 12:36 PST by Olli Pettay [:smaug]
Modified: 2015-05-05 09:39 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (11.72 KB, patch)
2012-02-15 12:18 PST, Olli Pettay [:smaug]
continuation: review+
Details | Diff | Review
CC log printer (2.61 KB, application/zip)
2012-02-15 12:55 PST, Olli Pettay [:smaug]
no flags Details
some more functionality (3.85 KB, application/octet-stream)
2012-02-15 19:05 PST, Olli Pettay [:smaug]
no flags Details
faster graph analysis (4.00 KB, application/octet-stream)
2012-02-16 08:04 PST, Olli Pettay [:smaug]
no flags Details
Some tweaks (4.00 KB, application/x-xpinstall)
2012-02-16 08:57 PST, Olli Pettay [:smaug]
no flags Details
don't hang so easily with *huge* graphs (4.12 KB, application/x-xpinstall)
2012-02-16 09:39 PST, Olli Pettay [:smaug]
no flags Details
Some tweaks (4.24 KB, application/x-xpinstall)
2012-02-16 17:46 PST, Olli Pettay [:smaug]
no flags Details
+comments (12.22 KB, patch)
2012-02-17 09:31 PST, Olli Pettay [:smaug]
no flags Details | Diff | Review
cc analyzer (4.30 KB, application/x-xpinstall)
2012-02-17 13:12 PST, Olli Pettay [:smaug]
no flags Details
cc analyzer (4.35 KB, application/x-xpinstall)
2012-02-17 13:20 PST, Olli Pettay [:smaug]
no flags Details
CCDump - better UI for CC object graph analysis (49.36 KB, application/x-xpinstall)
2012-02-20 09:30 PST, Jan Honza Odvarko [:Honza]
no flags Details
CCDump 2 (one bug fixed) (48.41 KB, application/x-xpinstall)
2012-02-20 10:14 PST, Jan Honza Odvarko [:Honza]
no flags Details
CCDump 3 (yet one problem fixed) (49.52 KB, application/x-xpinstall)
2012-02-20 10:35 PST, Jan Honza Odvarko [:Honza]
no flags Details
cc analyzer (4.37 KB, application/x-xpinstall)
2012-03-02 00:51 PST, Olli Pettay [:smaug]
no flags Details
+e10s support (4.44 KB, application/x-xpinstall)
2015-05-05 09:39 PDT, Olli Pettay [:smaug]
no flags Details

Description Olli Pettay [:smaug] 2012-02-11 12:36:21 PST
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.
Comment 1 Rob Campbell [:rc] (:robcee) 2012-02-13 16:42:59 PST
nice idea!

What are you thinking for a display? Maybe an event in the web console?
Comment 2 Olli Pettay [:smaug] 2012-02-13 16:53:17 PST
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.
Comment 3 Olli Pettay [:smaug] 2012-02-15 12:18:23 PST
Created attachment 597515 [details] [diff] [review]
wip
Comment 4 Olli Pettay [:smaug] 2012-02-15 12:55:08 PST
Created attachment 597532 [details]
CC log printer

about:cc
Comment 5 Olli Pettay [:smaug] 2012-02-15 19:05:05 PST
Created attachment 597657 [details]
some more functionality

Ugly UI, but it has most of the functionality I use from my perl script.
Comment 6 Jan Honza Odvarko [:Honza] 2012-02-16 01:18:26 PST
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
Comment 7 Olli Pettay [:smaug] 2012-02-16 08:04:30 PST
Created attachment 597819 [details]
faster graph analysis

Honza, the addon requires the core patch (which is waiting for review).
Comment 8 Olli Pettay [:smaug] 2012-02-16 08:57:52 PST
Created attachment 597845 [details]
Some tweaks
Comment 9 Olli Pettay [:smaug] 2012-02-16 09:39:10 PST
Created attachment 597868 [details]
don't hang so easily with *huge* graphs
Comment 10 Olli Pettay [:smaug] 2012-02-16 17:46:28 PST
Created attachment 598079 [details]
Some tweaks
Comment 11 Andrew McCreight [:mccr8] 2012-02-16 18:38:10 PST
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?
Comment 12 Olli Pettay [:smaug] 2012-02-16 18:47:03 PST
> > +  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);
Comment 13 Andrew McCreight [:mccr8] 2012-02-16 18:52:10 PST
(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...
Comment 14 Olli Pettay [:smaug] 2012-02-17 05:43:55 PST
Well, throwing (NS_ENSURE_*) gives a good hint that something is wrong :)
Comment 15 Andrew McCreight [:mccr8] 2012-02-17 09:06:16 PST
(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 16 Andrew McCreight [:mccr8] 2012-02-17 09:08:07 PST
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.
Comment 17 Olli Pettay [:smaug] 2012-02-17 09:31:29 PST
Created attachment 598261 [details] [diff] [review]
+comments
Comment 18 Olli Pettay [:smaug] 2012-02-17 10:17:55 PST
https://hg.mozilla.org/mozilla-central/rev/eb85fbbeb6d9
Comment 19 Olli Pettay [:smaug] 2012-02-17 13:12:41 PST
Created attachment 598347 [details]
cc analyzer
Comment 20 Olli Pettay [:smaug] 2012-02-17 13:20:55 PST
Created attachment 598349 [details]
cc analyzer
Comment 21 Jan Honza Odvarko [:Honza] 2012-02-20 09:30:45 PST
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
Comment 22 Olli Pettay [:smaug] 2012-02-20 09:37:09 PST
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.
Comment 23 Olli Pettay [:smaug] 2012-02-20 09:40:03 PST
Though, I guess one could make things more reliable if after deleting some parts of a 
dom tree you run cycle collector few times.
Comment 24 Olli Pettay [:smaug] 2012-02-20 09:50:12 PST
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
Comment 25 Jan Honza Odvarko [:Honza] 2012-02-20 10:14:24 PST
Created attachment 598913 [details]
CCDump 2 (one bug fixed)

One bug fixed in this version

Honza
Comment 26 Jan Honza Odvarko [:Honza] 2012-02-20 10:35:00 PST
Created attachment 598919 [details]
CCDump 3 (yet one problem fixed)
Comment 27 Olli Pettay [:smaug] 2012-02-21 09:13:55 PST
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
Comment 28 Peter Van der Beken [:peterv] 2012-02-22 06:28:16 PST
nsICycleCollectorListener.idl needs comments about the new interface and methods.
Comment 29 Olli Pettay [:smaug] 2012-03-02 00:51:13 PST
Created attachment 602281 [details]
cc analyzer
Comment 30 Olli Pettay [:smaug] 2012-03-02 00:51:50 PST
Comment on attachment 598349 [details]
cc analyzer

The latest version should work with about:telemetry
Comment 31 Olli Pettay [:smaug] 2015-05-05 09:39:05 PDT
Created attachment 8601578 [details]
+e10s support

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