1285006 - Remove Telemetry Probe IMAGE_MAX_DECODE_COUNT

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Chris H-C :chutten]

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)

Comment 1

[Georg Fritzsche [:gfritzsche]]

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.

Comment 2

[Chris H-C :chutten]

+seth

Does this sound okay?

Comment 3

[Chris H-C :chutten]

Attachment: 0001-bug-1285006-Remove-IMAGE_MAX_DECODE_COUNT-Telemetry.patch

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.

Comment 4

[Georg Fritzsche [: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.

Comment 5

[Seth Fowler [:seth] [:s2h]]

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

Comment 6

[Chris H-C :chutten]

Attachment: 0001-bug-1285006-Remove-IMAGE_MAX_DECODE_COUNT-Telemetry-.patch

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/385feddefee4
Remove IMAGE_MAX_DECODE_COUNT Telemetry r=gfritzsche, seth

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/385feddefee4