Closed
Bug 1383982
Opened 7 years ago
Closed 7 years ago
Introduce a general mechanism for measuring memory usage of graph-like structures
Categories
(Core :: XPCOM, enhancement, P2)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(1 file, 2 obsolete files)
47.35 KB,
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
We've never had a good way of measuring the memory usage of graph-like structures, e.g. those using ref-counted objects. One common idea is to divide the size by the refcount, but that has problems: the result can be fractional, and DMD doesn't handle it. Another common idea is to record which objects have been already measured. This can be done internally, by setting a flag within the object; the same flag must be cleared later on. Or it can be done externally, with a separate table. The external table approach is the most general and is already used in a couple of reporters, so I plan to standardize on that by providing a more general mechanism for it that can be shared across different memory reporters.
Assignee | ||
Comment 1•7 years ago
|
||
All the SizeOf{In,Ex}cludingThis() functions take a MallocSizeOf function which measures memory blocks. This patch introduces a new type, MRState, which includes a MallocSizeOf function *and* a table of already-measured pointers, called SeenPtrs. This gives us a general mechanism to measure graph-like data structures, by recording which nodes have already been measured. (This approach is used in a number of existing reporters, but not in a uniform fashion.) The patch also converts the window memory reporting to use MRState in a lot of places, all the way through to the measurement of Elements. This is a precursor for bug 1383977 which will measure Stylo elements, which involve Arcs. The patch also converts the existing mAlreadyMeasuredOrphanTrees table in the OrphanReporter to use the new mechanism.
Attachment #8889719 -
Flags: review?(erahm)
Updated•7 years ago
|
Priority: -- → P2
Comment 2•7 years ago
|
||
Comment on attachment 8889719 [details] [diff] [review] Introduce a general mechanism for measuring memory usage of graph-like structures Review of attachment 8889719 [details] [diff] [review]: ----------------------------------------------------------------- MRState appears to be missing. Can you add that as a separate patch? It'll make it easier to discuss the design.
Attachment #8889719 -
Flags: review?(erahm)
Assignee | ||
Comment 3•7 years ago
|
||
Whoops. Here's the patch as intended.
Attachment #8890074 -
Flags: review?(erahm)
Assignee | ||
Updated•7 years ago
|
Attachment #8889719 -
Attachment is obsolete: true
Comment 4•7 years ago
|
||
Comment on attachment 8890074 [details] [diff] [review] Introduce a general mechanism for measuring memory usage of graph-like structures Review of attachment 8890074 [details] [diff] [review]: ----------------------------------------------------------------- I have some thoughts on possible changes to MRState, so f+ for now. The rest of the implementation looks fine. ::: dom/base/nsINode.h @@ +266,5 @@ > // SizeOfExcludingThis from its super-class. SizeOfIncludingThis() need not be > // defined, it is inherited from nsINode. > // This macro isn't actually specific to nodes, and bug 956400 will move it into MFBT. > #define NS_DECL_SIZEOF_EXCLUDING_THIS \ > + virtual size_t SizeOfExcludingThis(mozilla::MRState* aState) const override; A ref seems more appropriate, is there a reason you went with a pointer? ::: xpcom/base/MRState.h @@ +21,5 @@ > + > +// A table that records seen pointers. Useful when measuring structures that > +// contain nodes that may be pointed to from multiple places, e.g. via RefPtr > +// (in C++ code) or Arc (in Rust code). > +class SeenPtrs : public nsTHashtable<nsPtrHashKey<const void>> This seems a little heavy, could we just move `HaveSeenPtr` to the state struct, store a hashtable there and hide the implementation details? @@ +26,5 @@ > +{ > +public: > + // Returns true if we have seen this pointer before, false otherwise. Also > + // remembers this pointer for later queries. > + bool HaveSeenPtr(const void* aPtr) Maybe just `NotePtr` or something along those lines? `HaveSeen` seems to imply we're just doing a lookup, not actually adding the pointer. @@ +32,5 @@ > + uint32_t oldCount = Count(); > + if (!PutEntry(aPtr, fallible)) { > + // If PutEntry() OOMs we can't tell if this pointer has been seen before. > + // When doing memory reporting it's better to err on the side of > + // under-reporting rather than over-reporting, so we return true. The comment's helpful, but we don't really need the if right? @@ +42,5 @@ > + return oldCount == Count(); > + } > +}; > + > +struct MRState I'd prefer this name to be more descriptive, it's not super clear what MR stands for (I'm assuming "memory report"). We're already pretty verbose with `MallocSizeOf`, maybe just be verbose and call it `MemoryReportState` or `PointerTracker` etc. A comment explaining the intended use would also be nice. @@ +48,5 @@ > + MRState(MallocSizeOf aMallocSizeOf) > + : mMallocSizeOf(aMallocSizeOf) > + {} > + > + MallocSizeOf mMallocSizeOf; What do you think about adding a conversion operator: > operator MallocSizeOf() { return mMallocSizeOf; } And then we could just do: > size_t SizeOfExcludingThis(MRState& aState) const { > mFoo.SizeOfExcludingThisThatJustTakesMallocSizeOf(aState); > } ::: xpcom/base/moz.build @@ +45,5 @@ > EXPORTS += [ > '!ErrorList.h', > '!ErrorNamesInternal.h', > 'CodeAddressService.h', > + 'MRState.h', This should probably be exported under 'EXPORTS.mozilla'.
Attachment #8890074 -
Flags: review?(erahm) → feedback+
Assignee | ||
Comment 5•7 years ago
|
||
> > + virtual size_t SizeOfExcludingThis(mozilla::MRState* aState) const override; > > A ref seems more appropriate, is there a reason you went with a pointer? Um, not really. I'll change to a reference. > > +// A table that records seen pointers. Useful when measuring structures that > > +// contain nodes that may be pointed to from multiple places, e.g. via RefPtr > > +// (in C++ code) or Arc (in Rust code). > > +class SeenPtrs : public nsTHashtable<nsPtrHashKey<const void>> > > This seems a little heavy, could we just move `HaveSeenPtr` to the state > struct, store a hashtable there and hide the implementation details? There's an additional constraint that's not obvious here, which is that these types will be passed to Rust code, for measuring Stylo. The pointer lookup has to be done in C++, so we need an opaque type and a C function that Rust code can call to do pointer lookups. I could make MRState the opaque type, but then I'd need another C function to extract the MallocSizeOf. So instead I make SeenPtrs the opaque type, and pass MallocSizeOf and SeenPtrs separately to Rust, so the Rust code can use MallocSizeOf directly. I could add a HaveSeenPtr() method to MRState, though. > > + bool HaveSeenPtr(const void* aPtr) > > Maybe just `NotePtr` or something along those lines? `HaveSeen` seems to > imply we're just doing a lookup, not actually adding the pointer. I chose that name because it makes the meaning of the bool return value obvious. If I had "if (t.NotePtr(p)) ...", what does a |true| return value mean? We've seen it? We've added it? I also think the fact that we're doing an add is clear. Consider this sequence: > t.HaveSeenPtr(0x1234); // false > t.HaveSeenPtr(0x1234); // true > > + uint32_t oldCount = Count(); > > + if (!PutEntry(aPtr, fallible)) { > > + // If PutEntry() OOMs we can't tell if this pointer has been seen before. > > + // When doing memory reporting it's better to err on the side of > > + // under-reporting rather than over-reporting, so we return true. > > The comment's helpful, but we don't really need the if right? Sorry, I don't understand the question. > > +struct MRState > > I'd prefer this name to be more descriptive, it's not super clear what MR > stands for (I'm assuming "memory report"). We're already pretty verbose with > `MallocSizeOf`, maybe just be verbose and call it `MemoryReportState` or > `PointerTracker` etc. I was deliberately avoiding long names precisely because we already are verbose and this name gets used a lot :/ We have other acronyms like DOM and XHR... > What do you think about adding a conversion operator: > > > operator MallocSizeOf() { return mMallocSizeOf; } > > And then we could just do: > > > size_t SizeOfExcludingThis(MRState& aState) const { > > mFoo.SizeOfExcludingThisThatJustTakesMallocSizeOf(aState); > > } Much too magical for my liking! :) > ::: xpcom/base/moz.build > @@ +45,5 @@ > > EXPORTS += [ > > '!ErrorList.h', > > '!ErrorNamesInternal.h', > > 'CodeAddressService.h', > > + 'MRState.h', > > This should probably be exported under 'EXPORTS.mozilla'. Good point.
Assignee | ||
Comment 6•7 years ago
|
||
I tried changing MRState to MemoryReporting state and it does make a bunch type signatures that currently fit on a single line have to be split onto two :(
Assignee | ||
Comment 7•7 years ago
|
||
> > The comment's helpful, but we don't really need the if right?
>
> Sorry, I don't understand the question.
Oh, I get it now. Yeah, it looks like it can be removed.
Comment 8•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #7) > > > The comment's helpful, but we don't really need the if right? > > > > Sorry, I don't understand the question. > > Oh, I get it now. Yeah, it looks like it can be removed. Yeah sorry should have said |if|.
Comment 9•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #6) > I tried changing MRState to MemoryReporting state and it does make a bunch > type signatures that currently fit on a single line have to be split onto > two :( Maybe split the difference and go for slightly shorter `MallocState` or `MemTable` or ...
Comment 10•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #5) > There's an additional constraint that's not obvious here, which is that > these types will be passed to Rust code, for measuring Stylo. The pointer > lookup has to be done in C++, so we need an opaque type and a C function > that Rust code can call to do pointer lookups. I could make MRState the > opaque type, but then I'd need another C function to extract the > MallocSizeOf. So instead I make SeenPtrs the opaque type, and pass > MallocSizeOf and SeenPtrs separately to Rust, so the Rust code can use > MallocSizeOf directly. Oh okay, roughly "because rust." None of that really makes sense to me without code to reference so I'll take your word for it. > I could add a HaveSeenPtr() method to MRState, though. > > > > > + bool HaveSeenPtr(const void* aPtr) > > > > Maybe just `NotePtr` or something along those lines? `HaveSeen` seems to > > imply we're just doing a lookup, not actually adding the pointer. > > I chose that name because it makes the meaning of the bool return value > obvious. If I had "if (t.NotePtr(p)) ...", what does a |true| return value > mean? We've seen it? We've added it? > > I also think the fact that we're doing an add is clear. Consider this > sequence: > > > t.HaveSeenPtr(0x1234); // false > > t.HaveSeenPtr(0x1234); // true It's still not obvious to me w/o the comments, but since you'll be the main person using this code I'll leave it up to you. > > What do you think about adding a conversion operator: > > > > > operator MallocSizeOf() { return mMallocSizeOf; } > > > > And then we could just do: > > > > > size_t SizeOfExcludingThis(MRState& aState) const { > > > mFoo.SizeOfExcludingThisThatJustTakesMallocSizeOf(aState); > > > } > > Much too magical for my liking! :) Fair enough, it would've avoided some churn but not a big deal.
Assignee | ||
Comment 11•7 years ago
|
||
> Maybe split the difference and go for slightly shorter `MallocState` or
> `MemTable` or ...
I originally had MallocBundle but I thought that MRState was better...
Assignee | ||
Comment 12•7 years ago
|
||
> Oh okay, roughly "because rust." None of that really makes sense to me
> without code to reference so I'll take your word for it.
Yeah :) Basically it makes the Rust side simpler if SeenPtrs is separately visible from MRState.
Assignee | ||
Comment 13•7 years ago
|
||
New version addresses most of the comments.
Attachment #8891145 -
Flags: review?(erahm)
Assignee | ||
Updated•7 years ago
|
Attachment #8890074 -
Attachment is obsolete: true
Comment 14•7 years ago
|
||
Comment on attachment 8891145 [details] [diff] [review] Introduce a general mechanism for measuring memory usage of graph-like structures Review of attachment 8891145 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Can you update your commit message with the new name?
Attachment #8891145 -
Flags: review?(erahm) → review+
Assignee | ||
Comment 15•7 years ago
|
||
> Looks good! Can you update your commit message with the new name?
Yes!
Thank you for the fast and thorough reviews.
Assignee | ||
Comment 16•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a57d8f30d1bf5de3ba5201a6f5e2a08ef1cf7d85 Bug 1383982 - Introduce a general mechanism for measuring memory usage of graph-like structures. r=erahm.
Comment 17•7 years ago
|
||
Backed out for build bustage at nsGlobalWindow.cpp:13826: 'class nsWindowSizes' has no member named 'mMallocSizeOf': https://hg.mozilla.org/integration/mozilla-inbound/rev/66e6d95dace315b3aa6ad2c1f6f21d2aa8a9a063 Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a57d8f30d1bf5de3ba5201a6f5e2a08ef1cf7d85&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=118845342&repo=mozilla-inbound [task 2017-07-28T07:33:10.406788Z] 07:33:10 INFO - /home/worker/workspace/build/src/dom/base/nsGlobalWindow.cpp: In member function 'void nsGlobalWindow::AddSizeOfIncludingThis(nsWindowSizes*) const': [task 2017-07-28T07:33:10.409477Z] 07:33:10 INFO - /home/worker/workspace/build/src/dom/base/nsGlobalWindow.cpp:13826:53: error: 'class nsWindowSizes' has no member named 'mMallocSizeOf' [task 2017-07-28T07:33:10.409556Z] 07:33:10 INFO - mPerformance->SizeOfUserEntries(aWindowSizes->mMallocSizeOf); [task 2017-07-28T07:33:10.409610Z] 07:33:10 INFO - ^ [task 2017-07-28T07:33:10.409685Z] 07:33:10 INFO - /home/worker/workspace/build/src/dom/base/nsGlobalWindow.cpp:13828:57: error: 'class nsWindowSizes' has no member named 'mMallocSizeOf' [task 2017-07-28T07:33:10.409740Z] 07:33:10 INFO - mPerformance->SizeOfResourceEntries(aWindowSizes->mMallocSizeOf); [task 2017-07-28T07:33:10.409792Z] 07:33:10 INFO - ^ [task 2017-07-28T07:33:10.409857Z] 07:33:10 INFO - /home/worker/workspace/build/src/config/rules.mk:1050: recipe for target 'nsGlobalWindow.o' failed [task 2017-07-28T07:33:10.409903Z] 07:33:10 INFO - gmake[5]: *** [nsGlobalWindow.o] Error 1
Flags: needinfo?(n.nethercote)
Assignee | ||
Comment 18•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d2ec43d7b204be47b195c566b55eb8723e5cc6 Bug 1383982 (attempt 2) - Introduce a general mechanism for measuring memory usage of graph-like structures. r=erahm.
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e0d2ec43d7b2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(n.nethercote)
You need to log in
before you can comment on or make changes to this bug.
Description
•