Closed
Bug 947798
Opened 10 years ago
Closed 10 years ago
Remove MemoryMultiReporter
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(2 files)
14.93 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
39.30 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
It's just a better name, and more consistent with other such macros in the codebase.
Attachment #8344462 -
Flags: review?(continuation)
Assignee | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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+
Comment 4•10 years ago
|
||
Why are you making everything threadsafe?
Updated•10 years ago
|
Flags: needinfo?(n.nethercote)
Assignee | ||
Comment 5•10 years ago
|
||
> 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 6•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/294b27f4a5f5 https://hg.mozilla.org/integration/mozilla-inbound/rev/4364781968e8
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/294b27f4a5f5 https://hg.mozilla.org/mozilla-central/rev/4364781968e8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•