New issue
Advanced search Search tips

Issue 603956 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Optimize the "panning (w/ text)" use case

Project Member Reported by f...@opera.com, Apr 15 2016

Issue description

(Warning: Rant ahead. Reduce speed and read signs carefully.)

Based on  issue 589525  (w/ duplicates) it seems there could be value looking at the "panning" use case - which seems fairly common among these reports. "Panning" in this context would generally be "changing the translation component of the transform on a container (<g>)".

I profiled the TC provided in  issue 603850 , which showed a large amount of time was spent computing shaped metrics. Not one time but three:

  Via LayoutSVGInlineText::updateMetricsList (no surprise)
  Via SVGTextLayoutEngine::computeCurrentFragmentMetrics (not too surprising)
  Via LineBreaker::nextLineBreak (possibly the most surprising)

The primary cause for these are that the "transform to root" changed, by means of the transform update on a container (LayoutSVGTransformableContainer). Additionally there also the issue that a container that "need layout" in turn will force layout of all its children.
The "transform to root" signal is used to signal that metrics needs to be recomputed (because the resolved font-scale may have changed.) When "panning" by changing a translation the font-scale should not change though.
The use for the other "signal" alludes me at the moment...

So it seems it should be possible to "eliminate" the "transform changed" signal by detecting a translating update (or more generally a same-scale change).

Haven't figured out the other issue yet, but maybe it would be possible to make use of more of the needs...Layout flags in LayoutObject.
 

Comment 1 by pdr@chromium.org, Apr 15 2016

Where do I signup for more rants!?

Text measurement is aggressively cached because HTML has the same "shape 3x per layout" problem. Are we actually paying the cost to fully re-shape metrics three times, or is it 1x shaping + 2 cached lookups? Even if it is 1x shaping + 2 cached lookups, are the lookups fast enough?

I like your idea of not tracking the full transform to root change (maybe transformScaleToRootChanged?), and of not forcing a layout for translation changes (though that may expose some paint invalidation bugs).

We should add this to the list of things to add to https://crbug.com/595235 too.

Comment 2 by f...@opera.com, Apr 17 2016

I just recently learnt that caching doesn't work for "words" > 15 characters. That likely accounts for some of the above. Unnecessary work is unnecessary still though.
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f7ffad51fe92305e463f4af147f67a04f8b5b1a0

commit f7ffad51fe92305e463f4af147f67a04f8b5b1a0
Author: fs <fs@opera.com>
Date: Tue Apr 19 19:40:57 2016

Get rid of SVGLayoutSupport::filtersForceContainerLayout

Save the "has filter resource" part, and rename it to hasFilterResource.
Hoist the normalChildNeedsLayout() part into the callers together with
the comment. This makes the condition for forcing layout of children
of an <svg> (outermost) root or a container more obvious.

Also update a few places to use the new hasFilterResource helper.

BUG= 603956 

Review URL: https://codereview.chromium.org/1899243002

Cr-Commit-Position: refs/heads/master@{#388284}

[modify] https://crrev.com/f7ffad51fe92305e463f4af147f67a04f8b5b1a0/third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp
[modify] https://crrev.com/f7ffad51fe92305e463f4af147f67a04f8b5b1a0/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp
[modify] https://crrev.com/f7ffad51fe92305e463f4af147f67a04f8b5b1a0/third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp
[modify] https://crrev.com/f7ffad51fe92305e463f4af147f67a04f8b5b1a0/third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.h
[modify] https://crrev.com/f7ffad51fe92305e463f4af147f67a04f8b5b1a0/third_party/WebKit/Source/core/paint/SVGRootPainter.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/51f58998a41a2ac0d379701b3f53095c1359092e

commit 51f58998a41a2ac0d379701b3f53095c1359092e
Author: fs <fs@opera.com>
Date: Tue Apr 19 20:17:09 2016

Hoist transformToRootChanged() out of SVGLayoutSupport::layoutChildren

This function is really only relevant for LayoutSVG*Containers, and
hence has a stronger logical tie to that part of the hierarchy. For
LayoutSVGRoot it will always return false.
Also rename the |selfNeedsLayout| argument to |forceLayout|, and the
(somewhat) corresponding local variable |forceLayout] to
|forceChildLayout|.
Eliminate an unnecessary virtual call in transformToRootChanged().
Add const qualifier to didTransformToRootUpdate().

BUG= 603956 

Review URL: https://codereview.chromium.org/1897263002

Cr-Commit-Position: refs/heads/master@{#388292}

[modify] https://crrev.com/51f58998a41a2ac0d379701b3f53095c1359092e/third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp
[modify] https://crrev.com/51f58998a41a2ac0d379701b3f53095c1359092e/third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.h
[modify] https://crrev.com/51f58998a41a2ac0d379701b3f53095c1359092e/third_party/WebKit/Source/core/layout/svg/LayoutSVGHiddenContainer.cpp
[modify] https://crrev.com/51f58998a41a2ac0d379701b3f53095c1359092e/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp
[modify] https://crrev.com/51f58998a41a2ac0d379701b3f53095c1359092e/third_party/WebKit/Source/core/layout/svg/LayoutSVGTransformableContainer.h
[modify] https://crrev.com/51f58998a41a2ac0d379701b3f53095c1359092e/third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.h
[modify] https://crrev.com/51f58998a41a2ac0d379701b3f53095c1359092e/third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp
[modify] https://crrev.com/51f58998a41a2ac0d379701b3f53095c1359092e/third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.h

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 19 2016

Project Member

Comment 7 by bugdroid1@chromium.org, Apr 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/641397ee2d4f59b3249329030d96a2177c918db1

commit 641397ee2d4f59b3249329030d96a2177c918db1
Author: fs <fs@opera.com>
Date: Wed Apr 20 16:32:10 2016

Move the m_didTransformToRootUpdate flag to LayoutSVGContainer

By pushing the update of the m_didTransformToRootUpdate flag out of the
various calculateLocalTransform() implementations, we both get
implementations of those methods that are more to the point, and expose
the redundant calls to SVGLayoutSupport::transformToRootChanged().
This also means that didTransformToRootUpdate() is devirtualized,
although it was never called "virtually" before either.
Also turn m_needsBoundariesUpdate into a single-bit flag.

BUG= 603956 

Review URL: https://codereview.chromium.org/1904683002

Cr-Commit-Position: refs/heads/master@{#388516}

[modify] https://crrev.com/641397ee2d4f59b3249329030d96a2177c918db1/third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp
[modify] https://crrev.com/641397ee2d4f59b3249329030d96a2177c918db1/third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.h
[modify] https://crrev.com/641397ee2d4f59b3249329030d96a2177c918db1/third_party/WebKit/Source/core/layout/svg/LayoutSVGTransformableContainer.cpp
[modify] https://crrev.com/641397ee2d4f59b3249329030d96a2177c918db1/third_party/WebKit/Source/core/layout/svg/LayoutSVGTransformableContainer.h
[modify] https://crrev.com/641397ee2d4f59b3249329030d96a2177c918db1/third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.cpp
[modify] https://crrev.com/641397ee2d4f59b3249329030d96a2177c918db1/third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.h

Project Member

Comment 8 by bugdroid1@chromium.org, Apr 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b035163d0abf61e489593d83b468e6988317d8c2

commit b035163d0abf61e489593d83b468e6988317d8c2
Author: fs <fs@opera.com>
Date: Wed Apr 20 21:28:33 2016

Drop transform-change propagation from LayoutSVGHiddenContainer::layout

No real transform changes should be detected in LayoutSVGHiddenContainer
since it's used for things making up (==roots of) "isolated subtrees".

BUG= 603956 

Review URL: https://codereview.chromium.org/1905533003

Cr-Commit-Position: refs/heads/master@{#388578}

[modify] https://crrev.com/b035163d0abf61e489593d83b468e6988317d8c2/third_party/WebKit/Source/core/layout/svg/LayoutSVGHiddenContainer.cpp

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b6a48975dfe23cf5721c0712052057112055cabb

commit b6a48975dfe23cf5721c0712052057112055cabb
Author: fs <fs@opera.com>
Date: Fri Apr 22 23:00:02 2016

Remove unnecessary full-subtree layouts for filtered SVG containers

The presence of a filter on a container (or the SVG root) would force
a layout of the entire subtree if any child needed a layout. This used
to serve the purpose of making sure that the entire subtree would get
repainted (an artifact of how 'filter' is handled on SVG elements.)
With the current paint invalidation system, this should however make
no practical difference, since the layout of the subtree will only
end up marking descendant LayoutObjects as "maybe needing paint
invalidation" - which the ones that didn't actually change don't - and
hence for these nothing will be invalidated.
The container itself will be invalidated via the call to
SVGResourcesCache::clientLayoutChanged, which will mark it for paint
invalidation if it has instantiated a filter resource.

BUG= 603956 

Review URL: https://codereview.chromium.org/1907273002

Cr-Commit-Position: refs/heads/master@{#389283}

[modify] https://crrev.com/b6a48975dfe23cf5721c0712052057112055cabb/third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp
[modify] https://crrev.com/b6a48975dfe23cf5721c0712052057112055cabb/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp

Project Member

Comment 11 by bugdroid1@chromium.org, Apr 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fe8421b9573ca696e596248811edf0a5d23fc191

commit fe8421b9573ca696e596248811edf0a5d23fc191
Author: fs <fs@opera.com>
Date: Mon Apr 25 01:37:22 2016

Order bounds update correctly for LayoutSVGShape and LayoutSVGImage

Since SVGResourcesCache::clientLayoutChanged can set the
m_needsBoundariesUpdate flag, make sure to check it, and act, after
that call. This ensures that state is consistent. when layout() return.
Also add a comment about the delicate situation in
LayoutSVGShape::layout wrt shape vs. bounds update. Additionally rename
the local variable used to notify the parent that its bounds needs to be
updated to |updateParentBoundaries|.

BUG= 603956 

Review URL: https://codereview.chromium.org/1907333002

Cr-Commit-Position: refs/heads/master@{#389410}

[modify] https://crrev.com/fe8421b9573ca696e596248811edf0a5d23fc191/third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp
[modify] https://crrev.com/fe8421b9573ca696e596248811edf0a5d23fc191/third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp
[modify] https://crrev.com/fe8421b9573ca696e596248811edf0a5d23fc191/third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp
[modify] https://crrev.com/fe8421b9573ca696e596248811edf0a5d23fc191/third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.h

Project Member

Comment 12 by bugdroid1@chromium.org, Apr 25 2016

Labels: merge-merged-2716
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fe8421b9573ca696e596248811edf0a5d23fc191

commit fe8421b9573ca696e596248811edf0a5d23fc191
Author: fs <fs@opera.com>
Date: Mon Apr 25 01:37:22 2016

Order bounds update correctly for LayoutSVGShape and LayoutSVGImage

Since SVGResourcesCache::clientLayoutChanged can set the
m_needsBoundariesUpdate flag, make sure to check it, and act, after
that call. This ensures that state is consistent. when layout() return.
Also add a comment about the delicate situation in
LayoutSVGShape::layout wrt shape vs. bounds update. Additionally rename
the local variable used to notify the parent that its bounds needs to be
updated to |updateParentBoundaries|.

BUG= 603956 

Review URL: https://codereview.chromium.org/1907333002

Cr-Commit-Position: refs/heads/master@{#389410}

[modify] https://crrev.com/fe8421b9573ca696e596248811edf0a5d23fc191/third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp
[modify] https://crrev.com/fe8421b9573ca696e596248811edf0a5d23fc191/third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp
[modify] https://crrev.com/fe8421b9573ca696e596248811edf0a5d23fc191/third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp
[modify] https://crrev.com/fe8421b9573ca696e596248811edf0a5d23fc191/third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.h

Project Member

Comment 13 by bugdroid1@chromium.org, Apr 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6260c0fbdec366ad2c78c3ba5cb9be297859dbef

commit 6260c0fbdec366ad2c78c3ba5cb9be297859dbef
Author: fs <fs@opera.com>
Date: Tue Apr 26 10:34:40 2016

Don't force layout of descendants of SVG containers needing self-layout

There isn't much "layout" required for a generic SVG container - it is
sensitive to changes to its descendants and any impact a filter it
itself references has. We would however always relayout the entire
subtree of a container that had been marked as needing layout.
Like he code removed by https://codereview.chromium.org/1907273002, this
remaining part of the condition is primarily a remnant of the old way of
performing paint invalidation.
This CL changes to not force layout unconditionally based on the
self-needs-layout flag, but instead relies on the scale-factor-changed
flag (and of course specific marking of descendants) to handle this job.
To achieve this, rudimentary scale-factor-changed "detection" is added
to LayoutSVGRoot, as well as code to propagate this state to
descendants. LayoutSVGResourceMarker is modified in a similar fashion.
This is needed to counter-act the removal of the forced layout, since
previously this would propagate to descendants (unconditionally) by
forcing layout in both the root and containers. This effect is now
achieved (for the root) by using the scale-factor-changed signal
instead. This signal will be improved in future CLs.

BUG= 603956 

Review URL: https://codereview.chromium.org/1920833002

Cr-Commit-Position: refs/heads/master@{#389742}

[modify] https://crrev.com/6260c0fbdec366ad2c78c3ba5cb9be297859dbef/third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp
[modify] https://crrev.com/6260c0fbdec366ad2c78c3ba5cb9be297859dbef/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.cpp
[modify] https://crrev.com/6260c0fbdec366ad2c78c3ba5cb9be297859dbef/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.h
[modify] https://crrev.com/6260c0fbdec366ad2c78c3ba5cb9be297859dbef/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp
[modify] https://crrev.com/6260c0fbdec366ad2c78c3ba5cb9be297859dbef/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.h
[modify] https://crrev.com/6260c0fbdec366ad2c78c3ba5cb9be297859dbef/third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp

Cc: e...@chromium.org
Re: line breaking I wanted to make sure you have seen eae@'s line breaking design document. I believe he is starting work on this shortly. He sent this to layout-dev@ end of March.

https://docs.google.com/document/d/1eMTBKTnWEMDu00uS2p8Xj-l9Pk7Kf0q5y3FbcCrWYjU/edit

Comment 15 by f...@opera.com, May 24 2016

Yepp, seen it, read it, looking forward to it =)
Project Member

Comment 16 by bugdroid1@chromium.org, Jun 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7505794623b19588cd5a77236d4baa21e41c0864

commit 7505794623b19588cd5a77236d4baa21e41c0864
Author: fs <fs@opera.com>
Date: Tue Jun 14 03:29:03 2016

Rename StyleLayoutData to StyleGeometryData

The term 'geometry' better matches what is stored here (and is also more
in line with the SVG spec chapter [1] that defines most of the properties
here.)
Rename the SVGComputedStyle::layout field to 'geometry' to match.

This is a mechanical rename-only CL, with the exception of some
additional whitespace fixups.

[1] https://svgwg.org/svg2-draft/geometry.html ("Geometry Properties")

BUG= 603956 

Review-Url: https://codereview.chromium.org/2066563002
Cr-Commit-Position: refs/heads/master@{#399649}

[modify] https://crrev.com/7505794623b19588cd5a77236d4baa21e41c0864/third_party/WebKit/Source/core/style/SVGComputedStyle.cpp
[modify] https://crrev.com/7505794623b19588cd5a77236d4baa21e41c0864/third_party/WebKit/Source/core/style/SVGComputedStyle.h
[modify] https://crrev.com/7505794623b19588cd5a77236d4baa21e41c0864/third_party/WebKit/Source/core/style/SVGComputedStyleDefs.cpp
[modify] https://crrev.com/7505794623b19588cd5a77236d4baa21e41c0864/third_party/WebKit/Source/core/style/SVGComputedStyleDefs.h

Project Member

Comment 17 by bugdroid1@chromium.org, Jun 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/30770a70834c73670884f0de91bb7624df0ba003

commit 30770a70834c73670884f0de91bb7624df0ba003
Author: fs <fs@opera.com>
Date: Tue Jun 14 21:48:21 2016

Remove redundant "layout size changed" state from LayoutSVGRoot

In LayoutSVGRoot::layout, two slightly different "layout size changed"
values are computed - one which is used for propagation to children
via SVGLayoutSupport::layoutSizeOfNearestViewportChanged
(|m_isLayoutSizeChanged|), and one which is used to mark direct
descendant children (local |layoutSizeChanged|).
Ultimately their use is the same though, so only using the more narrow
predicate for both of these cases should yield the same result.
It also has the side-effect of making it more obvious that changes to
layout-size is only of interest when there exist clients of the SVG
root that have relative lengths.

BUG= 603956 

Review-Url: https://codereview.chromium.org/2065093002
Cr-Commit-Position: refs/heads/master@{#399791}

[modify] https://crrev.com/30770a70834c73670884f0de91bb7624df0ba003/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp

Project Member

Comment 18 by bugdroid1@chromium.org, Jun 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6d58939f2253f0686141508c07c666a092e5b2a2

commit 6d58939f2253f0686141508c07c666a092e5b2a2
Author: fs <fs@opera.com>
Date: Tue Jun 14 22:10:24 2016

Remove redundant isLayoutSizeChanged check in LayoutSVGText::layout

SVGLayoutSupport::layoutChildren takes care to propagate the needs for
metrics updates via the |screenScalingFactorChanged| and
|layoutSizeChanged| arguments.

BUG= 603956 

Review-Url: https://codereview.chromium.org/2061793003
Cr-Commit-Position: refs/heads/master@{#399796}

[modify] https://crrev.com/6d58939f2253f0686141508c07c666a092e5b2a2/third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp

Project Member

Comment 19 by bugdroid1@chromium.org, Jun 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7505794623b19588cd5a77236d4baa21e41c0864

commit 7505794623b19588cd5a77236d4baa21e41c0864
Author: fs <fs@opera.com>
Date: Tue Jun 14 03:29:03 2016

Rename StyleLayoutData to StyleGeometryData

The term 'geometry' better matches what is stored here (and is also more
in line with the SVG spec chapter [1] that defines most of the properties
here.)
Rename the SVGComputedStyle::layout field to 'geometry' to match.

This is a mechanical rename-only CL, with the exception of some
additional whitespace fixups.

[1] https://svgwg.org/svg2-draft/geometry.html ("Geometry Properties")

BUG= 603956 

Review-Url: https://codereview.chromium.org/2066563002
Cr-Commit-Position: refs/heads/master@{#399649}

[modify] https://crrev.com/7505794623b19588cd5a77236d4baa21e41c0864/third_party/WebKit/Source/core/style/SVGComputedStyle.cpp
[modify] https://crrev.com/7505794623b19588cd5a77236d4baa21e41c0864/third_party/WebKit/Source/core/style/SVGComputedStyle.h
[modify] https://crrev.com/7505794623b19588cd5a77236d4baa21e41c0864/third_party/WebKit/Source/core/style/SVGComputedStyleDefs.cpp
[modify] https://crrev.com/7505794623b19588cd5a77236d4baa21e41c0864/third_party/WebKit/Source/core/style/SVGComputedStyleDefs.h

Project Member

Comment 20 by bugdroid1@chromium.org, Jun 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/30770a70834c73670884f0de91bb7624df0ba003

commit 30770a70834c73670884f0de91bb7624df0ba003
Author: fs <fs@opera.com>
Date: Tue Jun 14 21:48:21 2016

Remove redundant "layout size changed" state from LayoutSVGRoot

In LayoutSVGRoot::layout, two slightly different "layout size changed"
values are computed - one which is used for propagation to children
via SVGLayoutSupport::layoutSizeOfNearestViewportChanged
(|m_isLayoutSizeChanged|), and one which is used to mark direct
descendant children (local |layoutSizeChanged|).
Ultimately their use is the same though, so only using the more narrow
predicate for both of these cases should yield the same result.
It also has the side-effect of making it more obvious that changes to
layout-size is only of interest when there exist clients of the SVG
root that have relative lengths.

BUG= 603956 

Review-Url: https://codereview.chromium.org/2065093002
Cr-Commit-Position: refs/heads/master@{#399791}

[modify] https://crrev.com/30770a70834c73670884f0de91bb7624df0ba003/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp

Project Member

Comment 21 by bugdroid1@chromium.org, Jun 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6d58939f2253f0686141508c07c666a092e5b2a2

commit 6d58939f2253f0686141508c07c666a092e5b2a2
Author: fs <fs@opera.com>
Date: Tue Jun 14 22:10:24 2016

Remove redundant isLayoutSizeChanged check in LayoutSVGText::layout

SVGLayoutSupport::layoutChildren takes care to propagate the needs for
metrics updates via the |screenScalingFactorChanged| and
|layoutSizeChanged| arguments.

BUG= 603956 

Review-Url: https://codereview.chromium.org/2061793003
Cr-Commit-Position: refs/heads/master@{#399796}

[modify] https://crrev.com/6d58939f2253f0686141508c07c666a092e5b2a2/third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp

Project Member

Comment 22 by bugdroid1@chromium.org, Jun 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7534b63d4979b824b1c3705d63a856028ff90fc5

commit 7534b63d4979b824b1c3705d63a856028ff90fc5
Author: fs <fs@opera.com>
Date: Fri Jun 17 10:08:48 2016

Revert of Remove redundant "layout size changed" state from LayoutSVGRoot (patchset #1 id:1 of https://codereview.chromium.org/2065093002/ )

Reason for revert:
Possible cause of  crbug.com/620228 

Original issue's description:
> Remove redundant "layout size changed" state from LayoutSVGRoot
>
> In LayoutSVGRoot::layout, two slightly different "layout size changed"
> values are computed - one which is used for propagation to children
> via SVGLayoutSupport::layoutSizeOfNearestViewportChanged
> (|m_isLayoutSizeChanged|), and one which is used to mark direct
> descendant children (local |layoutSizeChanged|).
> Ultimately their use is the same though, so only using the more narrow
> predicate for both of these cases should yield the same result.
> It also has the side-effect of making it more obvious that changes to
> layout-size is only of interest when there exist clients of the SVG
> root that have relative lengths.
>
> BUG= 603956 
>
> Committed: https://crrev.com/30770a70834c73670884f0de91bb7624df0ba003
> Cr-Commit-Position: refs/heads/master@{#399791}

TBR=pdr@chromium.org,schenney@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 603956 

Review-Url: https://codereview.chromium.org/2071953004
Cr-Commit-Position: refs/heads/master@{#400411}

[modify] https://crrev.com/7534b63d4979b824b1c3705d63a856028ff90fc5/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp

Project Member

Comment 23 by bugdroid1@chromium.org, Jun 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785

commit 44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785
Author: fs <fs@opera.com>
Date: Tue Jun 21 11:11:21 2016

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 

Review-Url: https://codereview.chromium.org/1996543002
Cr-Commit-Position: refs/heads/master@{#400950}

[modify] https://crrev.com/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785/third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp
[modify] https://crrev.com/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785/third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.h
[modify] https://crrev.com/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.cpp
[modify] https://crrev.com/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.h
[modify] https://crrev.com/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785/third_party/WebKit/Source/core/layout/svg/LayoutSVGTransformableContainer.cpp
[modify] https://crrev.com/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785/third_party/WebKit/Source/core/layout/svg/LayoutSVGTransformableContainer.h
[modify] https://crrev.com/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785/third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.cpp
[modify] https://crrev.com/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785/third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.h
[modify] https://crrev.com/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785/third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp
[modify] https://crrev.com/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785/third_party/WebKit/Source/platform/transforms/AffineTransform.cpp
[modify] https://crrev.com/44f1431b20c16d8f8da0ce8ff7bbf2adddcdd785/third_party/WebKit/Source/platform/transforms/AffineTransform.h

Project Member

Comment 24 by bugdroid1@chromium.org, Jun 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc555750635d72734566153423baa67e240e12d4

commit cc555750635d72734566153423baa67e240e12d4
Author: Rebaseline Bot <blink-rebaseline-bot@chromium.org>
Date: Tue Jun 21 12:30:04 2016

Auto-rebaseline for r400950

https://chromium.googlesource.com/chromium/src/+/44f1431b2

BUG= 603956 
TBR=fs@opera.com

Review URL: https://codereview.chromium.org/2081983002 .

Cr-Commit-Position: refs/heads/master@{#400966}

[modify] https://crrev.com/cc555750635d72734566153423baa67e240e12d4/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/cc555750635d72734566153423baa67e240e12d4/third_party/WebKit/LayoutTests/platform/linux/svg/repaint/image-with-clip-path-expected.txt
[modify] https://crrev.com/cc555750635d72734566153423baa67e240e12d4/third_party/WebKit/LayoutTests/platform/linux/svg/repaint/inner-svg-change-viewPort-relative-expected.txt
[modify] https://crrev.com/cc555750635d72734566153423baa67e240e12d4/third_party/WebKit/LayoutTests/platform/linux/svg/text/text-rescale-expected.txt
[modify] https://crrev.com/cc555750635d72734566153423baa67e240e12d4/third_party/WebKit/LayoutTests/platform/mac/svg/text/text-rescale-expected.txt
[modify] https://crrev.com/cc555750635d72734566153423baa67e240e12d4/third_party/WebKit/LayoutTests/platform/win/svg/text/text-rescale-expected.txt
[modify] https://crrev.com/cc555750635d72734566153423baa67e240e12d4/third_party/WebKit/LayoutTests/svg/custom/resource-client-removal-expected.txt
[modify] https://crrev.com/cc555750635d72734566153423baa67e240e12d4/third_party/WebKit/LayoutTests/svg/repaint/image-with-clip-path-expected.txt
[modify] https://crrev.com/cc555750635d72734566153423baa67e240e12d4/third_party/WebKit/LayoutTests/svg/repaint/inner-svg-change-viewPort-relative-expected.txt

Project Member

Comment 25 by bugdroid1@chromium.org, Jun 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2fd756f78f5d05f69bd298515bb7eff07f500b6a

commit 2fd756f78f5d05f69bd298515bb7eff07f500b6a
Author: fs <fs@opera.com>
Date: Tue Jun 21 14:28:53 2016

Reland of "Remove redundant "layout size changed" state from LayoutSVGRoot"

In LayoutSVGRoot::layout, two slightly different "layout size changed"
values are computed - one which is used for propagation to children
via SVGLayoutSupport::layoutSizeOfNearestViewportChanged
(|m_isLayoutSizeChanged|), and one which is used to mark direct
descendant children (local |layoutSizeChanged|).
Ultimately their use is the same though, so only using the more narrow
predicate for both of these cases should yield the same result.
It also has the side-effect of making it more obvious that changes to
layout-size is only of interest when there exist clients of the SVG
root that have relative lengths.

BUG= 603956 

Review-Url: https://codereview.chromium.org/2065093002
Cr-Commit-Position: refs/heads/master@{#400987}

[modify] https://crrev.com/2fd756f78f5d05f69bd298515bb7eff07f500b6a/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp

Project Member

Comment 26 by bugdroid1@chromium.org, Jun 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1826b1322e40dcb8038ccfe970206345ee22d2d6

commit 1826b1322e40dcb8038ccfe970206345ee22d2d6
Author: fs <fs@opera.com>
Date: Tue Jun 21 21:19:12 2016

Common up SVG transform "change detection" (classification)

This moves the transform change classification to a helper class
(SVGTransformChangeDetector) and move
LayoutSVGContainer::TransformChange along with it, renaming it to
SVGTransformChange.

BUG= 603956 

Review-Url: https://codereview.chromium.org/2086583004
Cr-Commit-Position: refs/heads/master@{#401111}

[modify] https://crrev.com/1826b1322e40dcb8038ccfe970206345ee22d2d6/third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp
[modify] https://crrev.com/1826b1322e40dcb8038ccfe970206345ee22d2d6/third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.h
[modify] https://crrev.com/1826b1322e40dcb8038ccfe970206345ee22d2d6/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.cpp
[modify] https://crrev.com/1826b1322e40dcb8038ccfe970206345ee22d2d6/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.h
[modify] https://crrev.com/1826b1322e40dcb8038ccfe970206345ee22d2d6/third_party/WebKit/Source/core/layout/svg/LayoutSVGTransformableContainer.cpp
[modify] https://crrev.com/1826b1322e40dcb8038ccfe970206345ee22d2d6/third_party/WebKit/Source/core/layout/svg/LayoutSVGTransformableContainer.h
[modify] https://crrev.com/1826b1322e40dcb8038ccfe970206345ee22d2d6/third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.cpp
[modify] https://crrev.com/1826b1322e40dcb8038ccfe970206345ee22d2d6/third_party/WebKit/Source/core/layout/svg/LayoutSVGViewportContainer.h
[modify] https://crrev.com/1826b1322e40dcb8038ccfe970206345ee22d2d6/third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.h

Project Member

Comment 28 by bugdroid1@chromium.org, Dec 10 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a229e828a692a3ba58fa5136173f81df1ac8b327

commit a229e828a692a3ba58fa5136173f81df1ac8b327
Author: fs <fs@opera.com>
Date: Sat Dec 10 10:35:32 2016

Strength-reduce the "scale-factor changed" condition in LayoutSVGRoot

Spend some cycles examining the difference between the old and new
local-to-border-box transforms, and only signal scale-factor changes
if that part of the transform changed.
This also means that we now detect changes scale that we previously
didn't (like [1].)

[1] paint/invalidation/svg/absolute-sized-content-with-resources.xhtml

BUG= 603956 ,664961

Review-Url: https://codereview.chromium.org/2559123003
Cr-Commit-Position: refs/heads/master@{#437767}

[modify] https://crrev.com/a229e828a692a3ba58fa5136173f81df1ac8b327/third_party/WebKit/LayoutTests/paint/invalidation/svg/absolute-sized-content-with-resources-expected.txt
[modify] https://crrev.com/a229e828a692a3ba58fa5136173f81df1ac8b327/third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/svg/absolute-sized-content-with-resources-expected.txt
[modify] https://crrev.com/a229e828a692a3ba58fa5136173f81df1ac8b327/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp
[modify] https://crrev.com/a229e828a692a3ba58fa5136173f81df1ac8b327/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.h

Comment 29 by f...@opera.com, Dec 12 2016

Owner: f...@opera.com
Status: Fixed (was: Available)
I think I've now landed everything I intended to land here. Work will continue via issue 664961 though.

Sign in to add a comment