Add telemetry for imgLoader::LoadImage

VERIFIED WONTFIX

Status

()

Core
DOM
VERIFIED WONTFIX
9 months ago
8 months ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf-])

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
We should start measuring this function in the wild, since there is a lot going on in there which can be slow depending on the configuration.  For example, bug 1347379 is the kind of thing that makes larger sessions slower since the hash table search will take longer.
(Assignee)

Comment 1

9 months ago
Since the only thing we really care about in practice is image loads in the content process, I'm going to restrict the telemetry to the content process.  This way we won't be affected by XUL image loads, add-ons and whatnot as a bonus.
(Assignee)

Comment 2

9 months ago
Created attachment 8847415 [details] [diff] [review]
Add telemetry for imgLoader::LoadImage()
Attachment #8847415 - Flags: review?(michael)
Attachment #8847415 - Flags: review?(francois)
And what are we going to do with the results? I mean, does the telemetry really tell well enough what is being slow?

(In general I'm a bit worried that we add too many telemetry probes which no one will look at)
Whiteboard: [qf-]
Comment on attachment 8847415 [details] [diff] [review]
Add telemetry for imgLoader::LoadImage()

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

::: toolkit/components/telemetry/Histograms.json
@@ +1457,5 @@
> +    "bug_numbers": [1347376],
> +    "kind": "linear",
> +    "low": 1,
> +    "high": 1000,
> +    "n_buckets": 50,

This is going to produce 1 bucket ~every 20 ms grouping. Is this what we're looking for?
Attachment #8847415 - Flags: review?(michael) → review+
(Assignee)

Comment 5

9 months ago
(In reply to Olli Pettay [:smaug] from comment #3)
> And what are we going to do with the results? I mean, does the telemetry
> really tell well enough what is being slow?

I'm planning to experiment with this one as an example.  I'm going to actively try to bring it down, as much as I can.  I want to get a sense of how practical that is.  Since we are going to soon start getting native stacks we're going to have an incoming set of bugs where we have functions that emerge as being expensive in the wild, and we'll need to figure out if we can do work to bring down the cost.

> (In general I'm a bit worried that we add too many telemetry probes which no
> one will look at)

I will remove this one soon once I'm done with this experiment.  You can tell from me using my personal email address, a practice which I have resisted many times in the past, for this one.  :-)

(In reply to Michael Layzell [:mystor] from comment #4)
> Comment on attachment 8847415 [details] [diff] [review]
> Add telemetry for imgLoader::LoadImage()
> 
> Review of attachment 8847415 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/Histograms.json
> @@ +1457,5 @@
> > +    "bug_numbers": [1347376],
> > +    "kind": "linear",
> > +    "low": 1,
> > +    "high": 1000,
> > +    "n_buckets": 50,
> 
> This is going to produce 1 bucket ~every 20 ms grouping. Is this what we're
> looking for?

I don't want to have too many buckets, and I still don't have a sense of the distribution.  I'll refine the probe after some incoming data comes in as needed.  But it's probably more useful to use exponential on this one to begin with even...  I'll play with the simulator a bit more.
Comment on attachment 8847415 [details] [diff] [review]
Add telemetry for imgLoader::LoadImage()

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

datareview+
Attachment #8847415 - Flags: review?(francois) → review+

Comment 7

9 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3c323635d6
Add telemetry for imgLoader::LoadImage(); r=mystor,francois
https://hg.mozilla.org/mozilla-central/rev/fb3c323635d6
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 9

8 months ago
This telemetry proved to be quite useless in practice.  I'm going to back it out.
(Assignee)

Comment 10

8 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/616974bb80233137522000496f7e240bf2a0eb2b
Status: RESOLVED → VERIFIED
Resolution: FIXED → WONTFIX
You need to log in before you can comment on or make changes to this bug.