Closed Bug 639689 Opened 9 years ago Closed 9 years ago

Miscellaneous performance improvements to canvas.drawImage

Categories

(Core :: Canvas: 2D, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

(Whiteboard: [fx4-rc-ridealong])

Attachments

(5 files, 2 obsolete files)

No description provided.
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.
Depends on: 638241
Whiteboard: [fx4-rc-ridealong]
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.
Attachment #517603 - Flags: review?(joe) → review-
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?
Attachment #517605 - Flags: review?(joe) → review+
Attached patch part 1 v2Splinter Review
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.
Attachment #518555 - Flags: review?(joe)
Attached patch Part 2 v2Splinter Review
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.
Attachment #517603 - Attachment is obsolete: true
Attachment #517604 - Attachment is obsolete: true
Attachment #517604 - Flags: review?(joe)
Attachment #518560 - Flags: review?(joe)
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?
Attachment #518560 - Flags: review?(joe) → review+
Comment on attachment 517606 [details] [diff] [review]
Part 4: Optimize Redraw

RedrawUser could also have a hunk that optimized to Redraw() if mPredictManyRedrawCalls, right?
Attachment #517606 - Flags: review?(joe) → review+
(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().
Attachment #518555 - Flags: review?(joe) → review+
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.
Attachment #517607 - Flags: review?(joe) → review+
Is this ready to go?
I think so, but it could probably use a tryserver build.
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?
Depends on: 662898
Depends on: 662450
You need to log in before you can comment on or make changes to this bug.