Remove the SVG paint culling optimization

Remove the SVG paint culling optimization

This patch removes the paint culling optimization added in [1] which is
not as useful as it once was. We now use Skia which culls internally
and the overhead of checking for culling in SVG makes this optimization
a small loss in terms of performance on our microbenchmarks [2].

The primary change in this patch is to remove the updating and checking
of PaintInfo.rect within the SVG painting code. For ForeignObject and
Text, an adjustment of the PaintInfo rect is required to avoid clipping
in the text painting code. This adjustment is implemented by caching
the paint invalidation transform. For filters in SVG, this patch
replaces the PaintInfo rect adjustment with an infinite rect.

[1] http://trac.webkit.org/changeset/52900
[2] http://pr.gg/svgperfresults.png

BUG=442008
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187567

[pdr., 2014-12-13 05:58:19]

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

[pdr., 2014-12-15 05:02:21]

https://codereview.chromium.org/802833003/diff/20001/Source/core/paint/SVGFor...
File Source/core/paint/SVGForeignObjectPainter.cpp (right):

https://codereview.chromium.org/802833003/diff/20001/Source/core/paint/SVGFor...
Source/core/paint/SVGForeignObjectPainter.cpp:29: const AffineTransform&
transform = SVGRenderSupport::transformToRootBorderBox(m_renderSVGForeignObject,
m_renderSVGForeignObject.localTransform());
How can we cache this from paint invalidation? (similarly for SVGTextPainter)

https://codereview.chromium.org/802833003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGRenderSupport.cpp (right):

https://codereview.chromium.org/802833003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderSupport.cpp:152: while (next &&
!next->isSVGRoot()) {
linear algebra 101: instead of inverting each matrix, we can post-multiply them
in reverse and invert the result.

[fs, 2014-12-15 10:28:16]

So... removal of paintInvalidationRectInLocalCoordinates draws nigh ;-)

https://codereview.chromium.org/802833003/diff/20001/Source/core/paint/SVGFor...
File Source/core/paint/SVGForeignObjectPainter.cpp (right):

https://codereview.chromium.org/802833003/diff/20001/Source/core/paint/SVGFor...
Source/core/paint/SVGForeignObjectPainter.cpp:29: const AffineTransform&
transform = SVGRenderSupport::transformToRootBorderBox(m_renderSVGForeignObject,
m_renderSVGForeignObject.localTransform());
On 2014/12/15 05:02:21, pdr wrote:
> How can we cache this from paint invalidation? (similarly for SVGTextPainter)

Add:

m_cachedRootTransform = paintInvalidationState->svgTransform() *
localTransform();

(w/ checks and fallbacks)

in RenderSVGBlock::invalidateTreeIfNeeded (invalidatePaintIfNeeded might be
better in theory, but the result should be the same none the less...)

May also just say: 48 additional bytes in RenderSVGBlock - ouch!

https://codereview.chromium.org/802833003/diff/20001/Source/core/paint/SVGFor...
Source/core/paint/SVGForeignObjectPainter.cpp:30: childPaintInfo.rect =
enclosingIntRect(transform.inverse().mapRect(FloatRect(childPaintInfo.rect)));
One inverse() too many (since transformToRootBorderBox currently returns the
inverse.) I suppose this is the reason for the try-bot failures?

https://codereview.chromium.org/802833003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGRenderSupport.cpp (right):

https://codereview.chromium.org/802833003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderSupport.cpp:152: while (next &&
!next->isSVGRoot()) {
If next is/ends up as null, you'll be toast anyway.

https://codereview.chromium.org/802833003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderSupport.cpp:153: transform =
next->localToParentTransform() * transform;
(Last I checked a sequence like this generated horrible code... lots of copies.
Wanted (DoA - preferably A): multiply-from-the-left method on AffineTransform.)

https://codereview.chromium.org/802833003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderSupport.cpp:158: if
(!transform.isInvertible())
Maybe just return |transform| instead of its inverse? The name doesn't really
help imply one or the other; see also issue at callsite. Also, IIRC inverse()
gives you this behavior by default (det == 0 => identify else ...)

[chrishtr, 2014-12-15 16:36:25]

https://codereview.chromium.org/802833003/diff/20001/Source/core/paint/SVGFor...
File Source/core/paint/SVGForeignObjectPainter.cpp (right):

https://codereview.chromium.org/802833003/diff/20001/Source/core/paint/SVGFor...
Source/core/paint/SVGForeignObjectPainter.cpp:31: 
Maybe just use an infinite rect here to turn off culling for HTML subtrees of
SVG?

[pdr., 2014-12-16 06:29:56]

Thanks for the reviews! Fredrick's paint invalidation idea works but there are a
few failures I didn't have time to address today. Uploading for the curious; I
should have this up for a real review tomorrow.

https://codereview.chromium.org/802833003/diff/20001/Source/core/paint/SVGFor...
File Source/core/paint/SVGForeignObjectPainter.cpp (right):

https://codereview.chromium.org/802833003/diff/20001/Source/core/paint/SVGFor...
Source/core/paint/SVGForeignObjectPainter.cpp:29: const AffineTransform&
transform = SVGRenderSupport::transformToRootBorderBox(m_renderSVGForeignObject,
m_renderSVGForeignObject.localTransform());
On 2014/12/15 at 10:28:16, fs wrote:
> On 2014/12/15 05:02:21, pdr wrote:
> > How can we cache this from paint invalidation? (similarly for
SVGTextPainter)
> 
> Add:
> 
> m_cachedRootTransform = paintInvalidationState->svgTransform() *
localTransform();
> 
> (w/ checks and fallbacks)
> 
> in RenderSVGBlock::invalidateTreeIfNeeded (invalidatePaintIfNeeded might be
better in theory, but the result should be the same none the less...)
> 
> May also just say: 48 additional bytes in RenderSVGBlock - ouch!

Thank you! This worked a charm.

I'm not /too/ worried about the extra memory since it's only on RenderSVGText
and RenderSVGForeignObject. The hardest case I know of is iWork which, IIRC,
does one text element per word. WDYT?

https://codereview.chromium.org/802833003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGRenderSupport.cpp (right):

https://codereview.chromium.org/802833003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderSupport.cpp:153: transform =
next->localToParentTransform() * transform;
On 2014/12/15 at 10:28:16, fs wrote:
> (Last I checked a sequence like this generated horrible code... lots of
copies. Wanted (DoA - preferably A): multiply-from-the-left method on
AffineTransform.)

It does indeed, we should add this to AffineTransform next time we need it.

[fs, 2014-12-16 09:43:55]

https://codereview.chromium.org/802833003/diff/20001/Source/core/paint/SVGFor...
File Source/core/paint/SVGForeignObjectPainter.cpp (right):

https://codereview.chromium.org/802833003/diff/20001/Source/core/paint/SVGFor...
Source/core/paint/SVGForeignObjectPainter.cpp:29: const AffineTransform&
transform = SVGRenderSupport::transformToRootBorderBox(m_renderSVGForeignObject,
m_renderSVGForeignObject.localTransform());
On 2014/12/16 06:29:56, pdr wrote:
> On 2014/12/15 at 10:28:16, fs wrote:
> > On 2014/12/15 05:02:21, pdr wrote:
> > > How can we cache this from paint invalidation? (similarly for
> SVGTextPainter)
> > 
> > Add:
> > 
> > m_cachedRootTransform = paintInvalidationState->svgTransform() *
> localTransform();
> > 
> > (w/ checks and fallbacks)
> > 
> > in RenderSVGBlock::invalidateTreeIfNeeded (invalidatePaintIfNeeded might be
> better in theory, but the result should be the same none the less...)
> > 
> > May also just say: 48 additional bytes in RenderSVGBlock - ouch!
> 
> Thank you! This worked a charm.
> 
> I'm not /too/ worried about the extra memory since it's only on RenderSVGText
> and RenderSVGForeignObject. The hardest case I know of is iWork which, IIRC,
> does one text element per word. WDYT?

I'm not too worried either - let's just hope that we can offset this increase in
size somehow. (Just get used to me complaining every time you add a few bytes to
some datastructure ;-))

[pdr., 2014-12-17 08:09:21]

Issues fixed! I think we're getting close now. PTAL

[fs, 2014-12-17 10:16:11]

Almost there...

https://codereview.chromium.org/802833003/diff/100001/Source/core/paint/SVGFo...
File Source/core/paint/SVGForeignObjectPainter.cpp (right):

https://codereview.chromium.org/802833003/diff/100001/Source/core/paint/SVGFo...
Source/core/paint/SVGForeignObjectPainter.cpp:28: AffineTransform
transformToRoot = m_renderSVGForeignObject.cachedPaintInvalidationTransform() *
m_renderSVGForeignObject.localTransform();
Nit: Move this into the if-scope?

https://codereview.chromium.org/802833003/diff/100001/Source/core/paint/SVGTe...
File Source/core/paint/SVGTextPainter.cpp (right):

https://codereview.chromium.org/802833003/diff/100001/Source/core/paint/SVGTe...
Source/core/paint/SVGTextPainter.cpp:27: AffineTransform transformToRoot =
m_renderSVGText.cachedPaintInvalidationTransform() *
m_renderSVGText.localToParentTransform();
Nit: Same as for fO. (Would it make sense to move this to a method on
RenderSVGBlock?)

https://codereview.chromium.org/802833003/diff/100001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGBlock.cpp (right):

https://codereview.chromium.org/802833003/diff/100001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGBlock.cpp:130:
m_cachedPaintInvalidationTransform = paintInvalidationState.svgTransform();
I think you need an else-branch here which calls the method from one of the
older PS's (to compute this the slow and arduous way). This is needed because of
the ForceHorriblySlowRectMapping below, and I think a case like:

<svg>
  <foreignObject>
    <svg>
      <text>

could trigger it.

https://codereview.chromium.org/802833003/diff/100001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGResourceFilter.h (right):

https://codereview.chromium.org/802833003/diff/100001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGResourceFilter.h:49: FloatRect drawingRegion;
It looks like this field is now unused, so drop it?

[pdr., 2014-12-18 22:22:41]

PTAL

https://codereview.chromium.org/802833003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGRenderSupport.cpp (right):

https://codereview.chromium.org/802833003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderSupport.cpp:152: while (next &&
!next->isSVGRoot()) {
On 2014/12/15 at 10:28:16, fs wrote:
> If next is/ends up as null, you'll be toast anyway.

Replaced with an assert.

https://codereview.chromium.org/802833003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderSupport.cpp:153: transform =
next->localToParentTransform() * transform;
On 2014/12/16 at 06:29:56, pdr wrote:
> On 2014/12/15 at 10:28:16, fs wrote:
> > (Last I checked a sequence like this generated horrible code... lots of
copies. Wanted (DoA - preferably A): multiply-from-the-left method on
AffineTransform.)
> 
> It does indeed, we should add this to AffineTransform next time we need it.

I looked into this and premultiply doesn't seem as expensive as I remembered. We
do create a copy for each walk up the parent chain but AffineTransform is cheap
to copy. I did file http://crbug.com/443775 for some other AffineTransform
optimization ideas though.

https://codereview.chromium.org/802833003/diff/100001/Source/core/paint/SVGFo...
File Source/core/paint/SVGForeignObjectPainter.cpp (right):

https://codereview.chromium.org/802833003/diff/100001/Source/core/paint/SVGFo...
Source/core/paint/SVGForeignObjectPainter.cpp:28: AffineTransform
transformToRoot = m_renderSVGForeignObject.cachedPaintInvalidationTransform() *
m_renderSVGForeignObject.localTransform();
On 2014/12/17 at 10:16:10, fs wrote:
> Nit: Move this into the if-scope?

Done

https://codereview.chromium.org/802833003/diff/100001/Source/core/paint/SVGTe...
File Source/core/paint/SVGTextPainter.cpp (right):

https://codereview.chromium.org/802833003/diff/100001/Source/core/paint/SVGTe...
Source/core/paint/SVGTextPainter.cpp:27: AffineTransform transformToRoot =
m_renderSVGText.cachedPaintInvalidationTransform() *
m_renderSVGText.localToParentTransform();
On 2014/12/17 at 10:16:10, fs wrote:
> Nit: Same as for fO. (Would it make sense to move this to a method on
RenderSVGBlock?)

Good idea. Done.

https://codereview.chromium.org/802833003/diff/100001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGBlock.cpp (right):

https://codereview.chromium.org/802833003/diff/100001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGBlock.cpp:130:
m_cachedPaintInvalidationTransform = paintInvalidationState.svgTransform();
On 2014/12/17 at 10:16:10, fs wrote:
> I think you need an else-branch here which calls the method from one of the
older PS's (to compute this the slow and arduous way). This is needed because of
the ForceHorriblySlowRectMapping below, and I think a case like:
> 
> <svg>
>   <foreignObject>
>     <svg>
>       <text>
> 
> could trigger it.

Took a while to create a testcase but you were absolutely right. Fixed, and
added a test to prove it.

[fs, 2014-12-19 11:46:01]

LGTM w/ the issue in RenderSVGBlock addressed. (I'll be OOO for a while after
today, so any additional feedback could suffer from even higher delays.)

https://codereview.chromium.org/802833003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGRenderSupport.cpp (right):

https://codereview.chromium.org/802833003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderSupport.cpp:153: transform =
next->localToParentTransform() * transform;
On 2014/12/18 22:22:40, pdr wrote:
> On 2014/12/16 at 06:29:56, pdr wrote:
> > On 2014/12/15 at 10:28:16, fs wrote:
> > > (Last I checked a sequence like this generated horrible code... lots of
> copies. Wanted (DoA - preferably A): multiply-from-the-left method on
> AffineTransform.)
> > 
> > It does indeed, we should add this to AffineTransform next time we need it.
> 
> I looked into this and premultiply doesn't seem as expensive as I remembered.
We
> do create a copy for each walk up the parent chain but AffineTransform is
cheap
> to copy. I did file http://crbug.com/443775 for some other AffineTransform
> optimization ideas though.

Moves are cheap execution-wise, but cost in terms of decoder resources (and
code-size).

https://codereview.chromium.org/802833003/diff/140001/LayoutTests/svg/foreign...
File LayoutTests/svg/foreignObject/transformed-text-invalidation.html (right):

https://codereview.chromium.org/802833003/diff/140001/LayoutTests/svg/foreign...
LayoutTests/svg/foreignObject/transformed-text-invalidation.html:13:
<text>PASS</text>
Maybe add a transform to <text> as well (see issue in RenderSVGBlock.)

https://codereview.chromium.org/802833003/diff/140001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGBlock.cpp (right):

https://codereview.chromium.org/802833003/diff/140001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGBlock.cpp:132: AffineTransform transform =
localTransform();
This shouldn't be included, because it's included in updatePaintInfoRect (and
not included in the cached-part of the if-else).

[pdr., 2014-12-19 18:39:14]

The CQ bit was checked by pdr@chromium.org

[pdr., 2014-12-19 18:39:15]

Thanks for the excellent reviews--saved us from several tricky regressions. Off
to the CQ!

https://codereview.chromium.org/802833003/diff/140001/LayoutTests/svg/foreign...
File LayoutTests/svg/foreignObject/transformed-text-invalidation.html (right):

https://codereview.chromium.org/802833003/diff/140001/LayoutTests/svg/foreign...
LayoutTests/svg/foreignObject/transformed-text-invalidation.html:13:
<text>PASS</text>
On 2014/12/19 at 11:46:00, fs wrote:
> Maybe add a transform to <text> as well (see issue in RenderSVGBlock.)

Done.

https://codereview.chromium.org/802833003/diff/140001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGBlock.cpp (right):

https://codereview.chromium.org/802833003/diff/140001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGBlock.cpp:132: AffineTransform transform =
localTransform();
On 2014/12/19 at 11:46:00, fs wrote:
> This shouldn't be included, because it's included in updatePaintInfoRect (and
not included in the cached-part of the if-else).

Done

[commit-bot: I haz the power, 2014-12-19 18:40:25]

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

[commit-bot: I haz the power, 2014-12-19 21:18:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-19 21:18:57]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/42838)

[pdr., 2014-12-19 21:37:30]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-12-19 21:38:09]

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

[commit-bot: I haz the power, 2014-12-19 22:59:23]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187567