Last Comment Bug 672731 - Add UNITS_COUNT_CUMULATIVE to nsIMemoryReporter
: Add UNITS_COUNT_CUMULATIVE to nsIMemoryReporter
Status: RESOLVED FIXED
[MemShrink]
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla8
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 672694
Blocks: 673837
  Show dependency treegraph
 
Reported: 2011-07-19 23:33 PDT by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2011-07-26 11:08 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Add UNITS_COUNT_CUMULATIVE. (8.44 KB, patch)
2011-07-20 13:41 PDT, Justin Lebar (not reading bugmail)
n.nethercote: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2011-07-19 23:33:21 PDT
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) 2011-07-20 05:58:32 PDT
Adding a new unit is the right thing to do, I think.
Comment 2 Justin Lebar (not reading bugmail) 2011-07-20 13:41:52 PDT
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.
Comment 3 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-07-21 19:56:26 PDT
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) 2011-07-24 20:45:39 PDT
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) 2011-07-24 21:00:03 PDT
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] 2011-07-25 06:18:05 PDT
http://hg.mozilla.org/mozilla-central/rev/9b1a8e3b99aa
Comment 7 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-07-25 23:11:03 PDT
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) 2011-07-26 06:18:30 PDT
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) 2011-07-26 06:21:41 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/7454890376fc
Comment 10 :Ehsan Akhgari (out sick) 2011-07-26 11:08:30 PDT
http://hg.mozilla.org/mozilla-central/rev/7454890376fc

Note You need to log in before you can comment on or make changes to this bug.