1077961 - Stop doing lots of unnecessary and expensive Matrix4x4 multiplication

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

The patches in bug 952977 to switch us from gfx3DMatrix to Moz2D's Matrix4x4 seem to have caused considerable performance regressions in some cases. See "#2 Regression" in bug 1077842 comment 1, for example.

One thing that I notice about these patches is that they did not maintain functional parity in regards to the creation of simple scaling/translation matrices. Whereas previously we would use gfx3DMatrix::ScaleMatrix and gfx3DMatrix::Translation, we are now doing gfx::Matrix4x4().Scale() and gfx::Matrix4x4().Translate(). So whereas we previously just initialized the members of a gfx3DMatrix, we are now doing 12 floating point multiplications.

I've also noticed that we do a lot of this in the code:

  transform = transform * Matrix4x4().Translate(...);

In this case we're doing 12 floating point multiplications in the Translate() call, and _64_ floating point multiplications in the full matrix multiply. Code like this can be written as:

  transform.PostTranslate(...);

in order to avoid the full matrix multiply and keep the floating point multiplications to 12.

(I'm not sure to what extent bug 952977's patches are responsible for this latter issue, but we should fix it too.)

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Add various methods to Moz2D Matrix4x4 to allow us to minimize multiplications

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Stop doing lots of unnecessary and expensive Matrix4x4 multiplication

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #0)
> (I'm not sure to what extent bug 952977's patches are responsible for this
> latter issue, but we should fix it too.)

They are at least partly responsible. See for example:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cd72d8361cb

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 8500100 [details] [diff] [review]
part 1 - Add various methods to Moz2D Matrix4x4 to allow us to minimize multiplications

Review of attachment 8500100 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Matrix.h
@@ -544,5 @@
>  
>      return *this;
>    }
>  
> -  Rect ProjectRectBounds(const Rect& aRect) const;

nit: This removal seems to come a little out of nowhere? Was it just never implemented?

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 8500101 [details] [diff] [review]
part 2 - Stop doing lots of unnecessary and expensive Matrix4x4 multiplication

Review of attachment 8500101 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great to me!

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8500100 [details] [diff] [review]
part 1 - Add various methods to Moz2D Matrix4x4 to allow us to minimize multiplications

Review of attachment 8500100 [details] [diff] [review]:
-----------------------------------------------------------------

The return value of PostScale is wrong

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> The return value of PostScale is wrong

Or rather, the return value is fine but the temp matrix used to store the result is not, because that gets discarded and so the operation has no effect.

Comment 8

[Jonathan Watt [:jwatt]]

Hah, yeah, that wouldn't have passed Try. ;) Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a860977f8c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e4ec5fc709

I also added documenting comments matching those that we added to Matrix, and put the Translate methods before the Scale methods so those comments work better.

(In reply to Bas Schouten (:bas.schouten) from comment #4)
> nit: This removal seems to come a little out of nowhere? Was it just never
> implemented?

This was a move, not a remove. (Move that projection method declaration out from between the multiplication related methods and into the section dealing with projection.)

Comment 9

[Kartikaya Gupta (email:[e-mail])]

The comments in Matrix4x4 refer to PreTranslate but the function there is called Translate.

Comment 10

[Jonathan Watt [:jwatt]]

I'll follow up to change the names as we did for Matrix once I'm back on Tuesday.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a860977f8c7
https://hg.mozilla.org/mozilla-central/rev/c0e4ec5fc709