Closed Bug 1313686 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Content Processes, defect)

50 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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: nobody → mconley
Note that this probe is just about painting, and shouldn't include time to vsync or composite.
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+
Attachment #8805572 - Flags: review?(benjamin)
Attachment #8805572 - Flags: review?(benjamin) → review?(liuche)
Redirecting to liuche for data-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+
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!
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
Component: Graphics → DOM: Content Processes
Blocks: 1314772
https://hg.mozilla.org/mozilla-central/rev/71ddd5bf19ec
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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+
I'm hitting conflicts uplifting this to beta/release. Could we get a rebased patch?
Flags: needinfo?(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.
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)
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)
Okay, marking WONTFIX for 50 given comment 18.
You need to log in before you can comment on or make changes to this bug.