828240 - Create a single nsDisplayTransform for children-only transforms

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Right now we create a separate nsDisplayTransform item for each direct child frame of an SVG frame that has a children-only transform (as returned by the HasChildrenOnlyTransform implementations). This means that we can end up creating a lot more nsDisplayTransform items than we should which in itself can be a perf hit (see second paragraph of bug 801949 comment 11). It also means that we end up doing a lot more matrix operations than we should.

Comment 1

[Jonathan Watt [:jwatt]]

The main issue that any patch will need to address is that nsDisplayTransform consumers assume that the item's transform applies to its mFrame. Probably all consumers will need to be taught the difference between nsDisplayTransforms that applies to its mFrame, and those that apply only to the children of mFrame.

Comment 2

[Jonathan Watt [:jwatt]]

One option for fixing this would be to have all children of nsSVGOuterSVGFrame/nsSVGInnerSVGFrame wrapped by an extra anonymous child (turning the actual children into grandchildren). That way only one nsDisplayTransform would be created for a viewBox on an <svg>, because there would only be one direct child of the <svg>'s frame.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: Fix for nsSVGOuterSVGFrame

I tried several different approaches to doing this. I initially thought I'd create a new nsDisplayTransformChildren and teach the bits of the code that need to handle transforms how to handle that too. Unfortunately that turned out to require a lot of (invasive) code and was really awkward to fit in. Having to change so many bits of the code just for this one element that can have children-only transforms seemed wrong.

Instead this patch takes the approach of applying the children-only transforms to the nsSVGOuterSVGFrame's anonymous child frame. That isn't ideal either since when nsDisplayTransform code applies the transform it will affect the position of the anonymous frame (the position being to account for the <svg>'s border and padding). Specifically it will scale that offset, and we need to avoid that. I tried various things that I thought should work such as:

* overriding nsIFrame::GetUsedMargin() (margins are not scaled by transforms)
* changing 'transform-box'/'transform-origin' in svg.css
* accounting for the offset in the children-only transform

All of these ended up interacting badly with other parts of the code.

The least hacky way I can find to do this is to allow nsDisplayTransform to scale the anonymous frame's position, but to incorporate an extra translation into the transform that we return in order to cancel out the scaling of the position.

Comment 4

[Robert Longson [:longsonr]]

Comment on attachment 8854660 [details] [diff] [review]
Fix for nsSVGOuterSVGFrame

>+    if (ownMatrix.HasNonTranslation()) {
>+      // The nsDisplayTransform code will apply this transform to our frame,
>+      // including to our frame position.  We don't want our frame position to
>+      // be scaled though, so we need to correct for that in the transform.
>+      // XXX Yeah, this is a bit hacky.
>+      CSSPoint pos = CSSPixel::FromAppUnits(GetPosition());
>+      CSSPoint scaledPos = CSSPoint(ownMatrix._11 * pos.x, ownMatrix._22 * pos.y);

will this work if the svg element transform is a skew or in fact anything other than a scale + translate?
We don't need to do scaledPos = ownMatrix * pos here? I.e account for any _12 and _21 parts?

>+      CSSPoint deltaPos = scaledPos - pos;
>+      ownMatrix *= gfxMatrix::Translation(-deltaPos.x, -deltaPos.y);
>+    }
>+
>+    *aOwnTransform = gfx::ToMatrix(ownMatrix);
>+  }
>+

Comment 5

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #4)
> Comment on attachment 8854660 [details] [diff] [review]
> Fix for nsSVGOuterSVGFrame
> 
> >+    if (ownMatrix.HasNonTranslation()) {
> >+      // The nsDisplayTransform code will apply this transform to our frame,
> >+      // including to our frame position.  We don't want our frame position to
> >+      // be scaled though, so we need to correct for that in the transform.
> >+      // XXX Yeah, this is a bit hacky.
> >+      CSSPoint pos = CSSPixel::FromAppUnits(GetPosition());
> >+      CSSPoint scaledPos = CSSPoint(ownMatrix._11 * pos.x, ownMatrix._22 * pos.y);
> 
> will this work if the svg element transform is a skew or in fact anything
> other than a scale + translate?
> We don't need to do scaledPos = ownMatrix * pos here? I.e account for any
> _12 and _21 parts?

The transform we're handling here is the children-only transform. So purely the viewBox, unless we're a root-<svg> in which case it could inclued currentScale/Translate. So there is only scale, and translation. And it's only the scale part that causes problems since border/padding offset and translation interact fine together.

Comment 6

[Robert Longson [:longsonr]]

Comment on attachment 8854660 [details] [diff] [review]
Fix for nsSVGOuterSVGFrame

>+
>+    if (ownMatrix.HasNonTranslation()) {
>+      // The nsDisplayTransform code will apply this transform to our frame,
>+      // including to our frame position.  We don't want our frame position to
>+      // be scaled though, so we need to correct for that in the transform.
>+      // XXX Yeah, this is a bit hacky.

Can we assert ownMatrix.IsRectilinear() somewhere in this method if this is the viewBox transform?

>+      CSSPoint pos = CSSPixel::FromAppUnits(GetPosition());
>+      CSSPoint scaledPos = CSSPoint(ownMatrix._11 * pos.x, ownMatrix._22 * pos.y);
>+      CSSPoint deltaPos = scaledPos - pos;
>+      ownMatrix *= gfxMatrix::Translation(-deltaPos.x, -deltaPos.y);
>+    }
>+
>+    *aOwnTransform = gfx::ToMatrix(ownMatrix);
>+  }
>+
>+  return true;
> }

r=longsonr with the assert added.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Add an IsRectilinear method to gfxMatrix

We should have this on gfxMatrix for parity with gfx::Matrix. I could probably just land this without review, but I guess I shouldn't.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4365ec0e81de
part 1 - Add an IsRectilinear method to gfxMatrix. r=longsonr
https://hg.mozilla.org/integration/mozilla-inbound/rev/0901f80a81db
part 2 - Create only one nsDisplayTransform for outer-<svg> children-only transforms. r=longsonr

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4365ec0e81de
https://hg.mozilla.org/mozilla-central/rev/0901f80a81db

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

this improved our tsvg benchmark:
== Change summary for alert #5875 (as of April 06 2017 08:08 UTC) ==

Improvements:

  9%  tsvgx summary windows7-32 pgo e10s     513.98 -> 468.79
  8%  tsvgx summary linux64 opt e10s         495.59 -> 454.09
  8%  tsvgx summary windows7-32 pgo          551.46 -> 506.63
  8%  tsvgx summary osx-10-10 opt e10s       498.6 -> 459.73
  8%  tsvgx summary windows7-32 opt e10s     556.46 -> 513.42
  8%  tsvgx summary osx-10-10 opt            554.6 -> 512.54
  7%  tsvgx summary linux64 pgo e10s         344.85 -> 319.2
  7%  tsvgx summary linux64 opt              546.02 -> 508.98
  6%  tsvgx summary windows7-32 opt          605.41 -> 568.06
  6%  tsvgx summary linux64 pgo              377.97 -> 354.89
  5%  tsvgx summary windows8-64 opt e10s     258.81 -> 245.46
  5%  tsvgx summary windows8-64 pgo e10s     232.05 -> 220.25
  4%  tsvgx summary windows8-64 opt          304.87 -> 292.48
  4%  tsvgx summary windows8-64 pgo          269.56 -> 259.76

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=5875

Comment 11

[Jonathan Watt [:jwatt]]

\o/

Comment 12

[Jet Villegas (inactive)]

Adding [qf] so this gets traced back from Quantum Flow perf measurements.