847641 - Remove NS_MEMORY_REPORTER_IMPLEMENT, take 2

Status: RESOLVED WONTFIX

(Core :: General, defect)

Description

[Nicholas Nethercote [inactive]]

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...

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Minor nsMemoryReportermanager.{cpp,h} code style clean-ups.

If you're having trouble sleeping, I recommend reading this page while
listening to some soothing music.

Comment 2

[Nicholas Nethercote [inactive]]

> 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.

Comment 3

[Nicholas Nethercote [inactive]]

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 4

[Justin Lebar (not reading bugmail)]

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.

Comment 5

[Justin Lebar (not reading bugmail)]

> [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...

Comment 6

[Nicholas Nethercote [inactive]]

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 7

[Nicholas Nethercote [inactive]]

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.

Comment 8

[Nicholas Nethercote [inactive]]

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.