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

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks: 2 bugs)

50 Branch
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 wontfix, firefox51 fixed, firefox52 fixed)

Details

Attachments

(1 attachment)

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

Updated

2 years ago
Assignee: nobody → mconley
(Assignee)

Comment 1

2 years ago
Note that this probe is just about painting, and shouldn't include time to vsync or composite.
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 years ago
mozreview-review
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 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.
Attachment #8805572 - Flags: review?(wmccloskey) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8805572 - Flags: review?(benjamin)
(Assignee)

Updated

2 years ago
Attachment #8805572 - Flags: review?(benjamin) → review?(liuche)
(Assignee)

Comment 6

2 years ago
Redirecting to liuche for data-review.

Comment 7

2 years ago
mozreview-review
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)
Attachment #8805572 - Flags: review?(liuche) → review+
(Assignee)

Comment 8

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 10

2 years ago
Pushed by mconley@mozilla.com:
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
(Assignee)

Updated

2 years ago
Component: Graphics → DOM: Content Processes
(Assignee)

Updated

2 years ago
Blocks: 1314772

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/71ddd5bf19ec
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 12

2 years ago
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.
Attachment #8805572 - Flags: approval-mozilla-beta?
Attachment #8805572 - Flags: approval-mozilla-aurora?
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+
Attachment #8805572 - Flags: approval-mozilla-beta?
Attachment #8805572 - Flags: approval-mozilla-beta+
Attachment #8805572 - Flags: approval-mozilla-aurora?
Attachment #8805572 - Flags: approval-mozilla-aurora+
status-firefox50: --- → affected
status-firefox51: --- → affected
I'm hitting conflicts uplifting this to beta/release. Could we get a rebased patch?
Flags: needinfo?(mconley)

Comment 15

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/12e5c5d18fb3
status-firefox51: affected → fixed
(Assignee)

Comment 16

2 years ago
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.
(Assignee)

Comment 17

2 years ago
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
Flags: needinfo?(mconley)
(Assignee)

Updated

2 years ago
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 19

2 years ago
Okay, marking WONTFIX for 50 given comment 18.
status-firefox50: affected → wontfix
You need to log in before you can comment on or make changes to this bug.