1313686 - Add opt-out Telemetry probe to see how long it takes for TabChild::RecvSetDocShellIsActive to paint a tab

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

As part of our ongoing efforts to improve tab switch time, we're hoping to measure how long it takes for the content process to fully paint content after a tab switch.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Note that this probe is just about painting, and shouldn't include time to vsync or composite.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1313686 - Add opt-out Telemetry probe to see how long it takes for TabChild::RecvSetDocShellIsActive to paint a tab. data-review=liuche,

Review commit: https://reviewboard.mozilla.org/r/89330/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89330/

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8805572 [details]
Bug 1313686 - Add opt-out Telemetry probe to see how long it takes for TabChild::RecvSetDocShellIsActive to paint a tab. data-review=liuche,

https://reviewboard.mozilla.org/r/89330/#review88516

::: dom/ipc/TabChild.cpp:2628
(Diff revision 1)
>  
>      docShell->SetIsActive(aIsActive);
>    }
>  
>    if (aIsActive) {
> +    Telemetry::AutoTimer<Telemetry::TABCHILD_PAINT_TIME> timer;

Is this the right place for this, billm? Or should I put it down closer to where we suppress the displayport?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8805572 [details]
Bug 1313686 - Add opt-out Telemetry probe to see how long it takes for TabChild::RecvSetDocShellIsActive to paint a tab. data-review=liuche,

https://reviewboard.mozilla.org/r/89330/#review88644

::: dom/ipc/TabChild.cpp:2628
(Diff revision 1)
>  
>      docShell->SetIsActive(aIsActive);
>    }
>  
>    if (aIsActive) {
> +    Telemetry::AutoTimer<Telemetry::TABCHILD_PAINT_TIME> timer;

Yeah, let's put it right before the first SuppressDisplayport call. I don't think the other stuff should matter, but just to be sure. I only want to measure gfx here.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8805572 [details]
Bug 1313686 - Add opt-out Telemetry probe to see how long it takes for TabChild::RecvSetDocShellIsActive to paint a tab. data-review=liuche,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89330/diff/1-2/

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Redirecting to liuche for data-review.

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 8805572 [details]
Bug 1313686 - Add opt-out Telemetry probe to see how long it takes for TabChild::RecvSetDocShellIsActive to paint a tab. data-review=liuche,

https://reviewboard.mozilla.org/r/89330/#review89410

r+ on data with comment addressed

::: toolkit/components/telemetry/Histograms.json:10358
(Diff revision 2)
> +    "bug_numbers": [1313686],
> +    "expires_in_version": "56",
> +    "kind": "exponential",
> +    "high": 1000,
> +    "n_buckets": 50,
> +    "description": "Time spent painting the contents of a remote browser.",

Nit: add the unit of time (e.g., milliseconds)

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8805572 [details]
Bug 1313686 - Add opt-out Telemetry probe to see how long it takes for TabChild::RecvSetDocShellIsActive to paint a tab. data-review=liuche,

https://reviewboard.mozilla.org/r/89330/#review89410

Thanks!

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8805572 [details]
Bug 1313686 - Add opt-out Telemetry probe to see how long it takes for TabChild::RecvSetDocShellIsActive to paint a tab. data-review=liuche,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89330/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71ddd5bf19ec
Add opt-out Telemetry probe to see how long it takes for TabChild::RecvSetDocShellIsActive to paint a tab. data-review=liuche, r=billm,liuche

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/71ddd5bf19ec

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8805572 [details]
Bug 1313686 - Add opt-out Telemetry probe to see how long it takes for TabChild::RecvSetDocShellIsActive to paint a tab. data-review=liuche,

Approval Request Comment
[Feature/regressing bug #]:

None.

[User impact if declined]:

None, but we get less data on how long it takes to paint the tab content with e10s enabled.

[Describe test coverage new/current, TreeHerder]:

None, but I've confirmed locally that the probe is working properly. A bug to add a regression test was filed at bug 1314772.

[Risks and why]: 

Extremely low risk.

[String/UUID change made/needed]:

None.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8805572 [details]
Bug 1313686 - Add opt-out Telemetry probe to see how long it takes for TabChild::RecvSetDocShellIsActive to paint a tab. data-review=liuche,

Ideally it is too late for such changes to be uplifted to Beta50. However, as we increase e10s rollout, we need more usage data and use that data to improve end-user experience. Aurora51+, Beta50+

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm hitting conflicts uplifting this to beta/release. Could we get a rebased patch?

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/12e5c5d18fb3

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Hm. TabChild::RecvSetDocShellIsActive is different enough on beta / release that I'm less confident in applying the change there.

I'll want to talk to billm to see if there's a reasonable place to put the probe, but I think this can safely slip to a point release.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Hey billm, do you know if this is still a sensible place to put the probe?: https://dxr.mozilla.org/mozilla-beta/rev/24b8f08f77565f859898b45f62d2132ccc64c6d8/dom/ipc/TabChild.cpp#2641

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

No, unfortunately that's not a good place. I think we end up painting in a separate turn of the event loop there. I think it's fine not to have this probe on beta.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Okay, marking WONTFIX for 50 given comment 18.