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

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM: Content Processes
RESOLVED FIXED
8 months ago
7 months 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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
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

8 months ago
Assignee: nobody → mconley
(Assignee)

Comment 1

8 months 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

8 months 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 4

8 months 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/#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

8 months ago
Attachment #8805572 - Flags: review?(benjamin)
(Assignee)

Updated

8 months ago
Attachment #8805572 - Flags: review?(benjamin) → review?(liuche)
(Assignee)

Comment 6

8 months ago
Redirecting to liuche for data-review.

Comment 7

8 months 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

8 months 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

8 months 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

8 months ago
Component: Graphics → DOM: Content Processes
(Assignee)

Updated

8 months ago
Blocks: 1314772

Comment 11

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

Comment 12

8 months 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 13

8 months 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,

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+

Updated

8 months ago
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

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

Comment 16

8 months 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

8 months 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

8 months 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

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