Bug 79215 – Update canvas path construction/usage behavior

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Canvas

[Matthew Delaney]

For whatever historical reasons, the context's path is currently being constructed in an somewhat odd fashion. The path is effectively inverse transformed every time the CTM changes, so that when it's used for stroking or filling the CTM will undo those transforms to get the points in the correct locations. This causes a lot of unnecessary copying of the path (i.e. when it's transformed) and matrix multiplications all around. All of the overhead of dealing with the path in this way can be avoided if instead it's points are transformed as they're added. As an added benefit, other canvas path usage logic becomes simpler.

[Matthew Delaney]

Created attachment 128164 [details]
Patch

[Philippe Normand]

Comment on attachment 128164 [details]
Patch

Attachment 128164 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11561443

[Matthew Delaney]

This is a WIP patch that will currently only build on top of CG since I didn't update the other port's Path[port-name].cpp files to support the new methods signatures (i.e. path construction methods that take a transform). Even if their underlying Path APIs don't support it, it would be easy to just add in a matrix mult. to the inputs.

It passes all the layout tests on my local machine. Though, since it's not exactly a small change, I wanted to see what others thought before cleaning it up all the way.

[Early Warning System Bot]

Comment on attachment 128164 [details]
Patch

Attachment 128164 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11556461

[Gyuyoung Kim]

Comment on attachment 128164 [details]
Patch

Attachment 128164 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11557464

[WebKit Review Bot]

Comment on attachment 128164 [details]
Patch

Attachment 128164 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11558455

[Simon Fraser (smfr)]

> For whatever historical reasons
You need to understand and explain those reasons in the Changelog.

[Daniel Bates]

Comment on attachment 128164 [details]
Patch

r- by comment #7 and because this breaks every non-Mac build as reported by the EWS bots.

[Dirk Schulze]

Comment on attachment 128164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128164&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:797
> +    m_path.addLineTo(p1, state().m_transform);

I think it is really worth exploring the performance difference. It is true that we copy the path in CG for every transform operation. I don't think it is a problem for Cairo.

However, if we transform the points instead, we should do it in Context2d.cpp directly. Not in the platform files. Do you want to proceed? Ryosuke, do we have performance tests for that issue already?

[Dirk Schulze]

Created attachment 228499 [details]
Performance test

This is a short performance test. It creates a path, rotates the context 360,000 times and measures the time in ms it took to transform.

Safari 7: ~80ms
WebKit nightly: ~60ms;

Firefox Nightly: ~12ms;
Chrome/ium: ~120ms;

[Dirk Schulze]

(In reply to comment #10)
> Created an attachment (id=228499) [details]
> Performance test
> 
> This is a short performance test. It creates a path, rotates the context 360,000 times and measures the time in ms it took to transform.
> 
> Safari 7: ~80ms
> WebKit nightly: ~60ms;
> 
> Firefox Nightly: ~12ms;
> Chrome/ium: ~120ms;

Even though Firefox is much faster, relatively speaking it seems unlikely that you transform the context 360k times per routine call. So even if we might be able to improve the performance, I don't think it makes much difference.

[Dirk Schulze]

(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=228499) [details] [details]
> > Performance test
> > 
> > This is a short performance test. It creates a path, rotates the context 360,000 times and measures the time in ms it took to transform.
> > 
> > Safari 7: ~80ms
> > WebKit nightly: ~60ms;
> > 
> > Firefox Nightly: ~12ms;
> > Chrome/ium: ~120ms;
> 
> Even though Firefox is much faster, relatively speaking it seems unlikely that you transform the context 360k times per routine call. So even if we might be able to improve the performance, I don't think it makes much difference.

On debug builds it is a difference from 290ms without transform and 320ms with transform. This is ~10% difference. Note, with this micro optimization the average time to call the path methods increase. Since the path methods are much more often used, we need very careful performance measurements. I don't think it is worth it. Closing the bug.