Avoid using forced layout to trigger paint invalidation for SVG containers

Avoid using forced layout to trigger paint invalidation for SVG containers

Currently, SVG containers in the LayoutObject hierarchy force layout of
their children if the transform changes. The main reason for this is to
trigger paint invalidation of the subtree. In some cases - changes to the
scale factor - there are other reasons to trigger layout, like computing
a new scale factor for <text> or re-layout nodes with non-scaling stroke.

Compute a "scale-factor change" in addition to the "transform change"
already computed, then use this new signal to determine if layout should
be forced for the subtree. Trigger paint invalidation using the
LayoutObject flags instead.

The downside to this is that paint invalidation will walk into "hidden"
containers which rarely require repaint (since they are not technically
visible). This will hopefully be rectified in a follow-up CL.

For the testcase from 603850, this essentially eliminates the cost of
layout (from ~350ms to ~0ms on authors machine; layout cost is related
to text metrics recalculation), bumping frame rate significantly.

BUG=603956, 603850
Committed: https://crrev.com/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785
Cr-Commit-Position: refs/heads/master@{#400950}

[fs, 2016-06-14 14:08:20]

Description was changed from

==========
Compute both scale-factor change and transform change

BUG=603956
==========

to

==========
Avoid using forced layout to trigger paint invalidation for SVG containers

Currently, SVG containers in the LayoutObject hierarchy force layout of
their children if the transform changes. The main reason for this is to
trigger paint invalidation of the subtree. In some cases - changes to the
scale factor - there are other reasons to trigger layout, like computing
a new scale factor for <text> or re-layout nodes with non-scaling stroke.

Compute a "scale-factor change" in addition to the "transform change"
already computed, then use this new signal to determine if layout should
be forced for the subtree. Trigger paint invalidation using the
LayoutObject flags instead.

The downside to this is that paint invalidation will walk into "hidden"
containers which rarely require repaint (since they are not technically
visible). This will hopefully be rectified in a follow-up CL.

For the testcase from 603850, this essentially eliminates the cost of
layout (from ~350ms to ~0ms on authors machine; layout cost is related
to text metrics recalculation), bumping frame rate significantly.

BUG=603956
==========

[fs, 2016-06-14 14:08:37]

Description was changed from

==========
Avoid using forced layout to trigger paint invalidation for SVG containers

Currently, SVG containers in the LayoutObject hierarchy force layout of
their children if the transform changes. The main reason for this is to
trigger paint invalidation of the subtree. In some cases - changes to the
scale factor - there are other reasons to trigger layout, like computing
a new scale factor for <text> or re-layout nodes with non-scaling stroke.

Compute a "scale-factor change" in addition to the "transform change"
already computed, then use this new signal to determine if layout should
be forced for the subtree. Trigger paint invalidation using the
LayoutObject flags instead.

The downside to this is that paint invalidation will walk into "hidden"
containers which rarely require repaint (since they are not technically
visible). This will hopefully be rectified in a follow-up CL.

For the testcase from 603850, this essentially eliminates the cost of
layout (from ~350ms to ~0ms on authors machine; layout cost is related
to text metrics recalculation), bumping frame rate significantly.

BUG=603956
==========

to

==========
Avoid using forced layout to trigger paint invalidation for SVG containers

Currently, SVG containers in the LayoutObject hierarchy force layout of
their children if the transform changes. The main reason for this is to
trigger paint invalidation of the subtree. In some cases - changes to the
scale factor - there are other reasons to trigger layout, like computing
a new scale factor for <text> or re-layout nodes with non-scaling stroke.

Compute a "scale-factor change" in addition to the "transform change"
already computed, then use this new signal to determine if layout should
be forced for the subtree. Trigger paint invalidation using the
LayoutObject flags instead.

The downside to this is that paint invalidation will walk into "hidden"
containers which rarely require repaint (since they are not technically
visible). This will hopefully be rectified in a follow-up CL.

For the testcase from 603850, this essentially eliminates the cost of
layout (from ~350ms to ~0ms on authors machine; layout cost is related
to text metrics recalculation), bumping frame rate significantly.

BUG=603956,603850
==========

[fs, 2016-06-14 15:16:38]

fs@opera.com changed reviewers:
+ chrishtr@chromium.org, pdr@chromium.org, wangxianzhu@chromium.org

[Xianzhu, 2016-06-14 16:52:37]

Is there try result of a patch set showing the change of test expectations? Or
please include local rebaseline on your platform in the patch.

The paint invalidation method lgtm (great).

Leaving review of svg transform/scale change logic to pdr@.

[fs, 2016-06-14 18:02:23]

On 2016/06/14 at 16:52:37, wangxianzhu wrote:
> Is there try result of a patch set showing the change of test expectations? Or
please include local rebaseline on your platform in the patch.

Yes,
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
is one example.

https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...

I saw that you just landed https://codereview.chromium.org/2061963002, so I'll
try to make sure to adjust my TestExpectations changes accordingly (and do
manual rebaselines.)

[pdr., 2016-06-15 12:15:17]

Super cool

https://codereview.chromium.org/1996543002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.h (right):

https://codereview.chromium.org/1996543002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.h:73: enum
TransformChangeFlags {
Returning just ScaleChange from LayoutSVGResourceMarker::calculateLocalTransform
might introduce bugs because the TransformChange bit will be 0.

WDYT about switching the logic and renaming ScaleChange to
ScaleKnownToBeUnchanged?

https://codereview.chromium.org/1996543002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/transforms/AffineTransform.cpp (right):

https://codereview.chromium.org/1996543002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/transforms/AffineTransform.cpp:73: return
m_transform[0] * m_transform[0] + m_transform[1] * m_transform[1];
A little out of scope for this patch, just noting because it seems odd: wouldn't
xScale be m_transform[0] and yScale be m_transform[3]? I.e., no need to include
the sheer components? Squaring could also introduce an edge-case bug when the
scale value is negative.

[fs, 2016-06-15 22:10:33]

https://codereview.chromium.org/1996543002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.h (right):

https://codereview.chromium.org/1996543002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.h:73: enum
TransformChangeFlags {
On 2016/06/15 at 12:15:17, pdr. wrote:
> Returning just ScaleChange from
LayoutSVGResourceMarker::calculateLocalTransform might introduce bugs because
the TransformChange bit will be 0.
> 
> WDYT about switching the logic and renaming ScaleChange to
ScaleKnownToBeUnchanged?

Yes, not too happy about this bit (no pun intended). Returning a "negative" bit
might work for now.

https://codereview.chromium.org/1996543002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/transforms/AffineTransform.cpp (right):

https://codereview.chromium.org/1996543002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/transforms/AffineTransform.cpp:73: return
m_transform[0] * m_transform[0] + m_transform[1] * m_transform[1];
On 2016/06/15 at 12:15:17, pdr. wrote:
> A little out of scope for this patch, just noting because it seems odd:
wouldn't xScale be m_transform[0] and yScale be m_transform[3]? I.e., no need to
include the sheer components? Squaring could also introduce an edge-case bug
when the scale value is negative.

It's more like "x/y basis magnitude" than "x/y scale". This is what's being used
"downstream" in clients though. (And I think the shearing component will affect
non-scaling stroke?) Similarly for negative scales (== flips), since those won't
affect the size of the descendants.

[fs, 2016-06-20 11:48:49]

https://codereview.chromium.org/1996543002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.h (right):

https://codereview.chromium.org/1996543002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.h:73: enum
TransformChangeFlags {
On 2016/06/15 at 22:10:33, fs wrote:
> On 2016/06/15 at 12:15:17, pdr. wrote:
> > Returning just ScaleChange from
LayoutSVGResourceMarker::calculateLocalTransform might introduce bugs because
the TransformChange bit will be 0.
> > 
> > WDYT about switching the logic and renaming ScaleChange to
ScaleKnownToBeUnchanged?
> 
> Yes, not too happy about this bit (no pun intended). Returning a "negative"
bit might work for now.

Rewrote it to use an (ordered) enumeration instead. Maybe a little better, WDYT?
PTAL.

[pdr., 2016-06-21 00:13:04]

LGTM with or without a comment.

https://codereview.chromium.org/1996543002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.h (right):

https://codereview.chromium.org/1996543002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.h:73: enum class
TransformChange {
WDYT of adding a small comment here? The enum approach here is the most
maintainable approach I've seen yet but I can see us scratching our heads in the
future about whether checking scale is required.

Trial balloon:
// TransformChange is used to optimize cases where the scale is known to
// static (see: LayoutSVGContainer::layout). Full can be used in the general
// case when the scale change is unknown, or known to change.

[fs, 2016-06-21 08:24:45]

https://codereview.chromium.org/1996543002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.h (right):

https://codereview.chromium.org/1996543002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.h:73: enum class
TransformChange {
On 2016/06/21 at 00:13:04, pdr. wrote:
> WDYT of adding a small comment here? The enum approach here is the most
maintainable approach I've seen yet but I can see us scratching our heads in the
future about whether checking scale is required.
> 
> Trial balloon:
> // TransformChange is used to optimize cases where the scale is known to
> // static (see: LayoutSVGContainer::layout). Full can be used in the general
> // case when the scale change is unknown, or known to change.

Comment added.

[fs, 2016-06-21 11:04:45]

The CQ bit was checked by fs@opera.com

[fs, 2016-06-21 11:04:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1996543002/#ps140001
(title: "TE updates")

[commit-bot: I haz the power, 2016-06-21 11:05:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996543002/140001

[commit-bot: I haz the power, 2016-06-21 11:11:42]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-21 11:13:00]

Description was changed from

==========
Avoid using forced layout to trigger paint invalidation for SVG containers

Currently, SVG containers in the LayoutObject hierarchy force layout of
their children if the transform changes. The main reason for this is to
trigger paint invalidation of the subtree. In some cases - changes to the
scale factor - there are other reasons to trigger layout, like computing
a new scale factor for <text> or re-layout nodes with non-scaling stroke.

Compute a "scale-factor change" in addition to the "transform change"
already computed, then use this new signal to determine if layout should
be forced for the subtree. Trigger paint invalidation using the
LayoutObject flags instead.

The downside to this is that paint invalidation will walk into "hidden"
containers which rarely require repaint (since they are not technically
visible). This will hopefully be rectified in a follow-up CL.

For the testcase from 603850, this essentially eliminates the cost of
layout (from ~350ms to ~0ms on authors machine; layout cost is related
to text metrics recalculation), bumping frame rate significantly.

BUG=603956,603850
==========

to

==========
Avoid using forced layout to trigger paint invalidation for SVG containers

Currently, SVG containers in the LayoutObject hierarchy force layout of
their children if the transform changes. The main reason for this is to
trigger paint invalidation of the subtree. In some cases - changes to the
scale factor - there are other reasons to trigger layout, like computing
a new scale factor for <text> or re-layout nodes with non-scaling stroke.

Compute a "scale-factor change" in addition to the "transform change"
already computed, then use this new signal to determine if layout should
be forced for the subtree. Trigger paint invalidation using the
LayoutObject flags instead.

The downside to this is that paint invalidation will walk into "hidden"
containers which rarely require repaint (since they are not technically
visible). This will hopefully be rectified in a follow-up CL.

For the testcase from 603850, this essentially eliminates the cost of
layout (from ~350ms to ~0ms on authors machine; layout cost is related
to text metrics recalculation), bumping frame rate significantly.

BUG=603956,603850

Committed: https://crrev.com/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785
Cr-Commit-Position: refs/heads/master@{#400950}
==========

[commit-bot: I haz the power, 2016-06-21 11:13:01]

Patchset 8 (id:??) landed as
https://crrev.com/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785
Cr-Commit-Position: refs/heads/master@{#400950}