951216 - Avoid unnecessary Snapshot() copies by clearing the pattern in gfxSurfaceDrawable

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

gfxSurfaceDrawable::Draw() sets a gfxPattern(Snapshot()) to the gfxContext's pattern. If the gfxSurfaceDrawable is created by CreateSamplingRestrictedDrawable it will be destroyed before the Snapshot(). This causes the Snapshot to be copied unnecessarily.

If we set the pattern to something else we'll destroy the snapshot before the gfxSurfaceDrawable and save a copy.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Avoid unnecessary Snapshot() copies by clearing the pattern in gfxSurfaceDrawable

Comment 2

[Matt Woodrow (:mattwoodrow)]

What backends does this actually copy for? I thought we often just transferred ownership of the underlying data/object when a DT gets destroyed while a snapshot still exists.

It seems like this is quite an easy trap to fall into, making it fast seems worthwhile.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> What backends does this actually copy for? I thought we often just
> transferred ownership of the underlying data/object when a DT gets destroyed
> while a snapshot still exists.

Cairo and CG.

> It seems like this is quite an easy trap to fall into, making it fast seems
> worthwhile.

I agree.

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/767fa5332415

Comment 5

[Jeff Muizelaar [:jrmuizel]]

This gave a 4-7% improvement on tscroll on OS X

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8348812 [details] [diff] [review]
Avoid unnecessary Snapshot() copies by clearing the pattern in gfxSurfaceDrawable

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 896489
User impact if declined: Lower performance when drawing scaled images
Testing completed (on m-c, etc.): Has been on m-c for a little while now
Risk to taking this patch (and alternatives if risky): Very low risk.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/54709f91b972

Comment 9

[u279076]

Does this have or need tests?

Comment 10

[Jeff Muizelaar [:jrmuizel]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #9)
> Does this have or need tests?

The existing reftests should be sufficient.