Closed Bug 847641 Opened 7 years ago Closed 7 years ago

Remove NS_MEMORY_REPORTER_IMPLEMENT, take 2

Categories

(Core :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(1 obsolete file)

I wrote 20 patches to remove NS_MEMORY_REPORTER_IMPLEMENT in bug 831193, but jlebar discovered a flaw in my design relating to ref counting.  Let me try again...
No longer depends on: 831193
If you're having trouble sleeping, I recommend reading this page while
listening to some soothing music.
Attachment #720934 - Flags: review?(justin.lebar+bug)
> If you're having trouble sleeping, I recommend reading this page while
> listening to some soothing music.

And if you're confused by that comment, I recommend substituting "patch" for "page" while reading it.
jlebar, here's a description of my plan of attack.

There will be two ways to register a reporter (or multi-reporter).

"Permanent" registration is used for any reporter that doesn't relate to a particular object or structure, e.g. the "resident" reporter.  For such reporters, the nsMemoryReporterManager will hold a strong reference;  they should never be unregistered;  and they will be freed when the nsMemoryReporterManager is destroyed at shutdown.

"Temporary" registration is used for any reporter that relates to a particular object or structure, e.g. "xpcom/category-manager" (which measures the nsCategoryManager).  Such a structure will be implemented as a sub-class of MemoryReporterBase, and will need to register itself when it is created/inited.
nsMemoryReporterManager will hold a *weak* reference to it.  The object/structure must unregister itself in its own destructor.

Any reporter enumerator object will (unsurprisingly) hold strong references to all the registered reporters.

I'm pretty sure this will be thread-safe, e.g. in cases like the one you described in bug 831193 comment 41.
Comment on attachment 720934 [details] [diff] [review]
(part 1) - Minor nsMemoryReportermanager.{cpp,h} code style clean-ups.

r=me, but I really hope we don't have any changes we need to make to these files on b2g18, otherwise we're going to have a bad time merging them.
Attachment #720934 - Flags: review?(justin.lebar+bug) → review+
> [permanent reporters] should never be unregistered

...except during our tests, I guess?

> Any reporter enumerator object will (unsurprisingly) hold strong references to all the 
> registered reporters.

We'll have to make it clear in our APIs that you must never hold memory reporter objects or iterators alive longer than absolutely necessary.  That's a footgun I hadn't thought about before...
Ugh, I'm now wondering if the current idiom of separate reporters that measure
singleton objects via static means (e.g. static variables, static methods) is
better.  The irony is that NS_MEMORY_REPORTER_IMPLEMENT encourages that style,
because the method it takes cannot be passed any arguments.
Comment on attachment 720934 [details] [diff] [review]
(part 1) - Minor nsMemoryReportermanager.{cpp,h} code style clean-ups.

I landed this part 1 patch in bug 905465.
Attachment #720934 - Attachment is obsolete: true
NS_MEMORY_REPORTER_IMPLEMENT was removed elsewhere.  The temporary vs. permanent distinction is still promising, but if I do that I will do it in another bug.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.