Closed Bug 825519 Opened 7 years ago Closed 7 years ago

Add telemetry counting max(# times this image was decoded)

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Details

Attachments

(1 file)

We've now had two bugs where we enter into a loop decoding an image over and over again.

So it seems like it would be worthwhile to add telemetry which would let us see how many people are affected by this bug.  That way we can verify our eventual fix, and we can verify that it doesn't regress.

I'd probably want to report both

 1) # times this image was decoded (for all images), and
 2) max_over_all_images(# times this image was decoded).

(2) is what we need to see if we've hit this decode-infinitely bug, and (1) is simple to do once we have (2) and is interesting for tuning discard heuristics.
Oh, we already have (2).  So this is even easier.
Attached patch Patch, v1Splinter Review
Attachment #697107 - Flags: review?(joe)
Assignee: nobody → justin.lebar+bug
Comment on attachment 697107 [details] [diff] [review]
Patch, v1

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

::: image/src/RasterImage.cpp
@@ +2581,5 @@
> +    if (mDecodeCount > sMaxDecodeCount) {
> +      // Don't subtract out 0 from the histogram, because that causes its count
> +      // to go negative, which is not kosher.
> +      if (sMaxDecodeCount > 0) {
> +        Telemetry::GetHistogramById(Telemetry::IMAGE_MAX_DECODE_COUNT)->Subtract(sMaxDecodeCount);

It'd be nice if this underlying bug could be fixed.
Attachment #697107 - Flags: review?(joe) → review+
> It'd be nice if this underlying bug could be fixed.

It's merely an assertion, I think.  I guess to fix the bug we could have a "remove and add" function, but we'd still need to call the vanilla "add" function the first time...  :-/
I think from a telemetry perspective, you might be better off exposing this as a single number in the ping's simpleMeasurements or similar, since that's really what it is.  Slightly more complicated, though. :(  I'm not sure if the data collection portion will DTRT with a histogram whose buckets are not monotonically increasing over time...
> you might be better off exposing this as a single number in the ping's simpleMeasurements 
> or similar, since that's really what it is. 

I agree that would be better, but it's complicated, like you say.  :)

I was told that the data collection back-end deletes all old data for a session when a new ping for that session arrives.  If it doesn't DTRT with this patch, it's basically impossible to use Histogram::Subtract correctly, which is going to be a much more general problem...
I went ahead and landed this.  If the data collection is broked, that would be useful to discover!
cc1plus: warnings being treated as errors
../../../image/src/RasterImage.cpp: In member function 'nsresult mozilla::image::RasterImage::InitDecoder(bool)':
../../../image/src/RasterImage.cpp:2581:24: error: comparison between signed and unsigned integer expressions

le sigh.
https://hg.mozilla.org/mozilla-central/rev/ecd2331e6798
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.