Convert the 12 "images" memory reporters to a single multi-reporter

RESOLVED FIXED in mozilla13

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Posted patch patchSplinter Review
This patch converts the 12 "images" single reporters into one
multi-reporter.  This makes the reporting feel a lot more natural and
similar to other cases in the codebase.

It also fixes a telemetry reporter for images that was broken due to a bad
path.  This will be useful when various image-suck bugs are fixed.  I tested
it with the about:telemetry add-on.
Attachment #601842 - Flags: review?(justin.lebar+bug)
You were going to get that REPORT macro in there one way or another, eh?  :)
FWIW I like this one much better because it's file-local and it explicitly sets
UNITS_BYTES.

I'd even be OK with NS_ENSURE_SUCCESS(rv, rv) in this macro, if you wanted to.

In any case please undef REPORT it when you're done with it.

>+// This is used by telemetry.
>+NS_MEMORY_REPORTER_IMPLEMENT(
>+  ImagesContentUsedUncompressed,
>+  "images-content-used-uncompressed",
>+  KIND_OTHER,
>+  UNITS_BYTES,
>+  imgMemoryReporter::GetImagesContentUsedUncompressed,
>+  "This is the sum of the 'explicit/images/content/used/uncompressed-heap' "
>+  "and 'explicit/images/content/used/uncompressed-nonheap' numbers.  However, "
>+  "it is measured at a different time and so may give slightly different "
>+  "results.")

Adding this to KIND_OTHER just for the purposes of telemetry is kind of lame.

As a follow-up, can we either modify telemetry to understand multi-reporters,
or add a KIND_HIDDEN which isn't displayed in about:memory?

Also, whitespace at the end of the line immediately after this.
Attachment #601842 - Flags: review?(justin.lebar+bug) → review+
> I'd even be OK with NS_ENSURE_SUCCESS(rv, rv) in this macro, if you wanted
> to.

/me does a happy dance.


> >+// This is used by telemetry.
> >+NS_MEMORY_REPORTER_IMPLEMENT(
> >+  ImagesContentUsedUncompressed,
> 
> Adding this to KIND_OTHER just for the purposes of telemetry is kind of lame.
> 
> As a follow-up, can we either modify telemetry to understand multi-reporters,

I don't want to do that, because you could end up running a complex multi-reporter and ignoring all but 1 value, which would make the telemetry ping slow.

> or add a KIND_HIDDEN which isn't displayed in about:memory?

I like that better... but I'd prefer if all telemetry-reported numbers could be seen in about:memory.  (Generally I've erred on the side of too much information in about:memory, and one more line isn't going to break the bank.)  How strongly do you feel about it?
https://hg.mozilla.org/mozilla-central/rev/fed586e905ec
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
> > I'd even be OK with NS_ENSURE_SUCCESS(rv, rv) in this macro, if you wanted
> > to.

Just to be clear, the difference between this and what I'd previously thought you were proposing is that the #define is right above where it's used, rather than in a header somewhere.

> I like that better... but I'd prefer if all telemetry-reported numbers could be seen in 
> about:memory.  (Generally I've erred on the side of too much information in about:memory, and one 
> more line isn't going to break the bank.)  How strongly do you feel about it?

Not at all strongly.  But if we start adding many more repetitive reporters to KIND_OTHER for the purposes of telemetry, let's revisit this?
> Just to be clear, the difference between this and what I'd previously
> thought you were proposing is that the #define is right above where it's
> used, rather than in a header somewhere.

Yep.  I'm partway through a patch that converts all the multi-reporters to use local macros of this style, hopefully you'll still approve when it occurs in half a dozen multi-reporters :)


> Not at all strongly.  But if we start adding many more repetitive 
reporters
> to KIND_OTHER for the purposes of telemetry, let's revisit this?

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