1109654 - tab switching distinctly slower when one tab (esp treeherder) has a focused link

Status: RESOLVED DUPLICATE of bug 1131371

(Firefox :: Tabbed Browser, defect)

Description

[Luke Wagner [:luke]]

It seems like having a link focused (dotted rectangle) in a tab can make it visibly slower to switch to that tab.  To see this:
 1. open bugzilla and treeherder tabs
 2. switch back and forth between tabs rapidly; it's usually instant
 3. focus on a link in the treeherder tab (by tabbing or clicking) so it has the little dotted rectangle
 4. switch back and forth between tabs rapidly; it's much slower when switching to treeherder

If I was to guess, I'd say that we're blocking drawing of the new tab based on some traversal of the page's DOM.

To wit, e10s shows different behavior: first we display the new page without any focus-rectangle, then a few hundred ms later we draw the focus-rectangle.  CC'ing billm just in case this is interesting.

Comment 1

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

Wow, great find. I can reproduce this.

Comment 2

[Chris Peterson [:cpeterson]]

M6 for investigation and then consider scheduling fix for a later milestone.

Comment 3

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

A profile would definitely be good here - even better, if we use nsIProfiler AddMarker to add a marker at the entrance of that method, and another marker at the entrance of _finalizeTabSwitch, that'd make it easier to read profiles for tab switching.

Comment 4

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

I did a little profiling today, mostly for fun. It looks like we're doing reflow when the link is focused and otherwise we're not.

Matt, is there any reason for this? Why would we ever do reflow when switching tabs? The reflow happens in a stack like this:
  nsRefreshDriver::Tick
    PresShell::Flush (Flush_InterruptibleLayout)
      PresShell::DoReflow (<treeherder URL>)

Also, to reproduce this, you have to switch back and forth by pressing Ctrl+PgUp/PgDown. However, if you just hold the keys down, you won't see it. Instead, switch quickly back and forth by tapping your fingers quickly, like playing a video game. The time to switch will be noticeably longer if the link is focused.

Comment 5

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

I poked around in the profiler a little more, but it's difficult to find what's causing the reflow. The "Frames" timeline seems like it should do this, but it looks like it only includes information from the parent process. Benoit, is this something that would be easy to fix?

Comment 6

[Benoit Girard (:BenWa)]

Done:
https://github.com/bgirard/cleopatra/commit/3761a7249d9bf17f6873d6c875bcdf9c61e663e2

Comment 7

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

Thanks Benoit!

Comment 8

[Benoit Girard (:BenWa)]

(In reply to Bill McCloskey (:billm) from comment #7)
> Thanks Benoit!

You'll find a dropdown on the left hand side. I think the initial value may not reflect what's selected but after its changed it should be fine. Let me know how well it works.

Comment 9

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

I can now see the long reflow in the Frames view. Unfortunately, when I mouse over it, I just see this stack:

RestyleManager::ProcessRestyledFrames
RestyleTracker::ProcessRestyles
PresShell::Flush (Flush_Style)
nsRefreshDriver::Tick
Timer::Fire

I don't know what to make of that. Any help would be much appreciated.

Comment 10

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

Also, this reflow is 174ms, so it's definitely the main source of lag. However, rasterization also takes 35ms, which sounds slow to me. Maybe that's just Linux though.

Comment 11

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

Attachment: stack

Made a little more progress. I set a breakpoint in RestyleTracker::AddPendingRestyle and found the stack that seems to be triggering the reflow. I've attached it. The relevant part is this:

#8  0x00007fc6afbdbb97 in nsFocusManager::Focus (this=this@entry=0x7fc6a59e9c80, 
    aWindow=<optimized out>, aContent=0x7fc694765390, aFlags=aFlags@entry=0, 
    aIsNewDocument=aIsNewDocument@entry=true, aFocusChanged=aFocusChanged@entry=false, 
    aWindowRaised=false, aAdjustWidgets=true)
    at /home/billm/moz/in3/dom/base/nsFocusManager.cpp:1881
#9  0x00007fc6afbdcc44 in nsFocusManager::WindowRaised (this=0x7fc6a59e9c80, 
    aWindow=<optimized out>) at /home/billm/moz/in3/dom/base/nsFocusManager.cpp:741
#10 0x00007fc6b08d2d9a in nsWebBrowser::Activate (this=<optimized out>)
    at /home/billm/moz/in3/embedding/browser/nsWebBrowser.cpp:1738
#11 0x00007fc6b039ed5f in mozilla::dom::TabChild::RecvActivate (this=<optimized out>)
    at /home/billm/moz/in3/dom/ipc/TabChild.cpp:2247

If I comment out the Activate() call in frame 11, then the performance of tab switching is much better. However, we don't get the focus ring.

I also tested without e10s and tab switching in this situation is also janky, probably for the same reason. Somehow it's more noticeable in e10s, maybe because the tab switches before the content switches.

Olli, do you have any ideas here (or anyone to pass the needinfo? to)? It's really unfortunate that a focus ring is causing 174ms of jank when switching tabs. It also sucks that restyling is seemingly so inefficient, but that sounds like a harder problem to fix.

Comment 12

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

Where exactly are we triggering the reflow? http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp?rev=7ed611fb2797&mark=1773-1775#1740 perhaps?
(CheckIfFocusable flushes)
That looks like rather valid case for flushing.
Could we perhaps focus the tab asynchronously and when the asynchronous thing runs, we re-check that
the tab is still focused one before doing anything?



Heycam might have some ideas for the restyle performance.

Comment 13

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

I was working off revision 28fdae830289 if you need to match up line numbers in the attached stack (except TabChild.cpp might be off since I have changes to that).

The restyle is triggered from here (NotifyFocusStateChange):
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp?rev=7ed611fb2797&mark=1877-1877#1860
I don't know anything about layout, but it seems like restyling somehow triggers reflow. That somehow seems to happen through the stack in comment 9.

I guess the main question I'd like answered is why we keep having to re-do the reflow. Once the focus ring is shown, there doesn't seem to be any point in hiding it and then showing it again each time we switch away and then back to the tab.

Comment 14

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

Well, of course refreshtimer tick flushes. That is what the tick is about.
(Are you using pseudostacks in gecko profiler? they tend to be rather useless)


Web pages can detect whether something is focused by using the state, so I'd guess we have to
change the state when hiding a tab.

Comment 15

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

dbaron might have a comment why reflow takes so much time in this case.


The odd thing is that we do reflowing too often.
RefreshDriver::Tick reflows as expected, but based on the profile we do reflowing also under
PuppetWidget::Paint
 -mozilla::widget::PuppetWidget::Paint()                                           100.0%              0.0%             544B                    libxul.so   
 | -nsView::WillPaintWindow(nsIWidget*)                                            100.0%              0.0%              80B                    libxul.so   
 | | -nsViewManager::WillPaintWindow(nsIWidget*)                                   100.0%              0.0%             240B                    libxul.so   
 | | | -nsViewManager::CallWillPaintOnObservers()                                   89.1%              0.0%             160B                    libxul.so   
 | | | | -PresShell::FlushPendingNotifications(mozilla::ChangesToFlush)             89.1%              0.0%           1.14KB                    libxul.so   


(That 100% is 39% of the total time I'm seeing here.)

Comment 16

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

So I guess we should (1) set the focus on the tab early (even before it is actually painted) 
and (2) possibly remove http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=bf25101e66cf&mark=8831-8835#8811
(That flush predates refreshdriver.)

Comment 17

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

(In reply to Olli Pettay [:smaug] from comment #16)
(2) possibly remove
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.
> cpp?rev=bf25101e66cf&mark=8831-8835#8811
Or maybe not this, but PuppetWidget::Invalidate should use refreshdriver, not randomly dispatch a runnable to the event queue.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So is part of what's happening that :focus styles for focused elements inside the tab go away when we switch tabs away from the tab (because the stuff inside the tab is no longer focused) and then when we switch tabs back, it's suddenly focused again, and we restyle again?  In other words,we're applying the tree-like nature of focus in a way that causes restyling on tab switching when there's something focused inside a tab?  If so, that's pretty bad, and it seems like we should stop doing that, and just keep the focus style when the tab is hidden.  (That seems like the problem described directly in this bug.)

As to why we reflow again during paint -- worth seeing what marked something dirty between the other reflow and painting.  A breakpoint on PresShell::FrameNeedsReflow should answer that.  (Or was there no other reflow?)

As to why the reflow on this page in particular is slow:  again, that requires closer examination.

It seems like these are two or three different bugs; this bug report seems most directly related to the first, and the other two should be split off into separate bug reports, I think.

Comment 19

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

I'm going to resolve this since it was basically fixed by bug 1131371. Also filed bug 1139995 for the issue Olli mentioned in comment 17.