Add more "distinguished amounts" as attributes of nsMemoryReporterManager

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla27
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: Special-case more memory measurements as attributes of nsMemoryReporterManager → Add more "distinguished amounts" as attributes of nsMemoryReporterManager
(Assignee)

Updated

5 years ago
Blocks: 725248
(Assignee)

Comment 1

5 years ago
Created attachment 806387 [details] [diff] [review]
(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.
Attachment #806387 - Flags: review?(nfroyd)
(Assignee)

Comment 2

5 years ago
Created attachment 806388 [details] [diff] [review]
(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.
Attachment #806388 - Flags: review?(continuation)
(Assignee)

Comment 3

5 years ago
Created attachment 806389 [details] [diff] [review]
(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.
Attachment #806389 - Flags: review?(continuation)
(Assignee)

Comment 4

5 years ago
Created attachment 806390 [details] [diff] [review]
(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.
Attachment #806390 - Flags: review?(continuation)
(Assignee)

Comment 5

5 years ago
> - 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)
(Assignee)

Comment 6

5 years ago
Created attachment 807063 [details] [diff] [review]
(part 2) - Tweak ghost window reporters.

New version uses ClearOnShutdown to avoid the StaticRefPtr leak.
Attachment #807063 - Flags: review?(continuation)
(Assignee)

Updated

5 years ago
Attachment #806388 - Attachment is obsolete: true
Attachment #806388 - Flags: review?(continuation)
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?
Attachment #806387 - Flags: review?(nfroyd) → review+
(Assignee)

Updated

5 years ago
Blocks: 918557
(Assignee)

Comment 8

5 years ago
> 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 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.)
Attachment #807063 - Flags: review?(continuation) → review+
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
Attachment #806389 - Flags: review?(continuation) → review+
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.
Keywords: dev-doc-needed
(Assignee)

Comment 14

5 years ago
> (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 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.
Attachment #806390 - Flags: review?(continuation) → review+
(Assignee)

Comment 16

5 years ago
> > -  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"?
> 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...
(Assignee)

Comment 18

5 years ago
> 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.
(Assignee)

Comment 19

5 years ago
> Yeah, _FN or something would be good.

I'll add DECL_ and DEFINE_ prefixes, since those are a proud Mozilla tradition.
https://hg.mozilla.org/mozilla-central/rev/753597994318
https://hg.mozilla.org/mozilla-central/rev/fec6f224f378
https://hg.mozilla.org/mozilla-central/rev/9e162ee7d4d1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Updated

5 years ago
Blocks: 920366
You need to log in before you can comment on or make changes to this bug.