625454 - Resizing causes us to spend a lot of time in -[NSView _getNextResizeEventInvalidatingLiveResizeCacheIfNecessary:]

Status: RESOLVED WORKSFORME

(Core :: Widget: Cocoa, defect)

Description

[Patrick Walton (:pcwalton)]

When I resize the window rapidly, Firefox spends a lot of time processing events:

	0.0%	66.5%	AppKit	           -[NSTitledFrame resizeWithEvent:]
	0.0%	56.1%	AppKit	            -[NSWindow _setFrameCommon:display:stashSize:]
	0.0%	9.6%	AppKit	            -[NSView _getNextResizeEventInvalidatingLiveResizeCacheIfNecessary:]

Our frame rate drops to around 35 fps during this time.

Chrome doesn't do this: it spends essentially its time in the drawing code. This makes Chrome feel a lot faster when resizing. See this profile:

	0.0%	66.9%	AppKit	                -[NSWindow setFrame:display:]
	0.0%	1.2%	Foundation	                -[NSAutoreleasePool drain]
	0.0%	0.9%	AppKit	                -[NSView _getNextResizeEventInvalidatingLiveResizeCacheIfNecessary:]

Comment 1

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

So the problem here seems to be that -[NSView _getNextResizeEventInvalidatingLiveResizeCacheIfNecessary:] does, as part of some internal optimization (I guess?), this:

[NSApp nextEventMatchingMask:NSLeftMouseUp |
NSLeftMouseDragged untilDate:[NSDate distantFuture] inMode:NSEventTrackingRunLoopMode dequeue:YES];

Notice the NSEventTrackingRunLoopMode. It's trying to get back only mouse-move or mouse-up events. Unfortunately, since our Gecko event source registers for kCFRunLoopCommonModes, it gets called here, and we do all sorts of event processing during performance-critical code inside Cocoa. The fix seems to be to register our event source in the kCFRunLoopDefaultMode only, so that it won't fire here and Cocoa can quickly return to processing the resize.

Comment 2

[Patrick Walton (:pcwalton)]

Regression with this patch: Animations and transitions no longer occur while resizing (basically, we're starved for events). Neither Chrome nor Safari do this.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

> Regression with this patch: Animations and transitions no longer
> occur while resizing (basically, we're starved for events). Neither
> Chrome nor Safari do this.

This is exactly what I would have expected.  So your patch probably
isn't acceptable on its own.

I'm not sure what additional work (in addition to your patch) would be
needed to avoid this ... or whether it would be possible to avoid it
and still improve event-handling performance (over what it is now).

The basic problem (and this is true on all platforms) is that we have
(in effect) two event loops -- a Gecko one and a "native" one.  So
either one can starve the other, and we need to be careful to avoid
this (much of the code in nsBaseAppShell.cpp is devoted to this
purpose).

Safari and Chrome (like all WebKit browsers) somehow get away with
having a single event loop (or at least on OS X they do).  Which is
probably good for performance.  But it would be difficult for us (with
our much greater commitment to working on multiple platforms) to do
the same.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

What with all the weird bugs I've been dealing with, and my work to
make my Java Embedding Plugin compatible with FF4, I've had very
little time to spend looking for OS-X performance enhancements.

But I suspect better performance can be wrung out of the OS X appshell
code, and I really would like to spend a couple of weeks trying to do
that.  This will (of course) only be feasible after the FF4 release.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> couple of weeks

Let's be more realistic -- a couple of months :-)

Comment 6

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 2.

How about this? This one only blocks native events if we're in event tracking mode. Most of the performance problems are due to calling -[NSApplication nextEventMatchingMask:] inside the nested event loop. With this patch, the performance is better, and animations and JS still run during resize.

Comment 7

[Markus Stange [:mstange]]

The patch looks good to me, except that alwaysBlockNative should be aAlwaysBlockNative.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 503683 [details] [diff] [review]
Proposed patch, version 2.

This looks more promising than your previous patch, but I need to
spend several hours with it.  I should probably have time Monday or
Tuesday of next week.

How, precisely, have you been measuring performance?  (For example,
how did you get your figures from comment #0?)  I want to be able to
reproduce your results, and do my own testing.

Comment 9

[Patrick Walton (:pcwalton)]

(In reply to comment #8)
> Comment on attachment 503683 [details] [diff] [review]
> Proposed patch, version 2.
> 
> This looks more promising than your previous patch, but I need to
> spend several hours with it.  I should probably have time Monday or
> Tuesday of next week.
> 
> How, precisely, have you been measuring performance?  (For example,
> how did you get your figures from comment #0?)  I want to be able to
> reproduce your results, and do my own testing.

My frame rate monitor here: https://github.com/pcwalton/firefox-framerate-monitor

Specifically, I've found the Shark integration useful for finding these things.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 503683 [details] [diff] [review]
Proposed patch, version 2.

I've now done at least basic testing on this patch.  What I've found
indicates that it doesn't do any significant harm, and that (under
certain circumstances) it delivers a significant performance
improvement -- perhaps about 10%.

My performance tests were all done in Shark.  I'll have more to say
about them in a later comment.

I wasn't able to get usable results with Patrick's framerate monitor
(possibly because I don't fully understand what it does, or how to use
it):  When testing with the Starfield animation at
http://arapehlivanian.com/wp-content/uploads/2007/02/canvas.html, I
saw the framerate actually increase when I started resizing the
browser window!

My other tests showed that animations can be blocked during window
resize.  But this happens with or without the patch, and also happens
in Safari.  More about this in a later comment.

> alwaysBlockNative should be aAlwaysBlockNative

I agree with Markus.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Attachment: Shark test results

A detailed description of how I got these results is in the
attachment.

-[NSView _getNextResizeEventInvalidatingLiveResizeCacheIfNecessary:]
is only ever called from -[NSTitledFrame resizeWithEvent:].  So any
savings due to changing the frequency of calls to the former should be
directly reflected in performance statistics for the latter.
Furthermore, calls with -[NSTitledFrame resizeWithEvent:] on the stack
seem to be responsible for a large percentage of the "work" done by
the browser (between about 75% and about 85% in my tests).  So we
should get much more accurate results by looking at the statistics for
-[NSTitledFrame resizeWithEvent:] than we would be looking at the
statistics for -[NSView
_getNextResizeEventInvalidatingLiveResizeCacheIfNecessary:].

So we need to look at the results for -[NSTitledFrame
resizeWithEvent:].

Note that the v2 patch only shows a significant performance
improvement for the about:home page.  It actually results in a small
performance regression with the Starfield canvas animation at
http://arapehlivanian.com/wp-content/uploads/2007/02/canvas.html
(probably too small to be significant).  And only a small performance
improvement with the very noisy and obnoxious page at
http://www.vg.no/ (full of Flash objects) -- once again probably too
small to be significant.

So it would seem that the v2 patch only helps with very simple pages
(like the about:home page), and not at all with complex pages.  Which
is exactly the opposite of what I would have expected ... but that's
what my tests show.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

I forgot to mention that my tests were done with "universal" (32-bit
and 64-bit) distros made from current code -- with and without the v2
patch.  These distros had optimization, debugging and symbol-stripping
turned off (--disable-optimize --disable-debug --disable-strip
--disable-install-strip).

All my tests were done on OS X 10.6.5 in 64-bit mode.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Testing with the following animation examples, I find that their
animation stops whenever I click and hold the resize "button" (at the
bottom right-hand corner of the browser window).

http://arapehlivanian.com/wp-content/uploads/2007/02/canvas.html
https://developer.mozilla.org/samples/svg/svganimdemo1.html
http://devedge-temp.mozilla.org/toolbox/examples/2001/stock-ticker/

The animation restarts when I move the resize button (and the window
resizes), but stops again when I stop moving it (without letting go of
the mouse).

This happens with and without the patch, and also in Safari.  So I
assume this is some kind of bug or design flaw in OS X itself.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

> So I assume this is some kind of bug or design flaw in OS X itself.

Or maybe it's not a bug but a feature :-)

Comment 15

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 3.

Glad to see the promising performance results, thanks a lot! :)

Patch version 3 renames alwaysBlockNative to aAlwaysBlockNative. Asking for approval for this window resize performance win.

Comment 16

[Patrick Walton (:pcwalton)]

Regarding the request for approval: The risk for this patch seems moderate to me. There's a possibility that we could be starved for events during resizing. I haven't noticed any regressions myself, however.

The performance benefit, as stated above, is biggest for simpler pages (like google.com, where I first found the issue when testing against Chrome). I'd suspect that the reason for this is that resize performance is going to hurt on big pages anyway, and on small pages, our time spent processing native events tends to dominate.

Comment 17

[LegNeato]

Comment on attachment 505210 [details] [diff] [review]
Proposed patch, version 3.

Let's get this in for beta 10 and back it out if there are issues.

Comment 18

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/84ed248b728d

Comment 19

[Patrick Walton (:pcwalton)]

Backed out:

http://hg.mozilla.org/mozilla-central/rev/a7b806aebf09

See bug 629324.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 505210 [details] [diff] [review]
Proposed patch, version 3.

minusing approval due to backout

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:spohl, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 22

[Stephen A Pohl [:spohl]]

It seems that this has stalled and was not pursued further after the backout. Markus, should this be revived, or shall we close as wontfix?

Comment 23

[Markus Stange [:mstange]]

I think this is working well these days. Especially the switch to CoreAnimation greatly improved resize performance; it removed a WindowServer synchronization point between the OpenGL surface and the regular window surface.