1437492 - Add matrix class optimized for simple matrices

Status: RESOLVED FIXED

(Core :: Graphics, enhancement, P3)

Description

[Bas Schouten (:bas.schouten)]

Transformations are showing up a significant amount in profiles, both in display list building and frame layer builder. The vast amount of the time we are dealing with simple transforms, identity or 2D. We can optimize away a lot under those circumstances.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1437492 - Part 1: Add a Matrix class with optimizations for simple matrices.

Review commit: https://reviewboard.mozilla.org/r/219466/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219466/

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1437492 - Part 2: Based on profile data, use the simple-matrix optimized matrix class in some places.

Review commit: https://reviewboard.mozilla.org/r/219468/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219468/

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8950200 [details]
Bug 1437492 - Part 1: Add a Matrix class with optimizations for simple matrices.

https://reviewboard.mozilla.org/r/219466/#review225826

::: gfx/2d/Matrix.h:2028
(Diff revision 1)
> +  }
> +
> +  template <typename NewTargetUnits>
> +  Matrix4x4TypedFlagged<SourceUnits, NewTargetUnits> operator*(const Matrix4x4Typed<TargetUnits, NewTargetUnits> &aMatrix) const
> +  {
> +    if (mType == MatrixType::Identity) {

Don't we want to check the reverse case here too?

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8950201 [details]
Bug 1437492 - Part 2: Based on profile data, use the simple-matrix optimized matrix class in some places.

https://reviewboard.mozilla.org/r/219468/#review225830

::: layout/base/nsLayoutUtils.cpp:2753
(Diff revision 1)
>    }
>    return ctm;
>  }
>  
> +Matrix4x4Flagged
> +nsLayoutUtils::GetTransformToAncestorFlagged(nsIFrame *aFrame,

This function is pretty complex, and duplicating the logic is a bit sad.

It looks like the only different with the Flagged version is the implicit type conversion in the return statement, so could we just make it wrapper function?

Alternatively, make the Flagged version the real one, and have the normal one be a wrapper around it that calls GetMatrix() on the result.

nsIFrame::GetTransformMatrix primarily initializes its matrices using Matrix4x4::Translation, so we could generate flagged matrices without needing an analyze call, and then callers that don't care can drop back to the unflagged version.

::: layout/base/nsLayoutUtils.cpp:2772
(Diff revision 1)
> +      !parent->IsStackingContext() &&
> +      !FrameHasDisplayPort(parent)))) {
> +    if (!parent->Extend3DContext()) {
> +      ctm.ProjectTo2D();
> +    }
> +    ctm = ctm * parent->GetTransformMatrix(aAncestor, &parent, aFlags);

Retained-dl calls this function a lot, and the matrices are almost always just translations.

Optimizing the * operator for translations might be a win.

Comment 5

[Bas Schouten (:bas.schouten)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Comment on attachment 8950200 [details]
> Bug 1437492 - Part 1: Add a Matrix class with optimizations for simple
> matrices.
> 
> https://reviewboard.mozilla.org/r/219466/#review225826
> 
> ::: gfx/2d/Matrix.h:2028
> (Diff revision 1)
> > +  }
> > +
> > +  template <typename NewTargetUnits>
> > +  Matrix4x4TypedFlagged<SourceUnits, NewTargetUnits> operator*(const Matrix4x4Typed<TargetUnits, NewTargetUnits> &aMatrix) const
> > +  {
> > +    if (mType == MatrixType::Identity) {
> 
> Don't we want to check the reverse case here too?

aMatrix is not a flagged matrix here :). We could make an optimized case for that but we're not currently using it anywhere so let's add it once we are chaining that way.

Comment 6

[Bas Schouten (:bas.schouten)]

Comment on attachment 8950200 [details]
Bug 1437492 - Part 1: Add a Matrix class with optimizations for simple matrices.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219466/diff/1-2/

Comment 7

[Bas Schouten (:bas.schouten)]

Comment on attachment 8950201 [details]
Bug 1437492 - Part 2: Based on profile data, use the simple-matrix optimized matrix class in some places.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219468/diff/1-2/

Comment 8

[Bas Schouten (:bas.schouten)]

Comment on attachment 8950200 [details]
Bug 1437492 - Part 1: Add a Matrix class with optimizations for simple matrices.

Patch changed extensively, please re-review.

Comment 9

[Bas Schouten (:bas.schouten)]

Comment on attachment 8950201 [details]
Bug 1437492 - Part 2: Based on profile data, use the simple-matrix optimized matrix class in some places.

Significant changes as I implemented your suggestions, please re-review.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8950200 [details]
Bug 1437492 - Part 1: Add a Matrix class with optimizations for simple matrices.

https://reviewboard.mozilla.org/r/219466/#review226302

::: gfx/2d/Matrix.h:2021
(Diff revision 2)
> +
> +    if (mType == MatrixType::Simple) {
> +      TargetPoint point = TransformPointSimple(aPoint);
> +      return Point4DTyped<TargetUnits, F>(point.x, point.y, 0, 1);
> +    }
> +    

Trailing whitespace, here and a few other places.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8950201 [details]
Bug 1437492 - Part 2: Based on profile data, use the simple-matrix optimized matrix class in some places.

https://reviewboard.mozilla.org/r/219468/#review226304

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b618a66fc348
Part 1: Add a Matrix class with optimizations for simple matrices. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/19d0b0f36cce
Part 2: Based on profile data, use the simple-matrix optimized matrix class in some places. r=mattwoodrow

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/b618a66fc348
https://hg.mozilla.org/mozilla-central/rev/19d0b0f36cce