Decode time telemetry doesn't account for synchronous decoding

NEW
Unassigned

Status

()

7 years ago
6 years ago

People

(Reporter: rclick, Unassigned)

Tracking

(Depends on: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Currently the telemetry for total decode time only accounts for time spent decoding asynchronously (in imgDecodeWorker::Run()). This should be expanded to include the other places where we write data to the decoder.

Updated

7 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Robert, did you want to work on this? Jeff can probably give you some pointers if you need them.
I've known about this for a while. I'd actually hoped that we would switch to only writing data to the decoder in one place.
(Reporter)

Comment 3

7 years ago
Created attachment 571371 [details] [diff] [review]
patch

This stores the decode time as a member of RasterImage, times how long we spend decoding in SyncDecode() and AddSourceData(), and accumulates the decode time and speed in ShutdownDecoder().

This seems to work, although I'm commonly seeing a decode time of zero. I'm still investigating why that's happening.
(Reporter)

Comment 4

7 years ago
Created attachment 585681 [details] [diff] [review]
patch v2

The zero decode times I was seeing seem to be due to TimeStamp not having enough resolution on my Windows box to accurately time decoding of tiny images. The data isn't very useful in that case, so I've opted to just not accumulate the data if we have a zero decode time.

Assuming that's acceptable, this is ready for review. Jeff, can you look at this?
Attachment #571371 - Attachment is obsolete: true
Attachment #585681 - Flags: review?(jmuizelaar)
(Reporter)

Comment 5

7 years ago
Created attachment 585686 [details] [diff] [review]
patch v2

Oops, forgot to qref. This is the right patch.
Attachment #585681 - Attachment is obsolete: true
Attachment #585681 - Flags: review?(jmuizelaar)
Attachment #585686 - Flags: review?(jmuizelaar)
Comment on attachment 585686 [details] [diff] [review]
patch v2

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

This patch is ok, but it adds more code that's basically doing the same thing in two places. Instead do you think you could look at refactoring the code so that we only write to the decoder in one place and then will only have to do the timing in a single place?

::: image/src/RasterImage.cpp
@@ +2541,5 @@
>    CONTAINER_ENSURE_SUCCESS(rv);
>  
> +  TimeDuration decodeLatency = TimeStamp::Now() - start;
> +  Telemetry::Accumulate(Telemetry::IMAGE_DECODE_LATENCY,
> +                        PRInt32(decodeLatency.ToMicroseconds()));

It looks like this accumulates the DECODE_LATENCY even for size decodes. We don't do this in the other case.
(Reporter)

Comment 7

7 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> This patch is ok, but it adds more code that's basically doing the same
> thing in two places. Instead do you think you could look at refactoring the
> code so that we only write to the decoder in one place and then will only
> have to do the timing in a single place?
Sure. I filed Bug 718927 for that.
Depends on: 718927
Comment on attachment 585686 [details] [diff] [review]
patch v2

Dropping review request because of the refactoring needed for bug 718927
Attachment #585686 - Flags: review?(jmuizelaar)
You need to log in before you can comment on or make changes to this bug.