Bug 73830

Summary: optimize TransformationMatrix::scale by not calling through to generic multiply
Product: WebKit Reporter: Mike Reed <reed>
Component: New BugsAssignee: Mike Reed <reed>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, dglazkov, kbr, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Mike Reed 2011-12-05 07:52:38 PST
optimize TransformationMatrix::scale by not calling through to generic multiply
Comment 1 Mike Reed 2011-12-05 07:54:32 PST
Created attachment 117880 [details]
Patch
Comment 2 WebKit Review Bot 2011-12-05 08:44:04 PST
Comment on attachment 117880 [details]
Patch

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

New failing tests:
svg/custom/linking-uri-01-b.svg
Comment 3 Kenneth Russell 2011-12-05 11:07:58 PST
Comment on attachment 117880 [details]
Patch

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

Is the new SVG failure spurious? It's hard to believe that it could be caused by this patch.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:695
> +    m_matrix[0][0] *= sx;
> +    m_matrix[0][1] *= sx;
> +    m_matrix[0][2] *= sx;
> +    m_matrix[0][3] *= sx;
> +    
> +    m_matrix[1][0] *= sy;
> +    m_matrix[1][1] *= sy;
> +    m_matrix[1][2] *= sy;
> +    m_matrix[1][3] *= sy;

Could this one at least call scaleNonUniform to reduce code duplication? Would that eliminate the benefit of the optimization?
Comment 4 Mike Reed 2011-12-05 11:29:03 PST
I'll run drt locally and see if I can repro the failure. *possibly* we are getting different precision with this version, and therefore a slightly different matrix?

I think we can have the scale3 function call the 2D one, and then scale its column. I'll try that in the next patch.
Comment 5 Mike Reed 2011-12-06 05:17:25 PST
Created attachment 118031 [details]
Patch
Comment 6 Mike Reed 2011-12-06 05:57:42 PST
previous failure must have been a flake, as the new patch is the same (modulo sharing code between the scaleNonUniform and scale3D)
Comment 7 Kenneth Russell 2011-12-06 10:42:42 PST
Comment on attachment 118031 [details]
Patch

Looks good. r=me
Comment 8 WebKit Review Bot 2011-12-06 11:27:50 PST
Comment on attachment 118031 [details]
Patch

Clearing flags on attachment: 118031

Committed r102157: <http://trac.webkit.org/changeset/102157>
Comment 9 WebKit Review Bot 2011-12-06 11:27:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Simon Fraser (smfr) 2011-12-06 11:30:19 PST
Why were none of the main authors of TransformationMatrix.cpp cc'd on this bug?
Comment 11 Kenneth Russell 2011-12-06 11:36:02 PST
(In reply to comment #10)
> Why were none of the main authors of TransformationMatrix.cpp cc'd on this bug?

Sorry, that was a simple oversight on my part...

Please let us know if you find any issue with this change. I'm happy to roll it out and help rework it if necessary.
Comment 12 Mike Reed 2011-12-06 11:57:58 PST
My bad too. Is there a generic way to know who the interested authors are for a given directory/file?
Comment 13 Mike Reed 2011-12-06 12:44:41 PST
using 'svn blame' I see cmarrin and simon. Will remember to do that next time.