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

RESOLVED INCOMPLETE

Status

()

RESOLVED INCOMPLETE
8 years ago
2 years ago

People

(Reporter: pcwalton, Assigned: pcwalton)

Tracking

(Blocks: 2 bugs, {perf})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [frame-rate-monitor])

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
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).
(Assignee)

Comment 1

8 years ago
Created attachment 505160 [details] [diff] [review]
Proposed patch.

Proposed patch attached, baking on try server now.
Attachment #505160 - Flags: feedback?(fryn)
(Assignee)

Updated

8 years ago
Whiteboard: [frame-rate-monitor]
(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.
(Assignee)

Comment 3

8 years ago
(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.
(Assignee)

Comment 4

8 years ago
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.)
(Assignee)

Updated

8 years ago
Summary: Tab close animation shouldn't run at the same time as the tab is unloaded → Tab close animation shouldn't run simultaneously with switching to an existing tab
(Assignee)

Updated

8 years ago
Attachment #505160 - Flags: review?(gavin.sharp)

Comment 5

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

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

Comment 6

8 years ago
(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

8 years ago
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.
Attachment #505160 - Flags: feedback?(fryn)
(Assignee)

Comment 8

8 years ago
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.
(Assignee)

Comment 9

8 years ago
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 11

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

Okay, I'd suggest talking to Gavin then.

Comment 12

8 years ago
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.
(Assignee)

Comment 13

8 years ago
(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 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.
Attachment #505160 - Flags: review?(gavin.sharp)
Blocks: 789573
Blocks: 593680

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.