529386 - optimize canvas.width = canvas.width

Status: RESOLVED INACTIVE

(Core :: Graphics: Canvas2D, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

See bug 525673 -- the testcase there uses "canvas.width = canvas.width;" at the start of its draw loop as a way to clear/reset the canvas, becuase the canvas state and contents are supposed to be reset whenever width or height are set.  We always create a new surface in response to this, even when the width/height are the same as the previous; we should instead just clear the surface, but still create a new gfxContext to avoid the memory churn.

Would be good to profile this to make sure that it's a win, but I think it will be.

Comment 1

[Saint Wesonga]

Attachment: Simple performance test

Comment 2

[Saint Wesonga]

Attachment: Patch

Reuses the code in InitializeWithSurface to create a new gfxContext and initialize the canvas default state.

Comment 3

[Saint Wesonga]

With the attached perf test, average times were approx 183ms without the patch and 122ms with the patch.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 414436 [details] [diff] [review]
Patch

missed this, sorry!  Catching up on review backlog this week.

Comment 5

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/aa4791e4dad9

Comment 6

[Peter Van der Beken [:peterv]]

Backed out, it seems to cause a mochitest failure in content/canvas/test/test_canvas.html:

ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_canvas.html | unexpected exception thrown in: test_2d_path_arcTo_scale

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1263467375.1263467986.25860.gz

I'm also seeing a bunch of "unexpected exception thrown" failures in my local OS X debug build that go away when I remove this patch.

Comment 7

[Peter Van der Beken [:peterv]]

See also

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1263470418.1263471071.27256.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1263471234.1263472035.6214.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1263469753.1263471573.761.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1263473180.1263474919.7568.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1263472453.1263473649.25234.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1263474445.1263475246.11881.gz

Comment 8

[Saint Wesonga]

I've just applied this patch to a new build on Windows 7 and all the tests pass. looking into this to see what's going on...

Comment 9

[Saint Wesonga]

Attachment: Test results

This patch does not cause any test failures on my XP installation either. How long are the build logs kept for? I wasn't aware that they expire that soon... Any way to rebuild this on a try server or smth for new logs? (I don't have access to a Mac, which is where I think this was problem).

Comment 10

[Matt Woodrow (:mattwoodrow)]

This patch has rotted sadly (sorry for letting it sit this long).

The problem is most liking the skipped call to Destroy() (now called Reset()) which did more than just release the surface.

I'd suggest using an nsRefPtr to keep the surface alive, and still call Reset().

If you get a fixed patch uploaded, I'd be happy to handle running it on tryserver and landing it for you.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:lsalzman, could you have a look please?
For more information, please visit auto_nag documentation.