639689 - Miscellaneous performance improvements to canvas.drawImage

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Cache nsIImageLoadingContent pointer to avoid expensive do_QueryInterface in CanvasImageCache::Lookup hit path

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Some trivial cleanup and microoptimizations

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: Remove unnecessary full context save/restore, and redundant SetPattern call

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: Optimize Redraw

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 7

[Joe Drew (not getting mail)]

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 8

[Joe Drew (not getting mail)]

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?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 1 v2

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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2 v2

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 11

[Joe Drew (not getting mail)]

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 12

[Joe Drew (not getting mail)]

Comment on attachment 517606 [details] [diff] [review]
Part 4: Optimize Redraw

RedrawUser could also have a hunk that optimized to Redraw() if mPredictManyRedrawCalls, right?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 15

[Joe Drew (not getting mail)]

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.

Comment 16

[(no longer active)]

Is this ready to go?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://tbpl.mozilla.org/?tree=MozillaTry&rev=7c0f7238be0d

Comment 19

[(no longer active)]

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

Comment 20

[Gary [:streetwolf52]]

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

Comment 21

[Gary [:streetwolf52]]

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.

Comment 22

[Gary [:streetwolf52]]

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.

Comment 23

[Gary [:streetwolf52]]

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.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Can you please file a bug on that?