Closed Bug 1077961 Opened 5 years ago Closed 5 years ago

Stop doing lots of unnecessary and expensive Matrix4x4 multiplication

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

(Keywords: perf)

Attachments

(2 files)

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.)
(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 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?
Attachment #8500100 - Flags: review?(bas) → review+
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!
Attachment #8500101 - Flags: review?(bugmail.mozilla)
Attachment #8500101 - Flags: review?(bas)
Attachment #8500101 - Flags: review+
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
Attachment #8500101 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.)
The comments in Matrix4x4 refer to PreTranslate but the function there is called Translate.
I'll follow up to change the names as we did for Matrix once I'm back on Tuesday.
https://hg.mozilla.org/mozilla-central/rev/1a860977f8c7
https://hg.mozilla.org/mozilla-central/rev/c0e4ec5fc709
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1082477
Depends on: 1082483
You need to log in before you can comment on or make changes to this bug.