Closed Bug 726346 Opened 12 years ago Closed 12 years ago

Implement a version of nsICycleCollectorListener for devtools

Categories

(DevTools :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 11 obsolete files)

wip
11.72 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
12.22 KB, patch
Details | Diff | Splinter Review
4.37 KB, application/x-xpinstall
Details
4.44 KB, application/x-xpinstall
Details
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?
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.
Blocks: 722750
Assignee: nobody → bugs
Attached patch wipSplinter Review
Attached file CC log printer (obsolete) —
about:cc
Attachment #597532 - Attachment is patch: false
Attachment #597532 - Attachment mime type: text/plain → application/zip
Attached file some more functionality (obsolete) —
Ugly UI, but it has most of the functionality I use from my perl script.
Attachment #597532 - Attachment is obsolete: true
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
Attached file faster graph analysis (obsolete) —
Honza, the addon requires the core patch (which is waiting for review).
Attachment #597657 - Attachment is obsolete: true
Attached file Some tweaks (obsolete) —
Attachment #597819 - Attachment is obsolete: true
Attachment #597845 - Attachment mime type: application/octet-stream → application/x-xpinstall
Attachment #597845 - Attachment is obsolete: true
Blocks: 723783
No longer blocks: 722750
Attached file Some tweaks (obsolete) —
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?
> > +  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...
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+
Attached patch +commentsSplinter Review
https://hg.mozilla.org/mozilla-central/rev/eb85fbbeb6d9
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attached file cc analyzer (obsolete) —
Attachment #598079 - Attachment is obsolete: true
Attached file cc analyzer (obsolete) —
Attachment #598347 - Attachment is obsolete: true
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
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.
Though, I guess one could make things more reliable if after deleting some parts of a 
dom tree you run cycle collector few times.
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
Attached file CCDump 2 (one bug fixed) (obsolete) —
One bug fixed in this version

Honza
Attachment #598896 - Attachment is obsolete: true
Attached file CCDump 3 (yet one problem fixed) (obsolete) —
Attachment #598913 - Attachment is obsolete: true
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.
Attached file cc analyzer
Comment on attachment 598349 [details]
cc analyzer

The latest version should work with about:telemetry
Attachment #598349 - Attachment is obsolete: true
Blocks: 728568
Attached file +e10s support
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.