Bug 152119 – Shave off a TransformationMatrix copy in RenderLayer's transparencyClipBox()

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

Shave off a TransformationMatrix copy if RenderLayer's transparencyClipBox()

[Zan Dobersek]

Created attachment 267091 [details]
Patch

[Simon Fraser (smfr)]

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.

[Zan Dobersek]

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).

[Simon Fraser (smfr)]

Comment on attachment 267091 [details]
Patch

OK. Do tests break if you deliberately change the order?

[Zan Dobersek]

(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.

[Zan Dobersek]

Comment on attachment 267091 [details]
Patch

Clearing flags on attachment: 267091

Committed r194576: <http://trac.webkit.org/changeset/194576>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(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.

[Darin Adler]

Oh, I guess I got it backwards and it's the multiply function that is more efficient.

[Zan Dobersek]

`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.