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

REOPENED
Assigned to

Status

()

defect
REOPENED
9 years ago
8 years ago

People

(Reporter: pcwalton, Assigned: pcwalton)

Tracking

({perf})

unspecified
mozilla2.0b10
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status2.0 ?)

Details

(Whiteboard: [frame-rate-monitor][not-ready])

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

9 years ago
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:]
Assignee

Comment 1

9 years ago
Posted patch Proposed patch. (obsolete) — Splinter Review
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.
Assignee: nobody → pwalton
Status: NEW → ASSIGNED
Attachment #503623 - Flags: review?(smichaud)
Assignee

Comment 2

9 years ago
Regression with this patch: Animations and transitions no longer occur while resizing (basically, we're starved for events). Neither Chrome nor Safari do this.
> 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.
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.
Attachment #503623 - Flags: review?(smichaud) → review-
> couple of weeks

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

Comment 6

9 years ago
Posted patch Proposed patch, version 2. (obsolete) — Splinter Review
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.
Attachment #503623 - Attachment is obsolete: true
Attachment #503683 - Flags: review?(smichaud)
The patch looks good to me, except that alwaysBlockNative should be aAlwaysBlockNative.
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.
Assignee

Comment 9

9 years ago
(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.
status2.0: --- → ?
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.
Attachment #503683 - Flags: review?(smichaud) → review+
Posted file 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.
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.
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.
> 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 :-)
Assignee

Comment 15

9 years ago
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.
Attachment #503683 - Attachment is obsolete: true
Attachment #505210 - Flags: approval2.0?
Assignee

Comment 16

9 years ago
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

9 years ago
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.
Attachment #505210 - Flags: approval2.0? → approval2.0+
Assignee

Updated

9 years ago
Keywords: checkin-needed

Comment 18

9 years ago
http://hg.mozilla.org/mozilla-central/rev/84ed248b728d
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
Assignee

Comment 19

8 years ago
Backed out:

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

See bug 629324.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 505210 [details] [diff] [review]
Proposed patch, version 3.

minusing approval due to backout
Attachment #505210 - Flags: approval2.0+ → approval2.0-

Updated

8 years ago
Whiteboard: [frame-rate-monitor] → [frame-rate-monitor][not-ready]
You need to log in before you can comment on or make changes to this bug.