Initial PaintInvalidationState support for SVG

Initial PaintInvalidationState support for SVG

Extend PaintInvalidationState with a transform, to allow computing the
position/area of an SVG object first within the (outermost) SVG root, and
then relative to the document.
Handling of PaintInvalidationState is added only to RenderSVGModelObject-
derived render objects for now.

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

[fs, 2014-09-24 07:31:52]

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

[fs, 2014-09-24 07:31:53]

I've opted to stay as close as possible to what's currently being computed, and
all LayoutTests seem to pass after some minor tweaks. This means that there
might be a little more "impedance matching" code that one would hope for, as
well as some "oddities" (see FIXME in
SVGRenderSupport::computeFloatRectForPaintInvalidation).

Not sure how you feel about the AffineTransform, which amounts to "dead weight"
for HTML - it should be possible to marginalize a bit if deemed too intrusive.

[pdr., 2014-09-25 05:38:59]

This is cool! Do you know the performance impact of this change? Even though
it's only for RenderSVGModelObject, that accounts for nearly all SVG renderers.

https://codereview.chromium.org/598623002/diff/20001/Source/core/rendering/Pa...
File Source/core/rendering/PaintInvalidationState.h (right):

https://codereview.chromium.org/598623002/diff/20001/Source/core/rendering/Pa...
Source/core/rendering/PaintInvalidationState.h:63: AffineTransform
m_svgTransform;
Could we use an OwnPtr here to lazily allocate this?

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

https://codereview.chromium.org/598623002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:132: // If we didn't need
paint invalidation then our children don't need as well.
Nit: In my experience, children are extremely needy.

[fs, 2014-09-25 12:17:53]

On 2014/09/25 05:38:59, pdr wrote:
> ... Do you know the performance impact of this change?

Not with any high reliability. The existing tests (that I know of) that could
show improvements (SvgCubics) appeared to show a slight decrease in frame-time
(1-2% or so), but the noise-level was also quite high (probably in part due to
my machine not being idle enough). Given that paint-invalidation is tied to
layer updates I'd not expect it to be a large enough bottleneck to show a big
improvement. I pondered doing some synthetic test, but had a hard time
motivating that due to the above.

https://codereview.chromium.org/598623002/diff/20001/Source/core/rendering/Pa...
File Source/core/rendering/PaintInvalidationState.h (right):

https://codereview.chromium.org/598623002/diff/20001/Source/core/rendering/Pa...
Source/core/rendering/PaintInvalidationState.h:63: AffineTransform
m_svgTransform;
On 2014/09/25 05:38:59, pdr wrote:
> Could we use an OwnPtr here to lazily allocate this?

Yes, I suppose that could be a reasonable compromise (it makes the
initialization a bit more unwieldy though.) Done.

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

https://codereview.chromium.org/598623002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:132: // If we didn't need
paint invalidation then our children don't need as well.
On 2014/09/25 05:38:59, pdr wrote:
> Nit: In my experience, children are extremely needy.

My diplomatic response to this would be: "No comment"

On a more serious note, I'm not sure I (fully) agree with this statement, but
AFAIU it's an invariant of the current system. This comment just hitched a ride
from the previous (base-class) impl. in RenderObject.

[fs, 2014-09-25 12:30:26]

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

https://codereview.chromium.org/598623002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:132: // If we didn't need
paint invalidation then our children don't need as well.
On 2014/09/25 12:17:53, fs wrote:
> On 2014/09/25 05:38:59, pdr wrote:
> > Nit: In my experience, children are extremely needy.
> 
> My diplomatic response to this would be: "No comment"
> 
> On a more serious note, I'm not sure I (fully) agree with this statement, but
> AFAIU it's an invariant of the current system. This comment just hitched a
ride
> from the previous (base-class) impl. in RenderObject.

s/this statement/the statement the comment makes/ (to avoid confusion)

[dsinclair, 2014-09-25 13:14:10]

dsinclair@chromium.org changed reviewers:
+ dsinclair@chromium.org

[dsinclair, 2014-09-25 13:14:11]

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

https://codereview.chromium.org/598623002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:132: // If we didn't need
paint invalidation then our children don't need as well.
On 2014/09/25 12:30:26, fs wrote:
> On 2014/09/25 12:17:53, fs wrote:
> > On 2014/09/25 05:38:59, pdr wrote:
> > > Nit: In my experience, children are extremely needy.
> > 
> > My diplomatic response to this would be: "No comment"
> > 
> > On a more serious note, I'm not sure I (fully) agree with this statement,
but
> > AFAIU it's an invariant of the current system. This comment just hitched a
> ride
> > from the previous (base-class) impl. in RenderObject.
> 
> s/this statement/the statement the comment makes/ (to avoid confusion)


As part of repaint-after-layout, we made this an invariant. It's currently
maintained by assertSubtreeClearedPaintInvalidationState() and
assertRendererClearedPaintInvalidationState().

When invalidation is finished we walk over all renderers and ASSERT that the
state is clean.

[pdr., 2014-09-26 03:32:45]

The code looks great but I would have expected this to be a clear perf win. I'm
skeptical... are we missing something?

@leviw, could you comment on this one?

[fs, 2014-09-26 15:07:08]

On 2014/09/26 03:32:45, pdr wrote:
> The code looks great but I would have expected this to be a clear perf win.
I'm
> skeptical... are we missing something?

So I hacked a contrived example [1] to see what that might show. Unless my
measurements were off there it's more in the 10-20% range.

With https://codereview.chromium.org/609463002/ landed I also moved the code to
clippedOverflowRect...

[1] http://jsfiddle.net/ahnf13Lj/2/embedded/result/

[pdr., 2014-09-26 23:22:39]

On 2014/09/26 15:07:08, fs wrote:
> On 2014/09/26 03:32:45, pdr wrote:
> > The code looks great but I would have expected this to be a clear perf win.
> I'm
> > skeptical... are we missing something?
> 
> So I hacked a contrived example [1] to see what that might show. Unless my
> measurements were off there it's more in the 10-20% range.
> 
> With https://codereview.chromium.org/609463002/ landed I also moved the code
to
> clippedOverflowRect...
> 
> [1] http://jsfiddle.net/ahnf13Lj/2/embedded/result/

LGTM

[pdr., 2014-09-26 23:22:54]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-09-26 23:23:45]

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

[commit-bot: I haz the power, 2014-09-27 00:03:31]

Committed patchset #4 (id:60001) as 182799