Bug 152119

Summary: Shave off a TransformationMatrix copy in RenderLayer's transparencyClipBox()
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, glenn, kondapallykalyan, simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Zan Dobersek 2015-12-10 04:03:09 PST
Shave off a TransformationMatrix copy if RenderLayer's transparencyClipBox()
Comment 1 Zan Dobersek 2015-12-10 04:05:50 PST
Created attachment 267091 [details]
Patch
Comment 2 Simon Fraser (smfr) 2015-12-10 10:20:24 PST
Comment on attachment 267091 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:1762
> +        transform.multiply(*layer.transform());

According to the comment for TransformationMatrix::multiply(), this change affects multiplication order.
Comment 3 Zan Dobersek 2015-12-29 06:48:44 PST
Comment on attachment 267091 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:1762
>> +        transform.multiply(*layer.transform());
> 
> According to the comment for TransformationMatrix::multiply(), this change affects multiplication order.

The comments for the * and *= operators in TransformationMatrix are a bit misleading. Both `A = A * B` and `A *= B` call `A.multiply(B)`, which sets the matrix data of A to the results of a BA matrix product.

So A *= B behaves the same as A.multiply(B), and both yield the same result as `A = A * B`, but the latter is more wasteful since it first copies the A matrix into a temporary object, multiplies B into that, and assigns the temporary object to A (which may produce another copy, not sure).
Comment 4 Simon Fraser (smfr) 2015-12-31 20:37:37 PST
Comment on attachment 267091 [details]
Patch

OK. Do tests break if you deliberately change the order?
Comment 5 Zan Dobersek 2016-01-03 01:55:08 PST
(In reply to comment #4)
> Comment on attachment 267091 [details]
> Patch
> 
> OK. Do tests break if you deliberately change the order?

No, at least the CSS and fast/ tests don't.
Comment 6 Zan Dobersek 2016-01-04 23:38:09 PST
Comment on attachment 267091 [details]
Patch

Clearing flags on attachment: 267091

Committed r194576: <http://trac.webkit.org/changeset/194576>
Comment 7 Zan Dobersek 2016-01-04 23:38:17 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Adler 2016-01-11 08:50:13 PST
(In reply to comment #3)
> So A *= B behaves the same as A.multiply(B), and both yield the same result
> as `A = A * B`, but the latter is more wasteful since it first copies the A
> matrix into a temporary object, multiplies B into that, and assigns the
> temporary object to A (which may produce another copy, not sure).

Is there something straightforward we can do to fix A.multiply(B) so that it is not more wasteful than A *= B? Or perhaps we need to instead remove the multiply function.
Comment 9 Darin Adler 2016-01-13 08:55:16 PST
Oh, I guess I got it backwards and it's the multiply function that is more efficient.
Comment 10 Zan Dobersek 2016-01-13 11:00:07 PST
`A *= B` and `A.multiply(B)` are identical, the o*= operator implementation calls multiply().

The * operator is still useful, but it's easy enough to misuse it like `A = A * B` does.

As to spotting these occurrences -- some time ago I was playing around with Clang's AST matching facilities which allow the user to inspect the AST for specific code patterns. I used that mostly to spot wasteful container copies, and was also able to spot this specific issue. Extending that to cover additional patterns that we would want to eliminate from the WebKit codebase would probably be a worthwhile side-project.