913260 - Add more "distinguished amounts" as attributes of nsMemoryReporterManager

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nicholas Nethercote [inactive]]

In bug 911641 I wrestled with the issue that about:memory wants to skip a small number of memory reports, because they're redundant w.r.t. others.  (They are used for telemetry, however.)  I wrote a patch that prefixed these reports with "redundant/", which was an improvement over the status quo but still left me unhappy.

More generally, we have a problem that anybody who wants a subset of all the memory reports has to filter based on paths, and this creates an implicit coupling between the reporters and the consumers of their reports.  And that makes it hard to know what will break if we change any of the reporter paths.

But we already have the beginnings of a mechanism to improve this... nsMemoryReporterManager has |explicit| and |resident| attributes, because those are commonly-required measurements.  Consider |resident| in particular -- we have a reporter with the name/path "resident", and it uses the same implementation as the |resident| attribute.  We could add more of these attributes.

For example, the "resident-fast" is currently explicitly ignored by about:memory, and is only used for telemetry.  If we removed the "resident-fast" reporter, and added a |residentFast| attribute to nsMemoryReporterManager, then TelemetryPing.js could access that attribute directly, and about:memory would no longer have to ignore it.  Likewise for the user and system compartment counts reporters.  And likewise for all the reporters that telemetry currently accesses (though we could have both attributes and reporters for ones that we do want to show in about:memory, such as "resident" and "vsize").

Advantages of this.

- about:memory doesn't need to ignore any reporters.

- Telemetry can take measurements directly from attributes, rather than iterating through all reporters and matching on names.

- The coupling is still present, but it's more explicit.  If a measurement is done for an attribute, that indicates a greater level of reliability;  a consumer can assume an attribute is less likely to disappear than a random report.  At the very least, the breaking of tools that rely on attributes would require a change of nsMemoryReporterManager's UUID!

There are some interesting cases.  For example, the under-development memory profiler tool (https://github.com/past/memory-profiler) pulls out a subset of the DOM and JS reports.  And http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/test/harness.js#321 pulls out some compartment and zone measurements.  If we felt these cases were sufficiently important, we could commit to putting them into an attribute.

This feels like it strikes a good balance.  We can say "don't rely on the paths staying the same".  If someone is prototyping a new tool, they can work with the paths.  Once that tool becomes sufficiently mature and important, we can add an attribute.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Simplify TelemetryPing.js a little.

This patch:

- Removes all the trailing whitespace in TelemetryPing.js.

- Inlines TelemetryPing.addValue(), because it only has one caller.

- Removes the |path| arg of handleMemoryReport().  We now use |id| instead.
  This is valid because the two uses of |path| were as keys for the
  |_prevValues| and |_histograms| tables, and |id| also works for that, because
  each |path|/|id| pair is unique, so we're just trading one unique key for
  another.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Tweak ghost window reporters.

This patch:

- Folds GhostURLsReporter into nsWindowMemoryReporter.  It doesn't need to be 
  separate now that about:compartments is gone.

- Renames NumGhostReporter as GhostWindowsReporter, to match the 
  "ghost-windows" report path.

- Replaces GhostWindowsReporter::mWindowReporter with a static variable.  This
  is necessary for |ghostWindows| to be added as a distinguished amount in part
  4.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Formalize the concept of "distinguished amounts" in the memory reporter manager.

This patch formalizes the notion of "distinguished amounts" in the memory
reporter manager.  We already had |explicit| and |resident|;  this one converts
the three existing "redundant/"-prefixed reporters to distinguished amounts.
As a result, about:memory no longer needs to ignore any reporters/reports,
which is nice.  (This also means that the "resident-fast" measurement isn't
taken when dumping reports to file, or when sending them from a child process
to a parent process, which is also good.)

It also measures them in test_memoryReporters.xul, and improves how that test's
amount-checking works.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Use distinguished amounts for all the memory measurements done by telemetry.

This patch converts telemetry so it only uses distinguished amounts, instead of
memory reporters.  This prevents the current problem where telemetry could be
silently broken if someone changed one of the reporters it was relying on.  We
also no longer need the code that checks that a reporter used by telemetry is a
uni-reporter.

The following values were also kept as memory reporters, and so still show up
in about:memory:
- MEMORY_VSIZE
- MEMORY_JS_MAIN_RUNTIME_TEMPORARY_PEAK
- MEMORY_HEAP_ALLOCATED
- MEMORY_HEAP_COMMITTED_UNUSED_RATIO
- PAGE_FAULTS_HARD
- MEMORY_EVENTS_VIRTUAL
- MEMORY_EVENTS_PHYSICAL
- GHOST_WINDOWS

The following were not kept as memory reporters, and so don't show up in
about:memory (because they're redundant w.r.t. something else):
- MEMORY_IMAGES_CONTENT_USED_UNCOMPRESSED
- MEMORY_JS_GC_HEAP
- MEMORY_STORAGE_SQLITE

That leaves MEMORY_HEAP_COMMITTED_UNUSED -- turns out the reporter associated
with this telemetry histogram id got removed a while back!  This is exactly the
kind of breakage of implicit dependencies that this change will help avoid.

Comment 5

[Nicholas Nethercote [inactive]]

> - Replaces GhostWindowsReporter::mWindowReporter with a static variable. 
> This is necessary for |ghostWindows| to be added as a distinguished amount in
> part 4.

Ugh, this causes leak checks to fail on TBPL, e.g.:

TEST-UNEXPECTED-FAIL | leakcheck | 3336 bytes leaked (nsWeakReference, nsWindowMemoryReporter)

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Tweak ghost window reporters.

New version uses ClearOnShutdown to avoid the StaticRefPtr leak.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 806387 [details] [diff] [review]
(part 1) - Simplify TelemetryPing.js a little.

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

I wonder whether the mini-cache for getHistogramById is really necessary.  But that'd be a followup bug in any event; would you be so kind as to file that?

Comment 8

[Nicholas Nethercote [inactive]]

> I wonder whether the mini-cache for getHistogramById is really necessary. 
> But that'd be a followup bug in any event; would you be so kind as to file
> that?

Bug 918557.

Comment 9

[Nicholas Nethercote [inactive]]

Part 1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bca7f3175b16

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bca7f3175b16

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 807063 [details] [diff] [review]
(part 2) - Tweak ghost window reporters.

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

(My reviews were delayed because I was on PTO last week.)

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 806389 [details] [diff] [review]
(part 3) - Formalize the concept of "distinguished amounts" in the memory reporter manager.

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

Is there some guiding philosophy you have in mind for making a measurement distinguished?  The explicit etc. ones seem obviously important, but the JS compartment counts don't seem to be to me.  Are they there for telemetry or something?  Maybe add a little to the top of your comment in nsIMemoryReporter.idl about when somebody might want to add one, or just a blanket "many of these are for telemetry", as that seems to be happening in the next patch.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +147,4 @@
>   * @param aHandleReport
>   *        The function that's called for each non-skipped report.
>   */
> +function processMemoryReporters(aHandleReport)

woo!

@@ -238,5 @@
>  
>    switch (aFooterAction) {
>     case HIDE_FOOTER:   gFooter.classList.add('hidden');    break;
>     case SHOW_FOOTER:   gFooter.classList.remove('hidden'); break;
> -   case IGNORE_FOOTER:                                     break;

The removal of the unused IGNORE_FOOTER is just a cleanup you noticed while you were looking at the code?

::: toolkit/components/aboutmemory/tests/test_memoryReporters.xul
@@ +109,5 @@
>  
>    let mgr = Cc["@mozilla.org/memory-reporter-manager;1"].
>              getService(Ci.nsIMemoryReporterManager);
>  
> +  // Access the distinguished amounts (mgr.explicit  et al) just to make sure

nits: extra space?  Also, do you mean "etc." not "et al"?  "et al" means something like, the same as the previous citation.

@@ +158,5 @@
>    checkSpecialReport("storage-sqlite", storageSqliteAmounts);
>  
> +  ok(present.jsNonWindowCompartments,     "js-non-window compartments are present");
> +  ok(present.windowObjectsJsCompartments, "window-objects/.../js compartments are present");
> +  ok(present.sandboxLocation,             "sandbox locations are present");

nit: in the above series of branches sandboxLocation is after atomTable, not windowObjectsJsCompartments, so it might be nice to have the order be consistent here. Otherwise, they are in the same order.

::: toolkit/components/telemetry/TelemetryPing.js
@@ +480,5 @@
> +    // Get memory measurements from distinguished amount attributes.
> +    let h = this.handleMemoryReport.bind(this);
> +    let b = (id, n) => h(id, Ci.nsIMemoryReporter.UNITS_BYTES, n);
> +    let c = (id, n) => h(id, Ci.nsIMemoryReporter.UNITS_COUNT, n);
> +    let p = (id, n) => h(id, Ci.nsIMemoryReporter.UNITS_PERCENTAGE, n);

nit: p is unused

Comment 13

[Andrew McCreight [:mccr8]]

FWIW, I don't find "distinguished amounts" to be a particularly self-describing term :)

The memory reporter MDN page should be updated a bit to talk about this.

Comment 14

[Nicholas Nethercote [inactive]]

> (My reviews were delayed because I was on PTO last week.)

Yep, no problem.

> Is there some guiding philosophy you have in mind for making a measurement
> distinguished?

No.  It's a grey area.  Though all the telemetry ones end up there, as you'll see in the next patch.

> The removal of the unused IGNORE_FOOTER is just a cleanup you noticed while
> you were looking at the code?

Yes.

> Also, do you mean "etc." not "et al"?  "et al" means
> something like, the same as the previous citation.

http://en.wiktionary.org/wiki/et_al. says:

> et al.
>    And others; to complete a list, especially of people, as authors of a published work.
> Usage notes
>    Formerly, this was preferred by some over etc. for lists of people in all
>    contexts. At present the two abbreviations are used synonymously in many
>    contexts for completing lists. However, in lists of authors of a published
>    work et al. is still used.

So I think my usage is fine, though I'll add the final '.'.
</pedantic>

> nit: p is unused

It's used in the next patch :)

> FWIW, I don't find "distinguished amounts" to be a particularly
> self-describing term :)

True, and it's also quite long.  Any suggested alternatives?  I want to avoid using "reporter" or "reports", because I initially used "distinguished reports" and then I had to start writing "vanilla reports" or "normal" to contrast, which was bad.  I chose "amount" because it corresponds to the "amount" in a memory report.

> The memory reporter MDN page should be updated a bit to talk about this.

Yes.

Comment 15

[Andrew McCreight [:mccr8]]

Comment on attachment 806390 [details] [diff] [review]
(part 4) - Use distinguished amounts for all the memory measurements done by telemetry.

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

::: image/src/imgLoader.cpp
@@ -230,5 @@
> -public:
> -  ImagesContentUsedUncompressedReporter()
> -    : MemoryUniReporter("images-content-used-uncompressed",
> -                         KIND_OTHER, UNITS_BYTES,
> -"This is the sum of the 'explicit/images/content/used/uncompressed-heap' "

Should something like this comment be added to nsIMemoryReporter.idl?

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1593,1 @@
>  class JSMainRuntimeTemporaryPeakReporter MOZ_FINAL : public MemoryUniReporter

You can delete this reporter now, as it is unused.

::: xpcom/base/nsIMemoryReporter.idl
@@ +344,5 @@
>  
> +REGISTER_DISTINGUISHED_AMOUNT(Infallible, ImagesContentUsedUncompressed)
> +
> +REGISTER_DISTINGUISHED_AMOUNT(Infallible, StorageSQLite)
> +UNREGISTER_DISTINGUISHED_AMOUNT(StorageSQLite)

As in the other place, the register followed by unregister is odd.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +1339,5 @@
>  
> +REGISTER_DISTINGUISHED_AMOUNT(Infallible, ImagesContentUsedUncompressed)
> +
> +REGISTER_DISTINGUISHED_AMOUNT(Infallible, StorageSQLite)
> +UNREGISTER_DISTINGUISHED_AMOUNT(StorageSQLite)

Why do you register this, then immediately unregister it? An explanation here might be nice.

Comment 16

[Nicholas Nethercote [inactive]]

> > -  ImagesContentUsedUncompressedReporter()
> > -    : MemoryUniReporter("images-content-used-uncompressed",
> > -                         KIND_OTHER, UNITS_BYTES,
> > -"This is the sum of the 'explicit/images/content/used/uncompressed-heap' "
> 
> Should something like this comment be added to nsIMemoryReporter.idl?

I have this:

   * |imagesContentUsedUncompressed| (UNITS_BYTES)  Memory used for decoded
   * images in content.

Should I add more, or can I make this clearer?


> ::: js/xpconnect/src/XPCJSRuntime.cpp
> @@ +1593,1 @@
> >  class JSMainRuntimeTemporaryPeakReporter MOZ_FINAL : public MemoryUniReporter
> 
> You can delete this reporter now, as it is unused.

Whoops, that's a mistake!  I'll reinstate the code to register it as a reporter.  Good catch.


> > +REGISTER_DISTINGUISHED_AMOUNT(Infallible, StorageSQLite)
> > +UNREGISTER_DISTINGUISHED_AMOUNT(StorageSQLite)
> 
> Why do you register this, then immediately unregister it? An explanation
> here might be nice.

Those macros don't actually do the registering/unregistering;  they create the functions that do that.  Could I rename these to make it clearer?  Maybe "CREATE_REGISTER_DISTINGUISHED_AMOUNT_FN"?

Comment 17

[Andrew McCreight [:mccr8]]

> Any suggested alternatives?
Yeah, not really.  I can understand why you don't want to call it some kind of reporter, but the "distinguished amounts" functions are just single-purpose memory reporters.  Not that "single-purpose reporter" is a good name, either.

(In reply to Nicholas Nethercote [:njn] from comment #16)
> Should I add more, or can I make this clearer?

Well, that's a good description, but it loses the bit about how it is the sum of those other values.  I don't know if that really matters.

> Whoops, that's a mistake!  I'll reinstate the code to register it as a
> reporter.  Good catch.
Yeah, after I read the changes in the other files, I figured that you meant to actually register it.

> Those macros don't actually do the registering/unregistering;  they create
> the functions that do that.  Could I rename these to make it clearer?  Maybe
> "CREATE_REGISTER_DISTINGUISHED_AMOUNT_FN"?
Oh, I see!  Yeah, _FN or something would be good.  I read the body of the functions, but failed to notice they were functions...

Comment 18

[Nicholas Nethercote [inactive]]

> the "distinguished amounts" functions are just
> single-purpose memory reporters

No, they're just the amount -- no process, path, units, kind or description that a full report has.

Comment 19

[Nicholas Nethercote [inactive]]

> Yeah, _FN or something would be good.

I'll add DECL_ and DEFINE_ prefixes, since those are a proud Mozilla tradition.

Comment 20

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8691a26012
https://hg.mozilla.org/integration/mozilla-inbound/rev/09c71a3e7b85
https://hg.mozilla.org/integration/mozilla-inbound/rev/9195be8a50cb

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/753597994318
https://hg.mozilla.org/mozilla-central/rev/fec6f224f378
https://hg.mozilla.org/mozilla-central/rev/9e162ee7d4d1