1689156 - Slow tab switching on pages with long-running requestAnimationFrame handlers, like Slack

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P2)

Description

[Markus Stange [:mstange]]

I noticed long tab switch times when switching to my Slack tab.

It turns out Slack is running a lot of work (>= 1 second) during a requestAnimationFrame callback.
Our RenderLayers IPC message for the tab switch paint triggers a refresh tick. This is new behavior as of bug 1669239; in the past, it would only do the paint and not run the other phases of the refresh tick:
BrowserChild::RecvRenderLayers calls PuppetWidget::PaintNowIfNeeded, which has changed from doing just the paint to doing a full refresh tick.
So now every tab switch has the potential to run a lot more JS before the paint happens. This also applies to "PaintWhileInterruptingJS" paint, as far as I can tell. (Does this mean there's a potential to run rAF callbacks before other JS work has finished? Not sure.)

I've attached a testcase which demonstrates the tab spinner.

Comment 1

[Markus Stange [:mstange]]

Attachment: testcase

Comment 2

[Markus Stange [:mstange]]

I'm trying to find out if this regression was captured in telemetry.
Bug 1669239 landed on October 16, 2020.
Last build without: 20201015215335
First build with: 20201016094031

So far I can not find a "number of tab spinners" probe, or a "percentage of tab switches that showed a spinner" probe.

But I did find the following:

• On the fx_tab_switch_spinner_visible_trigger probe, onLayersReady went up a lot. But onEndSwapDocShells went down by about the same amount. I'm not sure what that means.
• On this dashboard: https://mikeconley.github.io/bug1310250/ the "Short Severity %" graph shows an increased percentage of "0ms - 49ms" spinners. I'm not sure what this percentage is relative to.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

The regressing bug was pretty much a correctness fix though... The HTML spec and such assumes that painting to the screen is associated effectively with a full refresh driver tick...

I guess we could try to revert the regressing bug and somehow find another way to keep reporting correct performance paint timing metrics to content or what not.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Did we effectively break the explicit code we have for fast painting when switching tabs.
It on purpose didn't run any JS.

Comment 5

[Markus Stange [:mstange]]

Yes, I think so.

Comment 6

[Sean Feng [:sefeng]]

Olli and I chatted about this.

We have 2 options.

1. We could bring the old code back and manually schedule a paint, however, the FCP we exposed isn't going to be the real FCP.
2. We could bring the old code back and allow FCP to be fired outside of ticks. Although this is going to expose the magic paint that we did for tab switches.

We think option 2 is the one we should go.

NI myself for making this change.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I thought we would create the entry for FCP at right time but update its timestamp when a real tick happens. That way web content wouldn't know about the magical tab switch paint.

Comment 8

[Sean Feng [:sefeng]]

Yeah, you are right. I think I forgot to say that we planned to tweak Option 2 by what you described.

Comment 9

[Sean Feng [:sefeng]]

Attachment: Bug 1689156 - Bring the fast tab switch paint back

In bug 1669239, we removed the magic paint which would occur during tab
switches, and instead we requested a tick. However, this approach
didn't work well because tick was a heavy operation which did not
only paints but also many other stuff, such as running
requestAnimationFrame handlers, so it made the tab switches slower
under certain conditions.

So here we reverts the changes we made in bug 1669239.

Comment 10

[Sean Feng [:sefeng]]

Attachment: Bug 1689156 - Don't generate first-contentful-paint if the paint doesn't come from Tick

When running Geckoview headless tests, sometimes the only paint that the tests could get
was the very first paint during tab switches, however, this paint didn't come from tick,
so we didn't notify this paint before this patch.

This patch makes Geckoview starts to aware this paint.

Depends on D107858

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9bd3506f2809
Bring the fast tab switch paint back r=mstange,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/40475e86e341
Don't generate first-contentful-paint if the paint doesn't come from Tick r=smaug

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9bd3506f2809
https://hg.mozilla.org/mozilla-central/rev/40475e86e341