new leak per url load tool

RESOLVED INCOMPLETE

Status

()

Core
XPCOM
--
enhancement
RESOLVED INCOMPLETE
17 years ago
4 years ago

People

(Reporter: Steve Adamski (gone), Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

(Reporter)

Description

17 years ago
I have a patch to source for mozilla, which generates one large file, which
contains leak information. The file consists of a header, which is the name of
the url that is loaded, followed by the leaks associated with that url.
Steve, would you care to attach a diff to this bug using
http://bugzilla.mozilla.org/createattachment.cgi?id=75796 ?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

17 years ago
Created attachment 30840 [details]
source code changes -- first prototype
(Reporter)

Comment 3

17 years ago
The first basic question, is how do we want to implement the interface to this
functionality? In other words, should we implement it as:
1) some conditional inclusion build-time macro constant
2) a pref
3) part of the debug build
4) dependent on some environment variable
5) other, or some combination (perhaps, a pref on the debug build)
(Reporter)

Comment 4

17 years ago
Concensus so far:
1) not part of debug build
2) only as part of boehm build
3) the rest (how to switch the functionality on or off) doesn't seem to matter
much to people. 

Updated

17 years ago
Blocks: 71874
(Reporter)

Comment 5

17 years ago
Created attachment 35197 [details] [diff] [review]
The real proposed patch, with correct MACROs, tabs, etc.
(Reporter)

Comment 6

17 years ago
Created attachment 35198 [details] [diff] [review]
patch: better conforms to coding style (Braces, and spaces)

Comment 7

17 years ago
You don't need to initialize an |nsCOMPtr| to |NULL|.  It does so by itself.
Cool C++-kitties prefer |0| over |NULL| or |nsnull| in any case.  Don't compare
with |NULL| or |0|, just test, e.g., prefer

  if ( foo )

over

  if ( foo != NULL )

...and as I mention above, prefer |0| (in general) or else |nsnull| (in mozilla)
over |NULL|.

Declare your variables right where you can first initialize them (where that is
compatible with the desired lifetime), so in your code you would move the
declaration of |currURI| into the |if (result)| clause, and the declaration of
|urlName| to inside the |if (currURI)| clause.

We have a general portability rule about not conditionally |#include|ing files

  http://www.mozilla.org/hacking/portable-cpp.html#dont_ifdef_include

This debugging case isn't explicitly mentioned as an exception, but I think it
should be, so you probably shouldn't be dissuaded from this if it comes up.

I think you'll want to double-check your auto-conf stuff with Chris Seawood, but
other than that, it looks good to me.  Fix the declarations and tests and you'll
have my sr.
  

Comment 8

17 years ago
Oops.  We crossed the streams :-)  My review above refers to your second patch

  http://bugzilla.mozilla.org/showattachment.cgi?attach_id=35197

and I see you have addressed some of the concerns (notably that you don't need
to init an |nsCOMPtr| to |NULL|).  The declaration and test comments still
apply, I think.
(Reporter)

Comment 9

17 years ago
Created attachment 36870 [details] [diff] [review]
patch, in reponse to scc's review
(Reporter)

Comment 10

17 years ago
Created attachment 36871 [details]
a sample of leak analysis that this feature allows
(Reporter)

Comment 11

17 years ago
Created attachment 36872 [details]
sample graph of bytes leaked per url, created from output
(Reporter)

Comment 12

17 years ago
Created attachment 36873 [details]
sample graph of number of leaks per url, created from output
Spontaneous review!

Please remove the #ifdef(linux) from all of the defines and move it into the
configure test instead, assuming that it has to be linux.  Having more than one
define for the option makes it hard to get running on other platforms and
increases the fragility and maintenance down the road.

Comment 14

17 years ago
Probably there's no need to limit it further than GC_LEAK_DETECTOR, since people
will use GC_LEAK_DETECTOR where it works and won't where it doesn't work
(because it won't work :-).  I would think this code ought to work on Mac, right?
Assignee: thesteve → nobody
QA Contact: scc → xpcom

Comment 15

12 years ago
Shouldn't bugs like this one have patch and perf keywords?

Comment 16

6 years ago
I believe that we have an equivalent of this now, and it is no longer useful code.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.