Closed Bug 1285006 Opened 3 years ago Closed 3 years ago

Remove Telemetry Probe IMAGE_MAX_DECODE_COUNT

Categories

(Core :: ImageLib, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: chutten|PTO, Assigned: chutten|PTO)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file, 1 obsolete file)

The Telemetry probe IMAGE_MAX_DECODE_COUNT was set to expire in 40. ( with its brethren CANVAS_WEBGL_USED, IMAGE_DECODE_CHUNKS, IMAGE_DECODE_COUNT, IMAGE_DECODE_LATENCY_US, IMAGE_DECODE_ON_DRAW_LATENCY, IMAGE_DECODE_SPEED_GIF, IMAGE_DECODE_SPEED_PNG, IMAGE_MAX_DECODE_COUNT)

It uses Telemetry::ClearHistogram in the child process which, after bug 1218576 lands, is verboten.

Near as I can tell, this metric is unused. (I suspect some of the others are as well, but it's hard to tell)
Oh, that probe.
Bug 1261063, comment 5 says it might be used.

We could still consider removing it - and potentially add it as a scalar later when its needed.
The scalar API actually has a "set probe to maximum value" function, so that would make this much cleaner.
Priority: -- → P3
Whiteboard: [measurement:client]
+seth

Does this sound okay?
Flags: needinfo?(seth)
This patch gets me up and running with child telemetry aggregation in the parent and is just a straight-up removal of the offending probe.
Assignee: nobody → chutten
Status: NEW → ASSIGNED
Attachment #8770118 - Flags: review?(gfritzsche)
Comment on attachment 8770118 [details] [diff] [review]
0001-bug-1285006-Remove-IMAGE_MAX_DECODE_COUNT-Telemetry.patch

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

This looks fine with me, but we have to check whether the users (seth at al) can live with this.

Seth, this is the only probe with this kind of semantics and it currently blocks our fixing of Telemetry for multiple processes.
If you could live with a temporary interruption of this, we could move forward with that project.

We could implement a similar measure soon with the "set maximum" functionality of the new scalar measurements (bug 1275517).
Alternatively we could track the sMaxDecodeCount in RasterImage.cpp and just record the probe once on shutdown. That would keep the current behavior working and only lose a bit of data where crashes occured.
Attachment #8770118 - Flags: review?(seth)
Attachment #8770118 - Flags: review?(gfritzsche)
Attachment #8770118 - Flags: review+
Comment on attachment 8770118 [details] [diff] [review]
0001-bug-1285006-Remove-IMAGE_MAX_DECODE_COUNT-Telemetry.patch

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

Looks good to me. As discussed on IRC, this is probably the least useful image-related telemetry measure, and I don't see any reason why we'd want to bring it back. (To the extent that it told us anything useful, I think we'd be better off with just a straight histogram of redecode counts.)
Attachment #8770118 - Flags: review?(seth) → review+
Flags: needinfo?(seth)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/385feddefee4
Remove IMAGE_MAX_DECODE_COUNT Telemetry r=gfritzsche, seth
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/385feddefee4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.