With these patches, on top of the patches in bug 638241, we're just a bit faster than IE9 RC1 on FishIE Tank on my laptop with 2000 fish (you have to locally modify the testcase to add that option). 37fps vs 38fps.

Comment on attachment 517603 [details] [diff] [review] Part 1: Cache nsIImageLoadingContent pointer to avoid expensive do_QueryInterface in CanvasImageCache::Lookup hit path You need to initialize mILC to null in the other constructor. I am otherwise unable to verify to myself that it won't be used uninitialized.

Comment on attachment 517604 [details] [diff] [review] Part 2: Some trivial cleanup and microoptimizations >diff --git a/content/canvas/src/nsCanvasRenderingContext2D.cpp b/content/canvas/src/nsCanvasRenderingContext2D.cpp > NS_IMETHODIMP > nsCanvasRenderingContext2D::Redraw(const gfxRect& r) > { >+ if (mIsEntireFrameInvalid) >+ return NS_OK; >+ > if (!mCanvasElement) { > NS_ASSERTION(mDocShell, "Redraw with no canvas element or docshell!"); > return NS_OK; > } > > #ifdef MOZ_SVG > nsSVGEffects::InvalidateDirectRenderingObservers(HTMLCanvasElement()); > #endif > >- if (mIsEntireFrameInvalid) >- return NS_OK; >- > if (++mInvalidateCount > kCanvasMaxInvalidateCount) > return Redraw(); > > HTMLCanvasElement()->InvalidateFrame(&r); > > return NS_OK; > } Why is it safe to skip calling nsSVGEffects::InvalidateDirectRenderingObservers if our entire frame is invalid?

Initialize mILC to null in the other constructor. This wasn't needed because CanvasImageCache::NotifyDrawImage always sets mILC when we add a cache entry, but there's no harm in initializing it anyway.

Good catch. It's actually not safe. We need a hunk that previously been in bug 622072 part 3: nsLayoutUtils::SurfaceFromElement needs to MarkContextClean so that the "direct observers" (currently, -moz-element) can get more invalidations after they've fetched the surface. The invariant here is that when mIsEntireFrameInvalid is true, everywhere the canvas had been painted, and every direct observer, has been notified of invalidation since the last repaint or SurfaceFromElement call.

Comment on attachment 518560 [details] [diff] [review] Part 2 v2 Two requests: 1) Can you take the mIsEntireFrameInvalid in Redraw() (no argument) hunk out of part 4 and put it in this part, since it's more thematically related? 2) Can you create a test (or do we have one already?) that ensures we don't break as you describe in comment 10?

Comment on attachment 517606 [details] [diff] [review] Part 4: Optimize Redraw RedrawUser could also have a hunk that optimized to Redraw() if mPredictManyRedrawCalls, right?

(In reply to comment #11) > Two requests: > 1) Can you take the mIsEntireFrameInvalid in Redraw() (no argument) hunk out of > part 4 and put it in this part, since it's more thematically related? Sure. > 2) Can you create a test (or do we have one already?) that ensures we don't > break as you describe in comment 10? We already have such a test. I caught the failure when I pushed to try and added the fix to bug 622072 part 3.

(In reply to comment #12) > RedrawUser could also have a hunk that optimized to Redraw() if > mPredictManyRedrawCalls, right? It doesn't need to, that already happens. When mPredictManyRedrawCalls && !mIsEntireFrameInvalid, the first call to RedrawUser will call Redraw(gfxRect) which will call Redraw().

Comment on attachment 517607 [details] [diff] [review] Part 5: Track whether the current path in the canvas context is empty. If it is, then optimize path save/restore to not make a copy of the current path You should maybe set mHasPath to false in Reset(), for completeness. If my reading is correct, there are a couple of methods that implicitly leave paths on the canvas's mThebes: - ClearRect - DrawRect - DrawImage - PutImageData_explicit It's not clear to me, from reading the 2D context spec, whether those paths are supposed to be persistent. If they are, we need to set mHasPath to true in there; if not, it doesn't really matter.

Is this ready to go?

I think so, but it could probably use a tryserver build.

http://hg.mozilla.org/mozilla-central/rev/449386785f85 http://hg.mozilla.org/mozilla-central/rev/aeba92cb0fcd http://hg.mozilla.org/mozilla-central/rev/a34fe4ee349a http://hg.mozilla.org/mozilla-central/rev/b247015a9fc0 http://hg.mozilla.org/mozilla-central/rev/6a95db826e08

When I first applied this bug fix I saw significantly increased performance on IE9's Speed Reading test and a few other tests. However, I ran into screen corruption bug that is fixed by changeset - 63858:9a1aa415c9a1. I ran that hourly and the current one(changeset - 63865:e11c2f95f781) and the speed tests are now much slower than even before this patch. Did something get regressed? Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110324 Firefox/4.2a1pre - Build ID: 20110324211433

Just ran with a new profile and speeds are now fast. However, I didn't need a new profile with just this patch on. Seems some patche(s) after this one caused a performance drop with my normal profile.

I had to delete Localstore to get things going the way they should. In the course of moving things around I ran into the same problem. Resizing the sidebar seemed to affect speed reading performance. As of this moment all is well but I haven't tried resizing the sidebar yet.

From my testing when I have the sidebar opened and my window is NOT maximized I get a significant slowdown in the IE9 Speed Reading test. No opened sidebar, window maximized or not, results in a fast test. Sidebar open and window maximized results in a fast test.

Can you please file a bug on that?