The default bug view has changed. See this FAQ.

Add UNITS_COUNT_CUMULATIVE to nsIMemoryReporter

RESOLVED FIXED in mozilla8

Status

()

Toolkit
Telemetry
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: Justin Lebar (not reading bugmail))

Tracking

({dev-doc-complete})

unspecified
mozilla8
x86_64
Linux
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
This is a follow-up to bug 672694, which added the js-compartment-count reporter.  From bug  672694 comment 3:

> TelemetryPing.js has code that assumes that any UNITS_COUNT reporter 
> reports a number that always increases, and so reports the change between
> pings.  This makes sense for the page-fault counters, but not for
> js-compartment-count...  We might need to distinguish between UNITS_COUNT
> and UNITS_COUNT_INCREASING.

jlebar, I'll let you figure out the best way to handle this :)
(Assignee)

Comment 1

6 years ago
Adding a new unit is the right thing to do, I think.
(Assignee)

Comment 2

6 years ago
Created attachment 547227 [details] [diff] [review]
Part 1 - Add UNITS_COUNT_CUMULATIVE.

I purposely left the horizontal alignment alone in a few places, because I thought the whitespace added above "_CUMULATIVE" is ugly.
Attachment #547227 - Flags: review?(nnethercote)
(Reporter)

Comment 3

6 years ago
Comment on attachment 547227 [details] [diff] [review]
Part 1 - Add UNITS_COUNT_CUMULATIVE.

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

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +267,5 @@
>    if (a._units != b._units) {
>      return a._units - b._units;   // use the enum order from nsIMemoryReporter
>    }
> +  if (a._units == UNITS_COUNT ||
> +      a._units == UNITS_COUNT_CUMULATIVE) {

This has bitrotted, but in the new code you won't need to make any changes.

@@ +533,5 @@
>  {
>    switch(aReporter._units) {
> +    case UNITS_BYTES:            return formatBytes(aReporter._amount);
> +    case UNITS_COUNT:            return formatInt(aReporter._amount);
> +    case UNITS_COUNT_CUMULATIVE: return formatInt(aReporter._amount);

You can fall through the first case and avoid the code duplication.

::: toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul
@@ +52,5 @@
>    const OTHER   = Ci.nsIMemoryReporter.KIND_OTHER;
>  
>    const BYTES = Ci.nsIMemoryReporter.UNITS_BYTES;
>    const COUNT = Ci.nsIMemoryReporter.UNITS_COUNT; 
> +  const COUNT_CUMULATIVE = Ci.nsIMemoryReporter.UNITS_COUNT_CUMULATIVE;

Can you use COUNT_CUMULATIVE somewhere in the test?  Either add a new fake reporter (which will require updating amExpectedText and amvExpectedText) or, if you're lazy, just change one of the COUNT occurrences to COUNT_CUMULATIVE.

::: xpcom/base/nsIMemoryReporter.idl
@@ +150,4 @@
>     */
>    const PRInt32 UNITS_BYTES = 0;
>    const PRInt32 UNITS_COUNT = 1;
> +  const PRInt32 UNITS_COUNT_CUMULATIVE = 2;

UNITS_PERCENTAGE snuck in and took the value 2, but I don't see why it can't be bumped to 3, esp. as we haven't shipped it yet.

This change'll need dev-doc-needed, BTW.
Attachment #547227 - Flags: review?(nnethercote) → review+
(Assignee)

Comment 4

6 years ago
I'm morphing this bug into adding UNITS_COUNT_CUMULATIVE to nsIMemoryReporter (rather than "Telemeterize js-compartment-count", which requires COUNT_CUMULATIVE as a prerequisite) so I can land COUNT_CUMULATIVE and avoid bitrotting Jez in bug 670084 further.
Summary: Telemeterize js-compartment-count → Add UNITS_COUNT_CUMULATIVE to nsIMemoryReporter
(Assignee)

Comment 5

6 years ago
Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/9b1a8e3b99aa

Bug 673837 is the new telemeterize js-compartment-count bug.
Blocks: 673837
(Assignee)

Updated

6 years ago
Keywords: dev-doc-needed
Whiteboard: [MemShrink] → [MemShrink][inbound]
http://hg.mozilla.org/mozilla-central/rev/9b1a8e3b99aa
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
(Reporter)

Comment 7

6 years ago
jlebar, the old comment for UNITS_COUNT talks about it being cumulative, which is no longer correct.  (trevorh on IRC spotted this.)  Can you fix this?  A follow-up here seems good enough.  Thanks!
(Assignee)

Comment 8

6 years ago
Thanks for catching this.  The patch I posted fixed this; I just failed at merging it.  I'll land what you already reviewed.
(Assignee)

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/7454890376fc

Updated

6 years ago
Keywords: dev-doc-needed → dev-doc-complete
http://hg.mozilla.org/mozilla-central/rev/7454890376fc
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink][inbound] → [MemShrink]
You need to log in before you can comment on or make changes to this bug.