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)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

Attachments

(1 file, 2 obsolete files)

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.
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)
Priority: -- → P2
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)
Whoops. Here's the patch as intended.
Attachment #8890074 - Flags: review?(erahm)
Attachment #8889719 - Attachment is obsolete: true
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+
> > +  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.
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 :(
> > 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.
(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|.
(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 ...
(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.
> Maybe split the difference and go for slightly shorter `MallocState` or
> `MemTable` or ...

I originally had MallocBundle but I thought that MRState was better...
> 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.
New version addresses most of the comments.
Attachment #8891145 - Flags: review?(erahm)
Attachment #8890074 - Attachment is obsolete: true
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+
> Looks good! Can you update your commit message with the new name?

Yes!

Thank you for the fast and thorough reviews.
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.
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)
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.
https://hg.mozilla.org/mozilla-central/rev/e0d2ec43d7b2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Flags: needinfo?(n.nethercote)
You need to log in before you can comment on or make changes to this bug.