Closed Bug 951216 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox28 --- fixed
firefox29 --- fixed

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

Details

Attachments

(1 file)

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.
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/767fa5332415
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
This gave a 4-7% improvement on tscroll on OS X
Duplicate of this bug: 951214
Attachment #8348812 - Flags: review?(bas)
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.
Attachment #8348812 - Flags: approval-mozilla-aurora?
Attachment #8348812 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Does this have or need tests?
Flags: needinfo?(jmuizelaar)
Flags: in-testsuite?
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #9)
> Does this have or need tests?

The existing reftests should be sufficient.
Flags: needinfo?(jmuizelaar)
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.