Bug 101828 – Improve the performance of rect transform

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Benjamin Poulain]

First step: improvement for all hardware.

[Benjamin Poulain]

Created attachment 173434 [details]
Patch

[Brent Fulgham]

Comment on attachment 173434 [details]
Patch

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

These changes seem very useful -- thanks for looking into this!  Both changes are effectively inlining the mapQuad/mapPoint logic to avoid the cost of the "isIdentityOrTranslation()" call.  Do you think we could make a helper function that didn't call this method, and have the full fledged mapQuad/mapPoint  check for identity, then call this internal function?

That way, we could reuse the same logic in both places without paying the high cost you identified in your change.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:702
> +    return result.boundingBox();

This is just an inline of mapQuad (and subsequent mapPoints) without the identity/translation check.  What if we had a mapQuadNoCheck (or mapQuadImpl) that we could call here (since we've already tested for identity/translation).  Then we could share the code here and in the mapQuad call.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:717
> +    result.setP1(FloatPoint(x, y));

This is just an inline of mapPoint that doesn't do the identity check.  What if we made a mapPointNoCheck (or something) that could be used here, and inside the real "mapPoint" call?

[Benjamin Poulain]

(In reply to comment #2)
> (From update of attachment 173434 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173434&action=review
> 
> These changes seem very useful -- thanks for looking into this!  Both changes are effectively inlining the mapQuad/mapPoint logic to avoid the cost of the "isIdentityOrTranslation()" call.  Do you think we could make a helper function that didn't call this method, and have the full fledged mapQuad/mapPoint  check for identity, then call this internal function?
> 
> That way, we could reuse the same logic in both places without paying the high cost you identified in your change.

The two function are a little different.
In the case of Quad, you really have 8 coordinates, for a rect, you only have 4. That matters because you can remove a number of relatively costly instructions.

The next step is to break into pieces to do fewer operation in the case of the rect. I could have done that in this patch but I wanted to keep it trivial (hoping for a fast review :)).

Then for ARM, I should implement them in assembly to take advantage of the structure we have in TransformationMatrix.

> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:702
> > +    return result.boundingBox();
> 
> This is just an inline of mapQuad (and subsequent mapPoints) without the identity/translation check.  What if we had a mapQuadNoCheck (or mapQuadImpl) that we could call here (since we've already tested for identity/translation).  Then we could share the code here and in the mapQuad call.

Check again, this is not exactly inline of mapQuad :)

> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:717
> > +    result.setP1(FloatPoint(x, y));
> 
> This is just an inline of mapPoint that doesn't do the identity check.  What if we made a mapPointNoCheck (or something) that could be used here, and inside the real "mapPoint" call?

Yep, that sounds like a good idea.

[Benjamin Poulain]

Created attachment 174783 [details]
Patch

[Early Warning System Bot]

Comment on attachment 174783 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 174783 [details]
Patch

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

[kov's GTK+ EWS bot]

Comment on attachment 174783 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 174783 [details]
Patch

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

[Benjamin Poulain]

Comment on attachment 174783 [details]
Patch

Forgot to close a parenthesis on the update.

[EFL EWS Bot]

Comment on attachment 174783 [details]
Patch

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

[EFL EWS Bot]

Comment on attachment 174783 [details]
Patch

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

[Benjamin Poulain]

Created attachment 174798 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 174798 [details]
Patch

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

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:382
> +    FloatPoint mapPointImpl(const FloatPoint& sourcePoint) const

I'm not a huge fan of mapPointImpl(). Maybe internalMapPoint()?

[Benjamin Poulain]

> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:382
> > +    FloatPoint mapPointImpl(const FloatPoint& sourcePoint) const
> 
> I'm not a huge fan of mapPointImpl(). Maybe internalMapPoint()?

Sounds good to me. I'll update the patch.

Thanks for the review.

[Benjamin Poulain]

Committed r135038: <http://trac.webkit.org/changeset/135038>