627159 - Tab close animation shouldn't run simultaneously with switching to an existing tab

Status: RESOLVED INCOMPLETE

(Firefox :: Tabbed Browser, defect)

Description

[Patrick Walton (:pcwalton)]

The tab close animation is often janky, especially on low-end computers, because the tab is being unloaded at the same time the animation goes off. If we wait for a little bit before running the animation, it often goes smoother for me (on my Linux netbook).

Comment 1

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

Proposed patch attached, baking on try server now.

Comment 2

[Dão Gottwald [:dao]]

(In reply to comment #0)
> because the tab is being unloaded at the same time the animation goes off.

That's not true. The page unloads when the animation has finished.

Comment 3

[Patrick Walton (:pcwalton)]

(In reply to comment #2)
> (In reply to comment #0)
> > because the tab is being unloaded at the same time the animation goes off.
> 
> That's not true. The page unloads when the animation has finished.

You're right, sorry. What's slow is the automatic switching of tabs when the currently-open tab is closed.

Comment 4

[Patrick Walton (:pcwalton)]

And this patch helps the perf problems resulting from that. (Ultimately we can't truly fix this until Electrolysis... we just have to hope that no events are going to happen and jank our UI while the animation is going on.)

Comment 5

[Frank Yan (:fryn)]

Comment on attachment 505160 [details] [diff] [review]
Proposed patch.

What code would be running during the delay?
_blurTab finishes before the animation starts.

Comment 6

[Patrick Walton (:pcwalton)]

(In reply to comment #5)
> Comment on attachment 505160 [details] [diff] [review]
> Proposed patch.
> 
> What code would be running during the delay?
> _blurTab finishes before the animation starts.

I believe it's rendering, but I'm not sure; I've only eyeballed it so far. I can profile tomorrow and get back to you.

Comment 7

[Frank Yan (:fryn)]

Comment on attachment 505160 [details] [diff] [review]
Proposed patch.

I don't think I can provide meaningful feedback on this, given that I haven't really experienced this problem, and I don't see how setTimeout could help significantly. If it turns out that TabClose handlers are causing the problem, we should investigate why TabClose handlers are slow.

Comment 8

[Patrick Walton (:pcwalton)]

As I suspected, we spend essentially all our time in the drawing code. The following profile is between when we set the setTimeout(... 0) and when it fires.

	0.0%	59.3%	XUL	         HandleEvent(nsGUIEvent*)
	0.0%	59.3%	XUL	          nsViewManager::DispatchEvent(nsGUIEvent*, nsIView*, nsEventStatus*)
	0.0%	59.3%	XUL	           nsViewManager::Refresh(nsView*, nsIWidget*, nsIntRegion const&, unsigned int)
	0.0%	59.3%	XUL	            nsViewManager::RenderViews(nsView*, nsIWidget*, nsRegion const&, nsIntRegion const&, int, int)
	0.0%	59.3%	XUL	             PresShell::Paint(nsIView*, nsIView*, nsIWidget*, nsRegion const&, nsIntRegion const&, int, int)
	0.0%	59.2%	XUL	              nsLayoutUtils::PaintFrame(nsIRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int)
	0.0%	56.5%	XUL	               nsDisplayList::PaintForFrame(nsDisplayListBuilder*, nsIRenderingContext*, nsIFrame*, unsigned int) const
	0.0%	34.3%	XUL	     nsAppShell::ProcessGeckoEvents(void*)
	0.0%	34.2%	XUL	      nsBaseAppShell::NativeEventCallback(int)
	0.1%	34.2%	XUL	       NS_ProcessPendingEvents_P(nsIThread*, unsigned int)
	0.1%	34.1%	XUL	        nsThread::ProcessNextEvent(int, int*)
	0.0%	26.2%	XUL	         nsAppShell::OnProcessNextEvent(nsIThreadInternal*, int, unsigned int)
	0.3%	26.1%	XUL	          nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, int, unsigned int)
	12.2%	25.8%	XUL	           nsAppShell::ProcessNextNativeEvent(int)
	0.0%	10.2%	XUL	            -[ChildView drawRect:]

Looks like we're painting the tab that you switched to, which competes with the tab close animation. If we wait to paint that tab before running the animation, it should run smoother.

Comment 9

[Patrick Walton (:pcwalton)]

The reason I'm using setTimeout() is that it adds a new callback to the event queue, which won't fire until after the painting is done (since painting is done on our UI thread). This lets us wait to paint the tab before running the animation.

Comment 10

[Patrick Walton (:pcwalton)]

Note the input on this issue: http://input.mozilla.com/en-US/beta/search?product=firefox&q=tab+animation

Comment 11

[Frank Yan (:fryn)]

(In reply to comment #8 and comment #9)

Okay, I'd suggest talking to Gavin then.

Comment 12

[(no longer active)]

IINM, the only thing that this patch does is to enable us hit the event loop one more time.  I can't see how this could have any perceivable improvements in performance.

Comment 13

[Patrick Walton (:pcwalton)]

(In reply to comment #12)
> IINM, the only thing that this patch does is to enable us hit the event loop
> one more time.  I can't see how this could have any perceivable improvements in
> performance.

It makes it more likely that we aren't painting the page at the same time the animation is running. Basically, the idea is to finish the page painting before running the tab close animation.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 505160 [details] [diff] [review]
Proposed patch.

I'm sorry this has been sitting in my queue for so long - the context behind this is far outside of everyone's mind at this point...

I had a hard time making a call on this, partially because I wanted to fully understand the technical reasoning behind why we think this will help, without being very familiar with how rendering/painting works (particularly when animations are involved).

Beyond that, additional experimental evidence of this being a win would be useful - "it often goes smoother for me (on my Linux netbook)" is useful, but it's also a single (anecdotal) data point.