672731 - Add UNITS_COUNT_CUMULATIVE to nsIMemoryReporter

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Nicholas Nethercote [inactive]]

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 :)

Comment 1

[Justin Lebar (not reading bugmail)]

Adding a new unit is the right thing to do, I think.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: 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.

Comment 3

[Nicholas Nethercote [inactive]]

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.

Comment 4

[Justin Lebar (not reading bugmail)]

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.

Comment 5

[Justin Lebar (not reading bugmail)]

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

Bug 673837 is the new telemeterize js-compartment-count bug.

Comment 6

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/9b1a8e3b99aa

Comment 7

[Nicholas Nethercote [inactive]]

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!

Comment 8

[Justin Lebar (not reading bugmail)]

Thanks for catching this.  The patch I posted fixed this; I just failed at merging it.  I'll land what you already reviewed.

Comment 9

[Justin Lebar (not reading bugmail)]

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

Comment 10

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/7454890376fc