Bug 73830 - optimize TransformationMatrix::scale by not calling through to generic multiply
Summary: optimize TransformationMatrix::scale by not calling through to generic multiply
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike Reed
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-05 07:52 PST by Mike Reed
Modified: 2011-12-06 12:44 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.36 KB, patch)
2011-12-05 07:54 PST, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (1.98 KB, patch)
2011-12-06 05:17 PST, Mike Reed
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.