Closed
Bug 831193
Opened 11 years ago
Closed 11 years ago
Remove NS_MEMORY_REPORTER_IMPLEMENT
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(20 files, 23 obsolete files)
25.99 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
4.75 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
7.70 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
3.77 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
3.58 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
12.20 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
10.05 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
5.71 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
11.81 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
8.22 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
6.20 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
3.17 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
13.09 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
4.53 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
7.48 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
28.85 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
4.70 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
4.44 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
6.45 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
3.80 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
Bug 829439 added the new mozilla::MemoryReporterBase type, which supersedes NS_MEMORY_REPORTER_IMPLEMENT. This bug is about removing the vestiges of it.
Assignee | ||
Comment 1•11 years ago
|
||
This patch removes the uses of NS_MEMORY_REPORTER_IMPLEMENT in nsMemoryReporterManager.cpp. jlebar, the formatting style I've used for the descriptions is a little unconventional, but I wanted to get something close to the density of the macro. Hopefully you don't mind it.
Attachment #702718 -
Flags: review?(justin.lebar+bug)
Comment 2•11 years ago
|
||
Comment on attachment 702718 [details] [diff] [review] (part 1) - Remove uses of NS_MEMORY_REPORTER_IMPLEMENT from nsMemoryReporterManager.cpp. >-NS_FALLIBLE_MEMORY_REPORTER_IMPLEMENT(Private, >- "private", >- KIND_OTHER, >- UNITS_BYTES, >- GetPrivate, >- "Memory that cannot be shared with other processes, including memory that " >- "is committed and marked MEM_PRIVATE, data that is not mapped, and " >- "executable pages that have been written to.") >+class PrivateReporter MOZ_FINAL : public MemoryReporterBase >+{ >+public: >+ PrivateReporter() >+ : MemoryReporterBase("private", KIND_OTHER, UNITS_BYTES, >+"Memory that cannot be shared with other processes, including memory that is " >+"committed and marked MEM_PRIVATE, data that is not mapped, and executable " >+"pages that have been written to.") >+ {} >+private: >+ NS_IMETHOD GetAmount(int64_t *aAmount) { return GetPrivate(aAmount); } >+ int64_t Amount() { MOZ_ASSERT(false); return 0; } >+}; What would you think about improving MemoryReporterBase so it has a better interface for writing fallible memory reporters? One simple option would be to add a default implementation of Amount(), which MOZ_ASSERTs as you have here. Another option would be to add FallibleMemoryReporterBase, which implements Amount() with a MOZ_ASSERT() and exposes a pure virtual GetFallibleAmount (or something) method. >+class PageFaultsHardReporter MOZ_FINAL : public MemoryReporterBase >+{ >+public: >+ PageFaultsHardReporter() >+ : MemoryReporterBase("page-faults-hard", KIND_OTHER, >+ UNITS_COUNT_CUMULATIVE, >+"The number of hard page faults (also known as 'major page faults') that have " >+"occurred since the process started. A hard page fault occurs when a process " >+"tries to access a page which is not present in physical memory. The " >+"operating system must access the disk in order to fulfill a hard page fault. " >+"When memory is plentiful, you should see very few hard page faults. But if " >+"the process tries to use more memory than your machine has available, you " >+"may see many thousands of hard page faults. Because accessing the disk is up " >+"to a million times slower than accessing RAM, the program may run very " >+"slowly when it is experiencing more than 100 or so hard page faults a second.") >+ {} >+private: >+ NS_IMETHOD GetAmount(int64_t *aAmount) { return GetPageFaultsHard(aAmount); } >+ int64_t Amount() { MOZ_ASSERT(false); return 0; } >+}; What would you think about inlining GetPageFaultsHard() into GetAmount() here? And similarly for the jemalloc and atoms table reporters. Being able to inline the Amount() function is the main advantage I see to moving away from these macros.
Assignee | ||
Comment 3•11 years ago
|
||
> What would you think about improving MemoryReporterBase so it has a better > interface for writing fallible memory reporters? > > One simple option would be to add a default implementation of Amount(), which > MOZ_ASSERTs as you have here. Good idea. I'll do that. > What would you think about inlining GetPageFaultsHard() into GetAmount() here? > And similarly for the jemalloc and atoms table reporters. Sure.
Assignee | ||
Comment 4•11 years ago
|
||
I was going to attach a patch that applied on top of the previous one, but then I typed |hg qref| instead of |hg qnew|. Bah. Anyway, as suggested: - I changed it so you only have to provide one of GetAmount() and Amount(). - I inlined: GetPageFaults{Hard,Soft}, GetPrivate, GetHeap*, GetExplicit, GetAtomTableSize. Also: - I fixed some |#if| conditions to |#ifdef| -- the behaviour is unchanged because the values are #defined to 1, but it's now consistent. - I renamed AtomTableReporter to AtomsTableReporter to match its path.
Attachment #703661 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Updated•11 years ago
|
Attachment #702718 -
Attachment is obsolete: true
Attachment #702718 -
Flags: review?(justin.lebar+bug)
Comment 5•11 years ago
|
||
Comment on attachment 703661 [details] [diff] [review] (part 1) - Remove uses of NS_MEMORY_REPORTER_IMPLEMENT from nsMemoryReporterManager.cpp. >diff --git a/xpcom/base/nsIMemoryReporter.idl b/xpcom/base/nsIMemoryReporter.idl >--- a/xpcom/base/nsIMemoryReporter.idl >+++ b/xpcom/base/nsIMemoryReporter.idl >@@ -425,17 +425,20 @@ void RunReporters(); > } > > namespace mozilla { > > // The following base class reduces the amount of boilerplate code required for > // memory reporters. You just need to provide the following. > // - The constant values: path, kind, units, and description. They are passed > // to the MemoryReporterBase constructor. >-// - An Amount() method. It can use the MallocSizeOf method if necessary. >+// - An Amount() or GetAmount() method. It can use the MallocSizeOf method if >+// necessary. Use Amount() if the reporter is infallible, and GetAmount() >+// otherwise. (You'll get assertion failures if you fail to provide one or >+// the other.) Nit: assertion failures /when the memory reporter is run/ >@@ -484,17 +487,22 @@ public: > > NS_IMETHOD GetDescription(nsACString& aDescription) > { > aDescription.Assign(mDescription); > return NS_OK; > } > > protected: >- virtual int64_t Amount() = 0; >+ virtual int64_t Amount() >+ { >+ // This fails if neither GetAmount() nor Amount() were overridden. neither was >diff --git a/xpcom/base/nsMemoryReporterManager.cpp b/xpcom/base/nsMemoryReporterManager.cpp >--- a/xpcom/base/nsMemoryReporterManager.cpp >+++ b/xpcom/base/nsMemoryReporterManager.cpp Wow, diff really did a number on this part.
Attachment #703661 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 6•11 years ago
|
||
part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/2d791defc804
Whiteboard: [leave open]
Comment 7•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2d791defc804
Assignee | ||
Comment 8•11 years ago
|
||
MemoryReporterBase is the new, nicer way of doing things.
Attachment #704366 -
Flags: review?(michal.novotny)
Assignee | ||
Comment 9•11 years ago
|
||
This is the new, improved style.
Attachment #704367 -
Flags: review?(terrence)
Assignee | ||
Comment 10•11 years ago
|
||
This is the new, improved style.
Attachment #704369 -
Flags: review?(benjamin)
Assignee | ||
Comment 11•11 years ago
|
||
This is the new, improved style.
Attachment #704370 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 12•11 years ago
|
||
Marco, this patch is tiny, so I figure you'll be ok to review it.
Attachment #704372 -
Flags: review?(mak77)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #704373 -
Flags: review?(nfroyd)
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #704374 -
Flags: review?(taras.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #704374 -
Attachment description: (part 7) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in StartupCache.cpp. → (part 8) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in StartupCache.cpp.
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #704379 -
Flags: review?(nfroyd)
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #704383 -
Flags: review?(bjacob)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #704384 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 18•11 years ago
|
||
Attachment #704385 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #704391 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #704394 -
Flags: review?(dbaron)
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #704396 -
Flags: review?(ehsan)
Assignee | ||
Comment 22•11 years ago
|
||
As well as removing the uses of NS_MEMORY_REPORTER_IMPLEMENT, this patch does the following. - Converts the two reporters into a single multi-reporter, which is a bit nicer. - Renames MediaMemoryReporter as MediaMemoryTracker, which better reflects its function.
Attachment #704397 -
Flags: review?(cpearce)
Assignee | ||
Comment 23•11 years ago
|
||
We don't need NumLowMemoryEventsReporter as a base class any more. A question... the paths for the three fields are: - "low-memory-events/virtual" - "low-memory-events/physical" - "low-commit-space-events" Should the last one be changed to "low-memory-events/commit-space"?
Attachment #704400 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 24•11 years ago
|
||
Ah, finally.
Attachment #704407 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 25•11 years ago
|
||
Attachment #704408 -
Flags: review?(gpascutto)
Assignee | ||
Comment 26•11 years ago
|
||
Attachment #704410 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 27•11 years ago
|
||
Fix a silly compile error.
Attachment #704414 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Updated•11 years ago
|
Attachment #704400 -
Attachment is obsolete: true
Attachment #704400 -
Flags: review?(justin.lebar+bug)
Comment 28•11 years ago
|
||
Comment on attachment 704397 [details] [diff] [review] (part 16) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in media/. Review of attachment 704397 [details] [diff] [review]: ----------------------------------------------------------------- Matthew wrote the original memory reporters, so I think he's a better reviewer.
Attachment #704397 -
Flags: review?(cpearce) → review?(kinetik)
Comment on attachment 704394 [details] [diff] [review] (part 14) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in layout/. I don't think review from a layout peer is needed for this sort of thing; you want review from somebody know knows the memory reporting infrastructure instead. Thus punting review to jlebar since he reviewed bug 829439, but feel free to punt further.
Attachment #704394 -
Flags: review?(dbaron) → review?(justin.lebar+bug)
Updated•11 years ago
|
Attachment #704369 -
Flags: review?(benjamin) → review?(jones.chris.g)
Comment 30•11 years ago
|
||
Comment on attachment 704373 [details] [diff] [review] (part 7) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in toolkit/components/. Review of attachment 704373 [details] [diff] [review]: ----------------------------------------------------------------- I like the new style! ::: toolkit/components/telemetry/Telemetry.cpp @@ +337,5 @@ > + n += mHistogramMap.SizeOfExcludingThis(nullptr, aMallocSizeOf); > + n += mPrivateSQL .SizeOfExcludingThis(nullptr, aMallocSizeOf); > + n += mSanitizedSQL.SizeOfExcludingThis(nullptr, aMallocSizeOf); > + n += mTrackedDBs .SizeOfExcludingThis(nullptr, aMallocSizeOf); > + n += mHangReports .SizeOfExcludingThis(); Please don't align these.
Attachment #704373 -
Flags: review?(nfroyd) → review+
Comment 31•11 years ago
|
||
Comment on attachment 704379 [details] [diff] [review] (part 9) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in Preferences.cpp. Review of attachment 704379 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libpref/src/Preferences.cpp @@ +217,5 @@ > class AddPreferencesMemoryReporterRunnable : public nsRunnable > { > NS_IMETHOD Run() > { > + return NS_RegisterMemoryReporter(new PreferencesReporter); I thought we had decided once upon a time that a ref to the memory reporter should be held over the call registering the reporter. Is this no longer the case? I still see a mix of both styles in existing code and the patch series for this bug.
Attachment #704379 -
Flags: review?(nfroyd) → review+
Updated•11 years ago
|
Attachment #704374 -
Flags: review?(taras.mozilla) → review+
Updated•11 years ago
|
Attachment #704396 -
Flags: review?(ehsan) → review+
Updated•11 years ago
|
Attachment #704367 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 32•11 years ago
|
||
> I don't think review from a layout peer is needed for this sort of thing;
> you want review from somebody know knows the memory reporting infrastructure
> instead.
I asked you because you reviewed the original memory reporter patches. But I'm fine with jlebar reviewing if you are.
Assignee | ||
Comment 33•11 years ago
|
||
> I thought we had decided once upon a time that a ref to the memory reporter > should be held over the call registering the reporter. Is this no longer > the case? I still see a mix of both styles in existing code and the patch > series for this bug. Bug 831193 will fix this situation fully. In the meantime, holding a ref isn't necessary and most of the existing code doesn't do it (excluding the cases where an nsCOMPtr is held by a parent object) so I'm going for consistency.
Updated•11 years ago
|
Attachment #704385 -
Flags: review?(jmuizelaar) → review+
Updated•11 years ago
|
Attachment #704391 -
Flags: review?(jmuizelaar) → review+
Updated•11 years ago
|
Attachment #704397 -
Flags: review?(kinetik) → review+
Comment 34•11 years ago
|
||
Comment on attachment 704383 [details] [diff] [review] (part 10) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in parts of gfx/. Review of attachment 704383 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContext.cpp @@ +104,2 @@ > } else { > + sAmount += bytes; Two questions here. I realize that these are questions about the existing code rather than your patch, so I'm not r-'ing for that, but it looks like the original patch could have used some r-'d. git blame says it's from bug 818060. 1. I thought that about:memory's approach to memory accounting was explicitly to re-count everything everytime, and not to use accumulating counters, as you explain on https://wiki.mozilla.org/Memory_Reporting But the counters here are violating this good principle. 2. sAmount is static, and OpenGL work is done on more than one thread (for now on Android, in the near future on other platforms). So shouldn't these operations be made atomic? ::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp @@ +191,5 @@ > > GrallocBufferActor::~GrallocBufferActor() > { > if (mAllocBytes > 0) { > + GrallocReporter::sAmount -= mAllocBytes; Same remarks about this counter.
Attachment #704383 -
Flags: review?(bjacob) → review+
Comment 35•11 years ago
|
||
Filed bug 833185 to continue that conversation.
Assignee | ||
Comment 36•11 years ago
|
||
I forgot to convert a couple of mReporter fields from raw pointers to nsCOMPtr.
Attachment #704771 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Updated•11 years ago
|
Attachment #704414 -
Attachment is obsolete: true
Attachment #704414 -
Flags: review?(justin.lebar+bug)
Comment 37•11 years ago
|
||
Comment on attachment 704408 [details] [diff] [review] (part 19) - Make nsPrefixSetReporter a subclass of MemoryReporterBase. Review of attachment 704408 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp @@ +33,5 @@ > #define LOG(args) > #define LOG_ENABLED() (false) > #endif > > +class PrefixSetReporter MOZ_FINAL : public MemoryReporterBase Given that the old code used THREADSAFE_ISUPPORTS1, shouldn't this be ThreadSafeMemoryReporterBase? @@ +68,5 @@ > > NS_IMETHODIMP > nsUrlClassifierPrefixSet::Init(const nsACString& aName) > { > + mReporter = new PrefixSetReporter(this, aName); Any reasoning for dropping the ns Prefix? It's inconsistent with the rest of this source file.
Attachment #704408 -
Flags: review?(gpascutto) → review+
Assignee | ||
Comment 38•11 years ago
|
||
> > +class PrefixSetReporter MOZ_FINAL : public MemoryReporterBase > > Given that the old code used THREADSAFE_ISUPPORTS1, shouldn't this be > ThreadSafeMemoryReporterBase? Good question! MemoryReporterBase is now threadsafe by default; registration isn't performance critical and it's safer. > Any reasoning for dropping the ns Prefix? It's inconsistent with the rest of > this source file. It's annoying, and the class isn't visible outside the file.
Comment 39•11 years ago
|
||
Comment on attachment 704372 [details] [diff] [review] (part 6) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in mozStorageService.cpp. Review of attachment 704372 [details] [diff] [review]: ----------------------------------------------------------------- ::: storage/src/mozStorageService.cpp @@ +1,2 @@ > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ this change clashes with http://mxr.mozilla.org/mozilla-central/source/storage/style.txt#14 and I suppose it's a bit late to change style and have all the module differ from it. please revert for now. @@ +53,5 @@ > +{ > +public: > + StorageSQLiteReporter() > + : MemoryReporterBase("storage-sqlite", KIND_OTHER, UNITS_BYTES, > +"Memory used by SQLite.") the indentation here is wrong, please align arguments @@ +56,5 @@ > + : MemoryReporterBase("storage-sqlite", KIND_OTHER, UNITS_BYTES, > +"Memory used by SQLite.") > + {} > +private: > + int64_t Amount() { return ::sqlite3_memory_used(); } please annotate with MOZ_OVERRIDE
Attachment #704372 -
Flags: review?(mak77) → review+
Updated•11 years ago
|
Attachment #704366 -
Flags: review?(michal.novotny) → review+
Comment 40•11 years ago
|
||
Comment on attachment 704384 [details] [diff] [review] (part 11) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in gfxAndroidPlatform.cpp. >+class FreetypeReporter MOZ_FINAL : public MemoryReporterBase >+{ >+public: >+ FreetypeReporter() >+ : MemoryReporterBase("explicit/freetype", KIND_HEAP, UNITS_BYTES, >+ "Memory used by Freetype.") >+ {} > >+ static void* CountingAlloc(FT_Memory, long size) > { > void *p = malloc(size); >+ sAmount += MallocSizeOfOnAlloc(p); > return p; > } Is this a diff -w, so the indentation is screwy? >+private: >+ NS_MEMORY_REPORTER_MALLOC_SIZEOF_ON_ALLOC_FUN(MallocSizeOfOnAlloc) >+ NS_MEMORY_REPORTER_MALLOC_SIZEOF_ON_FREE_FUN(MallocSizeOfOnFree) I'd prefer if these lived in MemoryReporterBase. >+ int64_t Amount() { return sAmount; } >+ >+ static int64_t sAmount; Using a static variable as a member variable of a non-singleton class is pretty scary. It also sets a bad example for others to cargo-cult off. Could you do something to ensure that this class is actually a singleton (even if that's just ensuring that the constructor never runs twice)? Anoter alternative would be to stick the reporter in a ClearOnShutdown'ed StaticRefPtr, add a factory method, and make the constructor private.
Attachment #704384 -
Flags: review?(justin.lebar+bug) → review-
Comment 41•11 years ago
|
||
Comment on attachment 704394 [details] [diff] [review] (part 14) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in layout/. >diff --git a/layout/base/nsStyleSheetService.cpp b/layout/base/nsStyleSheetService.cpp >--- a/layout/base/nsStyleSheetService.cpp >+++ b/layout/base/nsStyleSheetService.cpp >@@ -16,52 +17,50 @@ > #include "nsIServiceManager.h" > #include "nsICategoryManager.h" > #include "nsISupportsPrimitives.h" > #include "nsNetUtil.h" > #include "nsIObserverService.h" > #include "nsLayoutStatics.h" > #include "nsIMemoryReporter.h" > >-NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(LayoutStyleSheetServiceMallocSizeOf) >- >-static int64_t >-GetStyleSheetServiceSize() >+class LayoutStyleSheetServiceReporter MOZ_FINAL >+ : public mozilla::MemoryReporterBase > { >- return nsStyleSheetService::SizeOfIncludingThis( >- LayoutStyleSheetServiceMallocSizeOf); >-} >- >-NS_MEMORY_REPORTER_IMPLEMENT(StyleSheetService, >- "explicit/layout/style-sheet-service", >- KIND_HEAP, >- nsIMemoryReporter::UNITS_BYTES, >- GetStyleSheetServiceSize, >+public: >+ LayoutStyleSheetServiceReporter(nsStyleSheetService* aService) >+ : MemoryReporterBase("explicit/layout/style-sheet-service", >+ KIND_HEAP, UNITS_BYTES, > "Memory used for style sheets held by the style sheet service.") >+ , mService(aService) >+ {} >+private: >+ int64_t Amount() { return mService->SizeOfIncludingThis( MallocSizeOf); } "( " > nsStyleSheetService::nsStyleSheetService() > { > PR_STATIC_ASSERT(0 == AGENT_SHEET && 1 == USER_SHEET && 2 == AUTHOR_SHEET); > NS_ASSERTION(!gInstance, "Someone is using CreateInstance instead of GetService"); > gInstance = this; > nsLayoutStatics::AddRef(); > >- mReporter = new NS_MEMORY_REPORTER_NAME(StyleSheetService); >+ mReporter = new LayoutStyleSheetServiceReporter(this); >- (void)::NS_RegisterMemoryReporter(mReporter); >+ NS_RegisterMemoryReporter(mReporter); I suspect the (void) was put in to avoid compiler warnings, and the :: was put in out of someone's understanding of layout style. Since I'm not a layout peer, I'm not comfortable approving of a reversion of that style. >- (void)::NS_UnregisterMemoryReporter(mReporter); >- mReporter = nullptr; >+ NS_UnregisterMemoryReporter(mReporter); ibid. >@@ -221,24 +210,23 @@ nsLayoutStylesheetCache::nsLayoutStylesh >- mReporter = new NS_MEMORY_REPORTER_NAME(StyleSheetCache); >- (void)::NS_RegisterMemoryReporter(mReporter); >+ mReporter = new LayoutStyleSheetCacheReporter(this); >+ NS_RegisterMemoryReporter(mReporter); > } > > nsLayoutStylesheetCache::~nsLayoutStylesheetCache() > { >- (void)::NS_UnregisterMemoryReporter(mReporter); >- mReporter = nullptr; >+ NS_UnregisterMemoryReporter(mReporter); > } ibid. OT, but this idiom we've developed here of having classes with a nsCOMPtr<nsIMemoryReporter> member which they register on construction and unregister on destruction is kind of bad OOP. ISTM that a better way of doing it would be to inherit from a special class. E.g. class Foo : public MemoryReporterMixin<Foo> { Foo() : MemoryReporterMixin<Foo>(KIND_HEAP, ...) {} size_t SizeOfIncludingThis(...) const {...}; }; class MemoryReporterMixin<T> { int64_t Amount() { return T::SizeOfIncludingThis(...); } }; Then the MemoryReporterMixin constructor would register the memory reporter, and the MemoryReporterMixin destructor would unregister it. The main trick would be refcounting. The memory reporter manager couldn't keep a strong ref to Foo, because then Foo would never be destructed. Maybe a weak reference would do. Actually, now that I think of it, this is not entirely off-topic. Suppose we have the following situation. 1. On the main thread, we take a reference to a Foo memory reporter. (For example, we might be running telemetry, which holds refs to nsIMemoryReporter objects via JS.) Our memory reporter has a member |Foo* mFoo|. When we run the memory reporter, we do mFoo->SizeOfIncludingThis(). 2. Object Foo is destroyed off the main thread. This does NS_UnregisterMemoryReporter(Foo), but the memory reporter is not deleted, because JS has a ref to it! 3. We do GetAmount() on the memory reporter. So this idiom is not thread-safe. That probably invalidates some of your patches here. :-/
Attachment #704394 -
Flags: review?(justin.lebar+bug) → review+
Updated•11 years ago
|
Attachment #704407 -
Flags: review?(justin.lebar+bug) → review+
Updated•11 years ago
|
Attachment #704410 -
Flags: review?(justin.lebar+bug) → review+
Updated•11 years ago
|
Attachment #704771 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 42•11 years ago
|
||
> this change clashes with > http://mxr.mozilla.org/mozilla-central/source/storage/style.txt#14 > and I suppose it's a bit late to change style and have all the module differ > from it. please revert for now. Sigh. > >- (void)::NS_RegisterMemoryReporter(mReporter); > >+ NS_RegisterMemoryReporter(mReporter); > > I suspect the (void) was put in to avoid compiler warnings, and the :: was > put > in out of someone's understanding of layout style. Since I'm not a layout > peer, I'm not comfortable approving of a reversion of that style. It's required for the storage module style, and then it got copied everywhere else. So removing it here (and everywhere else) is a win. Storage will continue to live in its own little world, unfortunately.
Comment 43•11 years ago
|
||
> It's required for the storage module style, and then it got copied everywhere else. So
> removing it here (and everywhere else) is a win.
Okay, if it's not a layout thing, that's fine with me.
Assignee | ||
Comment 44•11 years ago
|
||
> > + int64_t Amount() { return ::sqlite3_memory_used(); }
>
> please annotate with MOZ_OVERRIDE
I can do it in this case, but I won't do it in general because if you get the override wrong then MemoryReporterBase::GetAmount() will assert.
Assignee | ||
Comment 45•11 years ago
|
||
> So this idiom is not thread-safe. That probably invalidates some of your
> patches here. :-/
Hmm. It's not the nsCOMPtr<nsIMemoryReporter>, which points from the Foo to the FooReporter, that's the problem. It's the |nsFoo*|, which points from the FooReporter to the Foo.
Currently we have various Foo/FooReporter pairs, for singleton classes. Generally the Amount() function equivalent is static and does something like this:
x = get unique instance of Foo
if (x)
measure x
These patches have changed this to:
x = local pointer to Foo
measure x // x may be dangling
Even the first case isn't thread-safe, AFAICT, because the Foo could be destroyed between getting and check |x|.
Comment 46•11 years ago
|
||
> Even the first case isn't thread-safe, AFAICT, because the Foo could be destroyed between getting
> and checking |x|.
Yes, although I'd guess that most of our static objects are main-thread-only, so I'm most concerned with the cases where we have a FooReporter for a multithreaded, non-singleton Foo.
Updated•11 years ago
|
Attachment #704369 -
Flags: review?(jones.chris.g) → review+
Updated•11 years ago
|
Attachment #704370 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 47•11 years ago
|
||
> ISTM that a better way of doing it would be to inherit from a special class.
> E.g.
>
> class Foo : public MemoryReporterMixin<Foo>
> {
> Foo()
> : MemoryReporterMixin<Foo>(KIND_HEAP, ...)
> {}
>
> size_t SizeOfIncludingThis(...) const {...};
> };
>
> class MemoryReporterMixin<T>
> {
> int64_t Amount() { return T::SizeOfIncludingThis(...); }
> };
>
> Then the MemoryReporterMixin constructor would register the memory
> reporter, and the MemoryReporterMixin destructor would unregister it.
>
> The main trick would be refcounting. The memory reporter manager
> couldn't keep a strong ref to Foo, because then Foo would never be
> destructed. Maybe a weak reference would do.
I think this will work. The manager would keep weak references (just raw pointers should suffice, I think). When enumerating the reporters, we'd use temporary strong pointers to ensure the objects stay alive until after GetAmount et al. are called.
But there's still one problem: what if a Foo object is destroyed on another thread while we are in the middle of enumerating? We'll be mixing enumeration with deletion, which sounds bad. In fact, I think we already have that problem with the existing code!
More specifically, nsMemoryReporterManager has a lock which it takes when adding and removing reporters, and when *creating* an enumerator. But it doesn't lock when *using* an enumerator. I guess we should hold the lock the entire time the enumerator is alive? The obvious way to do this is for EnumerateReporters() to take the lock and then add a FinishEnumeratingReporters() method that releases the lock, though hopefully there's an RAII approach that avoids the need for the latter method.
Comment 48•11 years ago
|
||
> what if a Foo object is destroyed on another thread while we are in the middle of
> enumerating?
While the enumerator object is alive, it holds a strong ref to all of the "enumeratees" (the memory reporters). So I think this isn't a problem?
Assignee | ||
Comment 49•11 years ago
|
||
> While the enumerator object is alive, it holds a strong ref to all of the
> "enumeratees" (the memory reporters).
That is true, but I was worried that we might e.g. try to enumerate N elements but by the time we get to the end there are only N-1 in the array. But NeilAway informed me (and I checked) that NS_NewArrayEnumerator makes a temporary copy of the array to avoid problems with this case. So I think it's safe.
Assignee | ||
Comment 50•11 years ago
|
||
After seven months I'm resurrecting this. jlebar's concerns about races were valid, so I'm going to avoid creating FooReporter classes that contain raw pointers to Foo objects -- instead I'll stick with the existing code that typically goes via a static singleton pointer. I've rebased the patches and made that change. Since it brings the patches closer to the current code than all the previous patches, I will carry over the r+'s that are already present.
Assignee | ||
Comment 51•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #704366 -
Attachment is obsolete: true
Assignee | ||
Comment 52•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #704367 -
Attachment is obsolete: true
Assignee | ||
Comment 53•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #704369 -
Attachment is obsolete: true
Assignee | ||
Comment 54•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #704370 -
Attachment is obsolete: true
Assignee | ||
Comment 55•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #704372 -
Attachment is obsolete: true
Assignee | ||
Comment 56•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #704373 -
Attachment is obsolete: true
Assignee | ||
Comment 57•11 years ago
|
||
Assignee | ||
Comment 58•11 years ago
|
||
Assignee | ||
Comment 59•11 years ago
|
||
Assignee | ||
Comment 60•11 years ago
|
||
jlebar, you r-'d this previously because it's a reporter with a static counter but no protection against multiple instantiation. I added an assertion to the constructor, which is the same strategy used in several other reporters.
Attachment #794466 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 61•11 years ago
|
||
Assignee | ||
Comment 62•11 years ago
|
||
Assignee | ||
Comment 63•11 years ago
|
||
Assignee | ||
Comment 64•11 years ago
|
||
Assignee | ||
Comment 65•11 years ago
|
||
Assignee | ||
Comment 66•11 years ago
|
||
Assignee | ||
Comment 67•11 years ago
|
||
This reporter was added after the first round of patches was written.
Attachment #794475 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 68•11 years ago
|
||
Assignee | ||
Comment 69•11 years ago
|
||
Assignee | ||
Comment 70•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #704374 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #704379 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #704383 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #704384 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #704385 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #704391 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #704394 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #704396 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #704397 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #704407 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #704408 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #704410 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #704771 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #794456 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794457 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794459 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794460 -
Attachment is obsolete: true
Attachment #794460 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794461 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794462 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794463 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794464 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794465 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794467 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794468 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794469 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794470 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794472 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794473 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794476 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794478 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #794479 -
Flags: review+
Updated•11 years ago
|
Attachment #794466 -
Flags: review?(justin.lebar+bug) → review+
Comment 71•11 years ago
|
||
Comment on attachment 794475 [details] [diff] [review] (part 18) - Don't use NS_MEMORY_REPORTER_IMPLEMENT for the ICU reporter. >diff --git a/xpcom/build/nsXPComInit.cpp b/xpcom/build/nsXPComInit.cpp >--- a/xpcom/build/nsXPComInit.cpp >+++ b/xpcom/build/nsXPComInit.cpp >@@ -330,61 +330,66 @@ NS_GetTraceRefcnt(nsITraceRefcnt** resul > > EXPORT_XPCOM_API(nsresult) > NS_InitXPCOM(nsIServiceManager* *result, > nsIFile* binDirectory) > { > return NS_InitXPCOM2(result, binDirectory, nullptr); > } > >-// |sSizeOfICU| can be accessed by multiple JSRuntimes, so it must be >-// thread-safe. >-static Atomic<size_t> sSizeOfICU; >+class ICUReporter MOZ_FINAL : public MemoryReporterBase >+{ >+public: >+ ICUReporter() >+ : MemoryReporterBase("explicit/icu", KIND_HEAP, UNITS_BYTES, >+"Memory used by ICU, a Unicode and globalization support library.") This patch duplicates a lot of code from the previous patch I reviewed, and you may have this same code elsewhere. Is it time to factor it out? r=me because this patch is a net improvement.
Attachment #794475 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 72•11 years ago
|
||
> This patch duplicates a lot of code from the previous patch I reviewed, and
> you
> may have this same code elsewhere.
>
> Is it time to factor it out?
Yeah, as a follow-up I've been thinking about having multiple variants of MemoryReporterBase, one of which would be for counter-based reporters like this.
Assignee | ||
Comment 73•11 years ago
|
||
Parts 2--21, hooray: https://hg.mozilla.org/integration/mozilla-inbound/rev/9af570dfad86 https://hg.mozilla.org/integration/mozilla-inbound/rev/76345eaa94fc https://hg.mozilla.org/integration/mozilla-inbound/rev/3b94a25c83df https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b1120fe7f2 https://hg.mozilla.org/integration/mozilla-inbound/rev/7121f9f524ee https://hg.mozilla.org/integration/mozilla-inbound/rev/24791c073aee https://hg.mozilla.org/integration/mozilla-inbound/rev/5c86a02a4c2f https://hg.mozilla.org/integration/mozilla-inbound/rev/24b0e5ef846a https://hg.mozilla.org/integration/mozilla-inbound/rev/e704db333c96 https://hg.mozilla.org/integration/mozilla-inbound/rev/5680c6a6162f https://hg.mozilla.org/integration/mozilla-inbound/rev/b9bce0abddbd https://hg.mozilla.org/integration/mozilla-inbound/rev/6e610d6a1995 https://hg.mozilla.org/integration/mozilla-inbound/rev/22b6c5183654 https://hg.mozilla.org/integration/mozilla-inbound/rev/95e4bc825071 https://hg.mozilla.org/integration/mozilla-inbound/rev/15a219d9119e https://hg.mozilla.org/integration/mozilla-inbound/rev/24b954d5230c https://hg.mozilla.org/integration/mozilla-inbound/rev/e6b4c661c616 https://hg.mozilla.org/integration/mozilla-inbound/rev/595cec1f096a https://hg.mozilla.org/integration/mozilla-inbound/rev/e4eb75e2d77e https://hg.mozilla.org/integration/mozilla-inbound/rev/eead1e72622d
Whiteboard: [leave open]
Comment 74•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9af570dfad86 https://hg.mozilla.org/mozilla-central/rev/76345eaa94fc https://hg.mozilla.org/mozilla-central/rev/3b94a25c83df https://hg.mozilla.org/mozilla-central/rev/f6b1120fe7f2 https://hg.mozilla.org/mozilla-central/rev/7121f9f524ee https://hg.mozilla.org/mozilla-central/rev/24791c073aee https://hg.mozilla.org/mozilla-central/rev/5c86a02a4c2f https://hg.mozilla.org/mozilla-central/rev/24b0e5ef846a https://hg.mozilla.org/mozilla-central/rev/e704db333c96 https://hg.mozilla.org/mozilla-central/rev/5680c6a6162f https://hg.mozilla.org/mozilla-central/rev/b9bce0abddbd https://hg.mozilla.org/mozilla-central/rev/6e610d6a1995 https://hg.mozilla.org/mozilla-central/rev/22b6c5183654 https://hg.mozilla.org/mozilla-central/rev/95e4bc825071 https://hg.mozilla.org/mozilla-central/rev/15a219d9119e https://hg.mozilla.org/mozilla-central/rev/24b954d5230c https://hg.mozilla.org/mozilla-central/rev/e6b4c661c616 https://hg.mozilla.org/mozilla-central/rev/595cec1f096a https://hg.mozilla.org/mozilla-central/rev/e4eb75e2d77e https://hg.mozilla.org/mozilla-central/rev/eead1e72622d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in
before you can comment on or make changes to this bug.
Description
•