Open Bug 625454 Opened 10 years ago Updated 10 years ago
Resizing causes us to spend a lot of time in -[NSView _get
Next Resize Event Invalidating Live Resize Cache If Necessary:]
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:]
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)
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 :-)
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.
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.
(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 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+
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 :-)
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.
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 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+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
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-
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.