705559 - Speed up canvas-to-canvas image drawing with Azure/D2D

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Because it relies on having an imgIRequest, as far as I can tell.  Or at least the place where we fill the cache is conditioned on that.

This may be what causes the performance slowdown at http://stackoverflow.com/questions/8286491/performance-issue-with-off-screen-canvas-in-firefox

Comment 1

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

Yes, the canvas image cache has never worked with canvas sources.

Comment 2

[Boris Zbarsky [:bzbarsky]]

roc points out that we don't really need the image cache here.  All we really need to do is fast-path the case when |canvas| is not null and |!mCanvasElement || canvas->OwnerDoc() == mCanvasElement->GetOwnerDoc()|.  And probably mCanvasElement != canvas, so we don't have to worry about the SFE_WANT_NEW_SURFACE bit.

At that point we should be taking the "simple" path through the <canvas> part of nsLayoutUtils::SurfaceFromElement, but even that has all sorts of complications with multiple contexts, etc.  Is there a sane common case here that would be fast to check for and cover the vast majority of cases?

Comment 3

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

I'm not sure if this is meant to be filed as Mac-only. I'm going to steal it for Windows on the assumption that that's what the stackoverflow issue was about.

Comment 4

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

On Windows, the immediate performance problems here are unrelated to the security checks. The problem is poor Azure-D2D performance. In particular, DrawTargetD2D->Snapshot followed by DrawSurface of the snapshot allocates a new snapshot with new GPU resources every time, even if the underlying DrawTarget state hasn't changed since the last snapshot.

Comment 5

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

Attachment: Part 1: nsCanvasRenderingContext2DAzure cleanups

Some small code cleanup to eliminate unnecessary branches etc. Might make some tiny performance improvements.

Comment 6

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

Attachment: performance test

BTW this is the canvas-to-canvas drawing performance test I'm using. The goal here is to get the canvas number close to or better than the image number --- and both of them as low as possible :-).

Comment 7

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

Attachment: Part 2: DrawTargetD2D fix

I don't understand why mSnapshots is currently a vector. We clear it when the target changes, so all the snapshot objects in the vector must represent the same surface contents. So let's just have a single snapshot object and let Snapshot() return the existing snapshot if there is one. That makes all the difference in this test; the mBitmap only needs to be created once, and everything else is fast.

Well, almost everything. mDependentTargets and mDependingOnTargets get big. So let's use hash-sets for those.

We need to keep the last mSnapshot alive otherwise it can be destroyed when the DrawImage call has finished using it. So make DrawTargetD2D hold a ref to it, and avoid cycles by having the snapshot surface store a non-addrefed pointer to its DrawTargetD2D.

Comment 8

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

Attachment: Part 3: use mTransformDirty

This flag was designed to allow us to avoid SetTransform calls. I don't know why we aren't doing that. This fixes that and speeds up both the image and canvas cases of my test by about 10% (since we never need to change the transform in this test).

Comment 9

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

With this patch, I get image times of about 1150ms and canvas times of about 1400ms. That's for 200K draws, so not too bad. Profiling shows that much of the remaining difference is mostly in the AddDependencyOnSource call in DrawTargetD2D, which is now about 10% of the profile. CanvasUtils::DoDrawImageSecurityCheck is still only about a few % of the profile (hard to tell exactly how much because inlining or something else is throwing off xperf).

Comment 10

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

Attachment: Part 4: speed up AddDependencyOnSource

Simple stuff, but it drops the canvas time to around 1250ms.

Probably good enough for now.

Comment 11

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

Attachment: Part 2 v2

Previous version failed to initialize mDrawTarget in SourceSurfaceD2DTarget.

Comment 12

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

Attachment: part 2 v3

Refresh patch correctly

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I'm not sure if this is meant to be filed as Mac-only.

That's just the usual bugzilla stupidity.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 577556 [details] [diff] [review]
Part 1: nsCanvasRenderingContext2DAzure cleanups

Can you describe what's changing here in a bit more detail? Should this have any functionality changes?

Comment 15

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

It's just refactoring.

We skip the canvas->CountContexts() == 1 checks because canvases currently can only have 0 or 1 contexts, so checking the canvas->GetContextAtIndex(0) result for null is equivalent.

We reorder some code so that two "if (canvas)" checks get fused into one.

The self-copy test gets moved inside the "if (canvas)" block after we've gotten srcCanvas, the rendering context, so we can simplify the self-copy test to "if (srcCanvas == this)".

nsLayoutUtils::SurfaceFromElementResult doesn't need to set imgsurf, we can use the surface in 'res' directly. Then imgsurf is only needed for the CanvasImageCache lookup, so we move it into there. It doesn't need to addref, since if non-null, it matches an entry in the cache which is holding a reference.

Comment 16

[Bas Schouten (:bas.schouten)]

Comment on attachment 577565 [details] [diff] [review]
Part 3: use mTransformDirty

Review of attachment 577565 [details] [diff] [review]:
-----------------------------------------------------------------

I have a patch like this on the graphics project branch. Note that when I last tested this, this made FishIE a little bit slower actually. I never investigated why (presumably this branch is poorly predicted in some cases?)

Comment 17

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

(In reply to Bas Schouten (:bas) from comment #16)
> I have a patch like this on the graphics project branch. Note that when I
> last tested this, this made FishIE a little bit slower actually. I never
> investigated why (presumably this branch is poorly predicted in some cases?)

I find that hard to believe, but OK!

Comment 18

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

https://tbpl.mozilla.org/?tree=Try&rev=953ed8559efc

Comment 19

[Bas Schouten (:bas.schouten)]

Comment on attachment 577581 [details] [diff] [review]
part 2 v3

Review of attachment 577581 [details] [diff] [review]:
-----------------------------------------------------------------

Let's add a documentation in 2D.h that the returned snapshot object can be a cached object and is not necessarily uniquely owned by the user.

::: gfx/2d/DrawTargetD2D.cpp
@@ +191,5 @@
>  
> +  if (mSnapshot && !mSnapshot->hasOneRef()) {
> +    // If we hold the only reference, just let it die. Otherwise we have to
> +    // detach it.
> +    mSnapshot->DrawTargetWillChange();

Let's just set mSnapshot->mDrawTarget = NULL; Copying mTexture is a waste since we know it'll never change and can happily be kept alive by the snapshot.

We could actually just do mSnapshot->MarkIndependent() here unconditionally I think, it'll clear our reference to the snapshot and the snapshot's reference to the DrawTarget.

@@ +217,5 @@
> +  if (!mSnapshot) {
> +    mSnapshot = new SourceSurfaceD2DTarget();
> +    mSnapshot->mFormat = mFormat;
> +    mSnapshot->mTexture = mTexture;
> +    mSnapshot->mDrawTarget = this;

Let's just move these initializations into the constructor while we're at it.

Comment 20

[Bas Schouten (:bas.schouten)]

Comment on attachment 577578 [details] [diff] [review]
Part 4: speed up AddDependencyOnSource

Review of attachment 577578 [details] [diff] [review]:
-----------------------------------------------------------------

Hrm, I'm fine with this patch, but doesn't this just -add- one operation in the !mDependingOnTargets.count(aSource->mDrawTarget) case (it does a count, and two inserts), and remove one operation in the mDependingOnTargets.count(aSource->mDrawTarget) case (i.e. it only does a count, instead of two inserts). Insert should not do anything if an entry is already found in the hash set?

Comment 21

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> (In reply to Bas Schouten (:bas) from comment #16)
> > I have a patch like this on the graphics project branch. Note that when I
> > last tested this, this made FishIE a little bit slower actually. I never
> > investigated why (presumably this branch is poorly predicted in some cases?)
> 
> I find that hard to believe, but OK!

Yeah, so do I. It confused me quite a bit since we introduced this based on earlier profiles! Anyway, since I was optimizing for FishIE/FishBowl and friends which change transforms often, I stopped using it because of the minor perf improvement.

I started using it again on the graphics project branch due to hitting cases where the transform didn't change a lot and it made us faster. And haven't seen a noticable slowdown on that branch in FishIE and friends. So we should be fine.

Comment 22

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

(In reply to Bas Schouten (:bas) from comment #19)
> Let's just set mSnapshot->mDrawTarget = NULL; Copying mTexture is a waste
> since we know it'll never change and can happily be kept alive by the
> snapshot.
> 
> We could actually just do mSnapshot->MarkIndependent() here unconditionally
> I think, it'll clear our reference to the snapshot and the snapshot's
> reference to the DrawTarget.

Good point.

> @@ +217,5 @@
> > +  if (!mSnapshot) {
> > +    mSnapshot = new SourceSurfaceD2DTarget();
> > +    mSnapshot->mFormat = mFormat;
> > +    mSnapshot->mTexture = mTexture;
> > +    mSnapshot->mDrawTarget = this;
> 
> Let's just move these initializations into the constructor while we're at it.

OK.

Comment 23

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

(In reply to Bas Schouten (:bas) from comment #20)
> Hrm, I'm fine with this patch, but doesn't this just -add- one operation in
> the !mDependingOnTargets.count(aSource->mDrawTarget) case (it does a count,
> and two inserts), and remove one operation in the
> mDependingOnTargets.count(aSource->mDrawTarget) case (i.e. it only does a
> count, instead of two inserts). Insert should not do anything if an entry is
> already found in the hash set?

Yes, but it makes a huge difference in practice. From profiles it looks to me like insert allocates memory even if the element is already in the set.

Comment 24

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

Some test failures on try that I need to look into.

Comment 25

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

> We could actually just do mSnapshot->MarkIndependent() here unconditionally I
> think, it'll clear our reference to the snapshot and the snapshot's reference to
> the DrawTarget.

Yeah, that works. I'm using a death-grip RefPtr to keep the snapshot object alive while we call MarkIndependent. Don't want MarkIndependent's setting of mDrawTarget->mSnapshot = NULL to cause the snapshot to die while we're in one of its methods.

The test failures were caused by DrawTargetD2D's destructor calling the mSnapshot RefPtr's destructor, which unrefs the SourceSurfaceD2DTarget and destroys that; ~SourceSurfaceD2DTarget runs MarkIndependent, which goes back to the DrawTargetD2D and sets it's mSnapshot to NULL, unreffing the SourceSurfaceD2DTarget *again*. Fixed this by observing that the SourceSurfaceD2DTarget destructor actually doesn't need to do anything: *something* must be clearing DrawTargetD2D::mSnapshot or the snapshot object wouldn't be destroyed, so ~SourceSurfaceD2DTarget doesn't need to do it again.

https://tbpl.mozilla.org/?tree=Try&rev=8fb88a4885eb

Comment 26

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/58146fa12d76
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a100a5d2796
https://hg.mozilla.org/integration/mozilla-inbound/rev/c635867cc091
https://hg.mozilla.org/integration/mozilla-inbound/rev/40a13154e0ba

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/58146fa12d76
https://hg.mozilla.org/mozilla-central/rev/9a100a5d2796
https://hg.mozilla.org/mozilla-central/rev/c635867cc091
https://hg.mozilla.org/mozilla-central/rev/40a13154e0ba