optimize canvas.width = canvas.width

REOPENED
Assigned to

Status

()

Core
Canvas: 2D
REOPENED
8 years ago
6 years ago

People

(Reporter: vlad, Assigned: Saint Wesonga)

Tracking

(Depends on: 1 bug)

Trunk
mozilla1.9.3a1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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.
Whiteboard: [good first bug]
(Assignee)

Comment 1

8 years ago
Created attachment 414435 [details]
Simple performance test
(Assignee)

Comment 2

8 years ago
Created attachment 414436 [details] [diff] [review]
Patch

Reuses the code in InitializeWithSurface to create a new gfxContext and initialize the canvas default state.
Assignee: nobody → wesongathedeveloper
Status: NEW → ASSIGNED
Attachment #414436 - Flags: review?(vladimir)
(Assignee)

Comment 3

8 years ago
With the attached perf test, average times were approx 183ms without the patch and 122ms with the patch.
Comment on attachment 414436 [details] [diff] [review]
Patch

missed this, sorry!  Catching up on review backlog this week.
Attachment #414436 - Flags: review?(vladimir) → review+
Keywords: checkin-needed
Whiteboard: [good first bug] → [has patch]
http://hg.mozilla.org/mozilla-central/rev/aa4791e4dad9
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla1.9.3a1
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

8 years ago
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...
(Assignee)

Comment 9

8 years ago
Created attachment 434128 [details]
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).
(Assignee)

Updated

8 years ago
Depends on: 572581
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.
You need to log in before you can comment on or make changes to this bug.