Closed Bug 947798 Opened 6 years ago Closed 6 years ago

Remove MemoryMultiReporter

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(2 files)

I added MemoryMultiReporter recently (bug 934321) but now that the |name| property is gone from nsIMemoryReporter, it's actually more concise to use nsIMemoryReporter directly instead of MemoryMultiReporter.  Using nsIMemoryReporter also avoids the use of NS_IMPL_ISUPPORTS_INHERITED<n>, which is nice.
It's just a better name, and more consistent with other such macros in the
codebase.
Attachment #8344462 - Flags: review?(continuation)
This is a nice simplification:

 26 files changed, 98 insertions(+), 129 deletions(-)

And if you exclude the removal of MemoryMultiReporter's declarations, it's
still a net reduction of 1 LOC!  So much for avoiding boilerplate.

Here's the recipe I used for each changed class.

- Inherit from nsIMemoryReporter instead of MemoryMultiReporter.

- Possibly change NS_DECL_ISUPPORTS to NS_DECL_THREADSAFE_ISUPPORTS (just being
  extra cautious).

- If CollectReports() isn't defined inline, add NS_DECL_NSIMEMORYREPORTER to
  the class decl, and remove CollectReports() from the class decl.

- Delete the constructor if it's a no-op.

- Replace NS_IMPL_ISUPPORTS_INHERITED<n> with NS_IMPL_ISUPPORTS<n+1>, or add
  NS_IMPL_ISUPPORTS1 if necessary.

- If the reporter uses |MallocSizeOf|, add a MOZ_DEFINE_MALLOC_SIZE_OF as
  necessary.
Attachment #8344465 - Flags: review?(continuation)
Comment on attachment 8344462 [details] [diff] [review]
(part 1) - Rename NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN as MOZ_DEFINE_MALLOC_SIZE_OF.

Review of attachment 8344462 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/Image.cpp
@@ +28,5 @@
>    if (mError)
>      return 0;
>  
>    // This is not used by memory reporters, but for sizing the cache, which is
>    // why it uses |moz_malloc_size_of| rather than an

super nit: an -> a. Or just remove "an".
Attachment #8344462 - Flags: review?(continuation) → review+
Why are you making everything threadsafe?
Flags: needinfo?(n.nethercote)
> Why are you making everything threadsafe?

A small number of reporters need to be threadsafe.  So MemoryUniReporter and MemoryMultiReporter are threadsafe -- i.e. have NS_DECL_THREADSAFE_ISUPPORTS.  I can't remember now which multi-reporters needed thread safety, and I'm lazy, and reporter addref/releases are uncommon, so I'm just playing it safe.

I could do the required archaeology to work out which ones need it and which don't, if you think it's important.
Flags: needinfo?(n.nethercote)
Comment on attachment 8344465 [details] [diff] [review]
(part 2) - Remove MemoryMultiReporter, because it's no longer helpful.

Review of attachment 8344465 [details] [diff] [review]:
-----------------------------------------------------------------

> I could do the required archaeology to work out which ones need it and which don't, if you think it's important.

I guess it isn't a huge deal, it is just confusing for anybody reading the classes.  I noted a few instances of things I think aren't multithreaded that you might as well just flip back.  If a header doesn't contain THREADSAFE anywhere else, it might be ok.  I'd assume thread errors would show up in a debug try run, so a few mistakes won't be too costly.

::: dom/workers/WorkerPrivate.cpp
@@ +2012,2 @@
>  {
> +  NS_DECL_THREADSAFE_ISUPPORTS

shouldn't need to be threadsafe

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2398,3 @@
>  {
>    public:
> +    NS_DECL_THREADSAFE_ISUPPORTS

This shouldn't need to be threadsafe.

::: modules/libpref/src/Preferences.cpp
@@ +227,3 @@
>  {
>  public:
> +  NS_DECL_THREADSAFE_ISUPPORTS

Preference service is mainthread only, so this shouldn't need to be threadsafe.

::: xpcom/base/nsCycleCollector.cpp
@@ +1075,2 @@
>  {
> +    NS_DECL_THREADSAFE_ISUPPORTS

This doesn't need to be threadsafe.
Attachment #8344465 - Flags: review?(continuation) → review+
https://hg.mozilla.org/mozilla-central/rev/294b27f4a5f5
https://hg.mozilla.org/mozilla-central/rev/4364781968e8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.