On OS X, virtually every plugin update event (updateEvt in the QuickDraw event model) is sent twice. This can be a big problem for plugins: The QuickDraw event model's updateEvt provides no information on what part of the plugin window needs updating, so each updateEvt implies that the plugin needs to update its entire window. This bug tends to make plugins do far too much display updating, which can lead to performance problems and/or flickering. (It's true that the Cocoa event model's NPCocoaEventDrawRect event does provide a dirty rect. But currently no release version of any browser or plugin supports the Cocoa event model.) Here's one way to see this bug in action: 1) Copy the latest version of the DebugEventsPlugin (from bug 441880) to /Library/Internet Plug-Ins. 2) Load the plugin distro's test.html into Firefox 3. 3) Make the browser window smaller (vertically) than the plugin display (so that a vertical scrollbar appears). 4) Click once on the scrollbar's up or down arrow, and observe the events logged to the system console. You'll notice four calls to NPP_SetWindow() (two identical calls that have an empty clipRect, and two more identical calls that have a non-empty clipRect). (This duplication is fixed by my patch for bug 459244.) After the third and fourth calls to NPP_SetWindow() (those that have a non-empty clipRect, each one of them) you'll see an updateEvt. In other words two updateEvts are sent without any change in the plugin window's location or dimensions. If you test the tryserver build from bug 459244 comment #1, you'll find that it fixes the duplication of calls to NPP_SetWindow(). But you still see two updateEvts after the second (and last) call to NPP_SetWindow(). You get the same results with Firefox 2. In Safari you also see two calls to NPP_SetWindow() and two updateEvts (one after each call to NPP_SetWindow()). But notice that each call to NPP_SetWindow() has a different location and clipRect -- so having more than one updateEvt is justified. In my next comment I'll attach a patch that fixes this problem.

My patch fixes this bug, and might also be a significant performance gain (not just for plugins). But it's a bit risky, since there's no way to test it thoroughly besides putting it into the hands of lots of users. Here's a tryserver build made with this patch: https://build.mozilla.org/tryserver-builds/2008-10-09_15:56-smichaud@pobox.com-bugzilla459319/smichaud@pobox.com-bugzilla459319-firefox-try-mac.dmg

In principle this patch should give about a 100% improvement in (a doubling of) redrawing/reformatting performance. I've found a primitive and indirect test that shows about a 50% improvement, but I'd really appreciate help from others who know more about this kind of thing. It'd be nice to see better examples of tests than the one I've been able to come up with. The following test only works on OS X 10.5.X. 1) Load the test case from bug 458051 (attachment 341294 [details]) and wait for its images to finish loading. 2) Put the cursor in front of one of the images (I used the top one), then type exactly enough characters to make the reformatting slow down (19 characters for the first line, 30 for the second line). 3) Press a different key, then hold it down for 10 seconds (or for some other arbitrary, sufficiently long period of time). Observe how many new characters appeared. I tested with my tryserver build from comment #1 and the 2008-10-10-02-mozilla-central nightly (the one closest in time to when I made my tryserver build). On two different machines (both running OS X 10.5.5) I saw about 50% more characters in step 3 with my tryserver build than with the nightly (the number of characters with my tryserver build was about 1.5 the number of characters with the nightly).

My (preliminary) test results show that this doesn't just concern plugins.

Since this patch is somewhat risky, I think we should first try to get it into Firefox 3.1 beta2 (and into the hands of a lot of users). Then, if things work out, we can land it on the 1.9.0 branch.

(Following up comment #2) > In principle this patch should give about a 100% improvement in (a > doubling of) redrawing/reformatting performance. I should have said the following (because the frequency of calls to [NSView drawRect:] can't be the only factor in redrawing/reformatting performance): In principle this patch should give up to a 100% improvement in (a doubling of) redrawing/reformatting performance.

just two nits: >+// But it's easy to forsee it causing preformance problems in any context. s/forsee/foresee/ and s/preformance/performance The sentence could be improved, as it is it's not very clear.

Here's a revision that updates my comment and (hopefully) makes it clearer. Vlad, Josh suggested I ask you to do the review. You know more about the code that calls nsChildView::Update() (aka nsIWidget::Update()), and would be better able to suggest ways in which my patch might cause problems. I'm quite sure that my patch won't cause any updates to be missed -- [NSView displayIfNeeded] is called automatically by the OS, in response to the calls to [ChildView setNeedsDisplay:] and [ChildView setNeedsDisplayInRect:] made in other nsChildView methods (like nsChildView::Move()). But it *might* (under special circumstances) cause the page to be updated less smoothly. I think this is highly unlikely. But it's still worth looking into, especially if you can suggest specific examples (or testcases) where, thanks to the intensity of calls to nsIWidget::Update(), page updates might become less smooth (because of my patch).

Here are logs of the same operation in unpatched and patched versions of Minefield. In addition to the logging provided by my DebugEventsPlugin from bug 441880, I added logging to Minefield to indicate how many times [ChildView drawRect:] was called, the parameters it was called with, and whether or not it was called as a result of a call to [NSView displayIfNeeded] from nsChildView::Update(). If anybody's interested, I can post (as a patch) the logging code I added to Minefield to generate these logs. For each log I did the following: 1) Load the DebugEventsPlugin distro's test.html into Minefield. 2) Make the browser window smaller (vertically) than the plugin display (so that a vertical scrollbar appears). 3) Click once on the scrollbar's down arrow, wait a few seconds, then (without having moved the mouse) click again. The "unpatched" log shows about 1.5 times as many calls to [ChildView drawRect:] as the "patched" log. The total number of "rectsBeingDrawn" is also about 1.5 times as high in the "unpatched" log as it is in the "patched" one. Above I speculated that my patch cuts in half the number of calls to [ChildView drawRect:]. This appears to be wrong (at least in the general case). But a 33% reduction in the number of calls is still quite impressive. Calls to [NSView drawRect:] are quite expensive -- for the drawing they do, and for the calls to lock and unlock the drawing "focus" that the OS needs to make (before any drawing can take place). I don't yet have any good way to measure exactly *now* expensive (compared, say, Gecko's own page-reformatting code). But with luck I may find a way to do so. And/or with Vlad's help :-)

I asked Josh this on irc -- why isn't displayIfNeeded clearing the update region? It should -- so why are we getting duplicated updates? Your patch would certainly result in fewer drawRect calls, but that doesn't mean that it's necessarily correct.

(In reply to comment #9) > why isn't displayIfNeeded clearing the update region? As best I can tell, it _is_ clearing the update region. Here's what I think is going on. I have no way to measure this directly, but it makes sense of the evidence we have (including my test results from comment #2): Needed updates ("dirty regions") are queued at the same rate, with or without my patch. With my patch the rate at which Cocoa checks and clears this queue (in calls to drawRect:) is reduced. This allows the consolidation of dirty rects to be done more efficiently -- which reduces the number of times a given region has to be redrawn. It also reduces the number of calls to lock and unlock the drawing focus.

(Following up comment #10) > With my patch the rate at which Cocoa checks and clears this queue > (in calls to drawRect:) is reduced. This should have been: With my patch the rate at which Cocoa checks and clears this queue (in preparation for calls to drawRect:) is reduced.

In an attempt to learn more about tests of redrawing/reformatting performance, I started a thread at mozilla.dev.platform (http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/60316b0aa6a9669c#). Martijn pointed me to a couple of different tests. So I now have a total of three performance tests (Martijn's Scrolltest, Vlad/cworth's Time Render test, and my test from comment #2). But they give radically different results, which is ... puzzling. I have a hunch what may be going on -- I suspect my patch speeds up *both* Cocoa drawing and Gecko drawing. Martijn's Scrolltest (which shows a 4.5% improvement) may (perhaps) be a relatively pure measure of the performance gains from a 33% reduction in the number of calls to [ChildView drawRect:] (and the consequent greater efficiency in dealing with the queue of dirty rects). If this is right, my test from comment #2 (which shows a 50% improvement) must mostly measure Gecko performance gains. Vlad's Time Render test (which shows an incredible 5400% performance gain) might be an almost pure measure of Gecko performance gains. Or maybe it's just broken :-) Vlad, can you shed light on any of this? Or anyone else? (By the way, I'd *really* like to find a way to measure redrawing/reformatting perfomance while resizing a browser window.)

The patch also fixes the stair step effect when scrolling diagonally with the touchpad.

(In reply to comment #7) > But it *might* (under special circumstances) cause the page to be > updated less smoothly. I think this is highly unlikely. But it's > still worth looking into, especially if you can suggest specific You might also look at the Tdhtml tests http://www.mozilla.org/performance/test-cases/dhtml/runTests.html (specifically the layers ones, perhaps; list of tests here: http://www.mozilla.org/performance/test-cases/dhtml/). I remember a couple of years ago we took a significant Tdhtml hit on the Mac from some change because that change caused us draw more than before, which in turn meant that we got smooth animation rather than just jumpy/every Nth frame we had been getting on those tests. (The perf hit was accepted because the resulting behavior was correct and the previous behavior was not; I don't remember the bug offhand.)

(In reply to comment #13) > The patch also fixes the stair step effect when scrolling diagonally with the > touchpad. This sounds good, but also a bit scary. Does this mean that we're missing some repaints?

(In reply to comment #15) > Does this mean that we're missing some > repaints? It means that we're repainting only when we return to the native event loop after processing the scroll event, i.e. after scrolling both axes. Since we don't really want the first repaint, I don't think we should call it "missing" them. See also bug 457864 comment 12: Combining the patch of this bug with the one in bug 457864 fixes bug 418351 - in bz's words, it makes you feel "lighter and bubblier". :)

(Following up comment #16) > It means that we're repainting only when we return to the native > event loop after processing the scroll event, i.e. after scrolling > both axes. Since we don't really want the first repaint, I don't > think we should call it "missing" them. This sounds like a special case of what I described in comment #10. Making nsChildView::Update() a no-op on OS X allows paints (the queue of dirty rects) to be handled more efficiently. Here the OS seems to know better than we do :-)

Thanks, Smokey, for mentioning the DHTML tests -- I'd forgotten about them. Here are the results of some tests I did. The DHTML tests are (ahem) a bit flaky. Many of their results go up and down randomly by several percent on each iteration. I've tried to smooth out some of this variation -- by using a local copy of the tests, and by averaging the result of three tests. But I suspect their "margin of error" is 2-3% -- meaning we should ignore differences that are that size or smaller. The tests were all run on OS X 10.5.5. For each series of three tests, I started the browser, and opened runTests.html (which runs the tests once). Then I reloaded the page twice more (each time running the tests once again). Over all, my patch seems to make no difference in the speed with which these tests run. I also noticed no differences in how smoothly they run, even on an old, slow PowerPC G4. Among the individual tests, the only significant difference seems to be that the "scrolling" test is about 5% slower with my patch.

Here's my archive of local tests. I created it by (more or less) copying them from the server.

> Among the individual tests, the only significant difference seems to > be that the "scrolling" test is about 5% slower with my patch. Which is actually quite odd, given that Martijn's Scrolltest showed a 4.5% gain in speed (see comment #12).

Markus Stange suggested testing how fast I can make an animation run without dropping frames. Sounds like a good idea ... but does anyone know where I can find this kind of test?

Since I have a local copy of the DHTML tests, I can change the number of test iterations. Here are the results of testing with 51 iterations. The ~5% performance loss in the "scrolling" test persists (though, as I mentioned above, this is contradicted by a 4.5% performance gain in Martijn's Scrolltest test). There are also a 3% performance loss in the "layers1" test, a 4% loss in the "layers5" test, a 1% gain in the "meter" test, a 1% loss in the "movingtext" test, a 2.5% loss in the "replaceimages" test, and a 1.5% gain in the "zoom" test. Even with 51 iterations, some of these smaller percentages may be just noise.

Not going to mark this blocking1.9.1 until we know that such an optimization is OK, even then it may be too late for such a risky change.

In bug 457864 comment 14 roc explains why synchronous repaints during scrolling are necessary.

(In reply to bug 457864 comment 33) > Scrolling has two parts: bitblitting part of the window from one location to > another, and repainting the rest. Those two parts need to happen close together > in time or you get obvious transient glitches. These glitches can only occur if there was a repaint after the first part. But as best I can tell, that's not the case. Even if I add a long delay at the start of ChildView's drawRect method (and apply the patch in this bug), I can't see the glitches. Can we give this patch another chance? Now that we've branched, we have a trunk that's open to experimentation ;) By the way, this patch also fixes bug 363757.

(In reply to comment #25) > (In reply to bug 457864 comment 33) > > Scrolling has two parts: bitblitting part of the window from one location to > > another, and repainting the rest. Those two parts need to happen close together > > in time or you get obvious transient glitches. > > These glitches can only occur if there was a repaint after the first part. But > as best I can tell, that's not the case. Even if I add a long delay at the > start of ChildView's drawRect method (and apply the patch in this bug), I can't > see the glitches. That blitting phase certainly used to happen immediately on Linux and Windows. Maybe things are changing with compositing window managers.

Comment on attachment 343082 [details] [diff] [review] Fix rev1 (update comment) I think the comment should say something like "The OS is managing repaints well enough, we don't have to flush them out here".

Comment on attachment 343082 [details] [diff] [review] Fix rev1 (update comment) If roc's ok with it, I'm ok with it!

(In reply to comment #27) > I think the comment should say something like "The OS is managing > repaints well enough, we don't have to flush them out here". How about this? // The OS manages repaints well enough on its own, so we don't have to // flush them out here. In other words, the OS will automatically call // displayIfNeeded at the appropriate times, so we don't need to do it // ourselves. This results in some performance gains, and also avoids // display problems caused by calling displayIfNeeded at inappropriate // times. See bmo bug 459319.

it's tricky because the repaint whose timing we care about is not really the currently-dirty region, it's the blitting we did earlier. So I'd prefer the comment to be vague rather than incorrect.

OK, then. How about this? // The OS manages repaints well enough on its own, so we don't have to // flush them out here. In other words, the OS will automatically call // displayIfNeeded at the appropriate times, so we don't need to do it // ourselves. See bmo bug 459319.

Landed on mozilla-central with the comment from comment #31. http://hg.mozilla.org/mozilla-central/rev/ceab3c598859

By the way, in my comment from comment #31, the second sentence means exactly the same thing as the first sentence. All of the OS's repaints are done via displayIfNeeded.

My patch should probably bake on the trunk for a while before being landed on the 1.9.1 branch.

On my old Mac Mini G4 scrolling with the mouse wheel or the keyboard arrows has become jerky since the landing of this patch.

(In reply to comment #35) Please be more specific. We need specific steps to reproduce, specific URLs where reproduction is easier, and a (very) precise description of the problem.

(In reply to comment #36) 1.find an old Mac G4. 2.turn on smooth scrolling. 3.open http://www.lemonde.fr or http://standblog.org/blog or any long bugzilla page like this one. 4.Scroll down using the keyboard down arrow. Results: Scrolling is jerky (it's smooth for a second or so but frequently there are short jumps). Before this patch scrolling was smooth.

Steven, what about the issue pointed out in comment 37? Wont it be ready for 1.9.1?

(In reply to comment #37 and comment #38) I can confirm your report, gatellie@noos.fr ... sort of. Testing (with your STR from comment #37) on two different machines (a very old PowerPC G4 and an early model MacBook Pro), on OS X 10.4.11, scrolling with the up and down arrow keys is definitely more "jerky" with my patch for this bug than without it. I tested with today's Minefield and Shiretoko nightlies (the former has the patch and the latter doesn't). My testcases were this bug's page and the two other examples from comment #37. But when I tested on OS X 10.5.6 (on the same early model MacBook Pro), the Minefield nightly (with my patch) wasn't any more "jerky" than the Shiretoko nightly (without my patch). Oddly (and somewhat disturbingly) I also found that scrolling this bug's page usually took considerably more CPU (testing with 'top') with my patch (using today's Minefield nightly) than without it (using today's Shiretoko nightly). This happened on my PowerPC G4 and my MacBook Pro (the latter on both OS X 10.4.11 and 10.5.6). But it didn't happen on my (much more powerful) MacPro (with 8 cores). Marcia, I know that you (unlike me) can test on both PPC 10.4.11 and PPC 10.5.6. Could you test the STR from comment #37 on your G4 (the one that runs both OSes), and see if you can reproduce the results I got on my MacBook Pro? In other words, does the difference (the degree of "jerkiness") between the two nightlies (Minefield and Shiretoko) disappear running in OS X 10.5.6 even on an old PPC Mac?

(Following up comment #39) We probably should postpone landing this patch on the branches until we've figured out why it (sometimes) makes scrolling "jerkier" when smooth scrolling is on. This may be a problem with smooth scrolling. I'll dig further over next few weeks. If I find problems with smooth scrolling (and fixes for them), I'll open a new bug.

(Following up comment #39) > Oddly (and somewhat disturbingly) I also found that scrolling this > bug's page usually took considerably more CPU (testing with 'top') > with my patch (using today's Minefield nightly) than without it > (using today's Shiretoko nightly). This happened on my PowerPC G4 > and my MacBook Pro (the latter on both OS X 10.4.11 and 10.5.6). > But it didn't happen on my (much more powerful) MacPro (with 8 > cores). I should have made clear that I only see this increased CPU usage with smooth scrolling on. It disappears when you turn smooth scrolling off.

Steven: Here are my first set of results from testing on my MBP, running 10.5.6 witwh 4 GM RAM and following the steps in Comment 37: Running Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090106 Shiretoko/3.1b3pre and loading http://www.lemonde.fr/, scrolling is relatively smooth. Running Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090106 Minefield/3.2a1pre and loading the same site, scrolling is jerky. The best way to describe it I feel like the screen hits some "hiccups" along the way. I will next move to the lab where I perform a set of test on the PPC mac.

This testing was done with the G5 I have in the lab running 10.4.11 with 1.25 GB of RAM. Using Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.1b3pre) Gecko/20090106 Shiretoko/3.1b3pre, scrolling on the lemonde.fr site seems to be slightly worse than using the Intel MBP, but not as bad as the trunk scrolling. Using Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.2a1pre) Gecko/20090106 Minefield/3.2a1pre, smooth scrolling is jerky and seems about the same as the Trunk build I tested on Intel in Comment 42. I will now boot into the Leopard partition on the same machine to compare scrolling.

Marcia, please also test using this bug's page. The other pages contain a lot of objects that can interrupt scrolling, whether or not you're using the Minefield or Shiretoko nightly (and whether or not smooth scrolling is on). Your results should be more "regular" (and less confusing) with this bug's page.

Using the same G5 in Comment 43 and running the following, I tested using both this bug page and the other sites listed in Comment 37. Using Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090106 Shiretoko/3.1b3pre-> This bug page-> Scrolling is relatively smooth http://www.lemonde.fr/->Scrolling is relatively smooth http://standblog.org/blog->Scrolling is relatively smooth Using Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090106 Minefield/3.2a1pre-> This bug page-> Scrolling is jerky http://www.lemonde.fr/->Scrolling is jerky http://standblog.org/blog->Scrolling is jerky

Back to testing this bug page using the same Intel machine in Comment 42 - I see the same jerky behavior when scrolling this bug page with the latest trunk, but I do not see it with the latest 1.9.1. I can go back and test the G5 in Comment 43, but I suspect I will likely see the same thing.

Thanks for all your tests, Marcia. Your results differ substantially from mine -- I see no increased jerkiness from my patch (for this bug) on OS X 10.5.6, while you do. But at least we both see the increased jerkiness on OS X 10.4.11. I still suspect this is a problem with smooth scrolling. I'll look further into it over the next few weeks.

Two things occurred to me when thinking more about this bug. First, do you have Full keyboard access turned on in System Preferences (just a wild thought) since before this has shown to cause some things to differ in testing. Also I am nominating for in litmus since I want to make sure we have a smooth scrolling test case.

> do you have Full keyboard access turned on in System Preferences? Nope. Not on any of the machines/partitions I tested with.