Reset m_didTransformToRootUpdate flag during invalidationTreeIfNeeded

Reset m_didTransformToRootUpdate flag during invalidationTreeIfNeeded

Previously m_didTransformToRootUpdate flag of
RenderSVGViewportContainer and RenderSVGTransformableContainer
only gets updated during layout.

As we use the flag during invalidationTreeIfNeeded and we may call
invalidationTreeIfNeeded when there is only repaint-only invalidation
(https://codereview.chromium.org/398343003/), we should reset the
flag after invalidation.

Will be tested in https://codereview.chromium.org/398343003/.

BUG=394619

[dsinclair, 2014-07-22 17:49:39]

This seems like it should be done in a clearPaintInvalidationState() override,
instead of in invalidateTreeIfNeeded().

[Xianzhu, 2014-07-22 17:53:03]

On 2014/07/22 17:49:39, dsinclair wrote:
> This seems like it should be done in a clearPaintInvalidationState() override,
> instead of in invalidateTreeIfNeeded().

Agreed. Hesitated about whether to make it virtual, but this is more correct and
useful. We can also use it to clear the scrollbar damage state in RenderBox.

[Xianzhu, 2014-07-22 17:58:44]

On 2014/07/22 17:53:03, Xianzhu wrote:
> On 2014/07/22 17:49:39, dsinclair wrote:
> > This seems like it should be done in a clearPaintInvalidationState()
override,
> > instead of in invalidateTreeIfNeeded().
> 
> Agreed. Hesitated about whether to make it virtual, but this is more correct
and
> useful. We can also use it to clear the scrollbar damage state in RenderBox.

Done.

[dsinclair, 2014-07-22 18:03:52]

Should we hold off on this until Julien gets his ASSERT landed? Will we need to
override the assert method in these classes so we can validate that this field
is also cleared?

https://codereview.chromium.org/408273003/diff/10005/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGTransformableContainer.cpp (right):

https://codereview.chromium.org/408273003/diff/10005/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGTransformableContainer.cpp:112:
m_didTransformToRootUpdate = false;
Should these also be calling the RenderObject::clearPaintInvalidationState() as
well to maek sure everything else is cleared?

[Xianzhu, 2014-07-22 18:22:30]

On 2014/07/22 18:03:52, dsinclair wrote:
> Should we hold off on this until Julien gets his ASSERT landed? Will we need
to
> override the assert method in these classes so we can validate that this field
> is also cleared?

OK.

> 
>
https://codereview.chromium.org/408273003/diff/10005/Source/core/rendering/sv...
> File Source/core/rendering/svg/RenderSVGTransformableContainer.cpp (right):
> 
>
https://codereview.chromium.org/408273003/diff/10005/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGTransformableContainer.cpp:112:
> m_didTransformToRootUpdate = false;
> Should these also be calling the RenderObject::clearPaintInvalidationState()
as
> well to maek sure everything else is cleared?

So hurried :)
Done.

[Xianzhu, 2014-07-22 23:12:39]

On 2014/07/22 18:22:30, Xianzhu wrote:
> On 2014/07/22 18:03:52, dsinclair wrote:
> > Should we hold off on this until Julien gets his ASSERT landed? Will we need
> to
> > override the assert method in these classes so we can validate that this
field
> > is also cleared?
> 
> OK.
> 

Done.

[Julien - ping for review, 2014-07-23 18:47:39]

https://codereview.chromium.org/408273003/diff/50001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGTransformableContainer.h (right):

https://codereview.chromium.org/408273003/diff/50001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGTransformableContainer.h:38: virtual bool
didTransformToRootUpdate() const OVERRIDE { return m_didTransformToRootUpdate; }
m_didTransformToRoot is reset during layout (RenderSVGContainer::layout()) so it
seems pretty weird for us to have to do something here.

More conceptually, we may trigger several layouts before producing a frame that
will generate invalidation so it's unclear to me:
A) what this tries to achieve.
B) why this is correct if subsequent layouts could have cleared the flag before
we generate an invalidation.

https://codereview.chromium.org/408273003/diff/50001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGViewportContainer.h (right):

https://codereview.chromium.org/408273003/diff/50001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGViewportContainer.h:45: virtual void
clearPaintInvalidationState() OVERRIDE;
This should be private as we assume people will call the RenderObject one.

[Xianzhu, 2014-07-23 18:52:30]

https://codereview.chromium.org/408273003/diff/50001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGTransformableContainer.h (right):

https://codereview.chromium.org/408273003/diff/50001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGTransformableContainer.h:38: virtual bool
didTransformToRootUpdate() const OVERRIDE { return m_didTransformToRootUpdate; }
On 2014/07/23 18:47:39, Julien Chaffraix - PST wrote:
> m_didTransformToRoot is reset during layout (RenderSVGContainer::layout()) so
it
> seems pretty weird for us to have to do something here.
> 
> More conceptually, we may trigger several layouts before producing a frame
that
> will generate invalidation so it's unclear to me:
> A) what this tries to achieve.
> B) why this is correct if subsequent layouts could have cleared the flag
before
> we generate an invalidation.

I think you are right. Perhaps we should not clear the flag during layout.

fs@opera.com, wdyt?

[Xianzhu, 2014-07-23 19:05:08]

https://codereview.chromium.org/408273003/diff/50001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGTransformableContainer.h (right):

https://codereview.chromium.org/408273003/diff/50001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGTransformableContainer.h:38: virtual bool
didTransformToRootUpdate() const OVERRIDE { return m_didTransformToRootUpdate; }
On 2014/07/23 18:52:29, Xianzhu wrote:
> On 2014/07/23 18:47:39, Julien Chaffraix - PST wrote:
> > m_didTransformToRoot is reset during layout (RenderSVGContainer::layout())
so
> it
> > seems pretty weird for us to have to do something here.
> > 
> > More conceptually, we may trigger several layouts before producing a frame
> that
> > will generate invalidation so it's unclear to me:
> > A) what this tries to achieve.
> > B) why this is correct if subsequent layouts could have cleared the flag
> before
> > we generate an invalidation.
> 
> I think you are right. Perhaps we should not clear the flag during layout.
> 
> mailto:fs@opera.com, wdyt?

The latest Patch Set avoids resetting the flag during layout.

https://codereview.chromium.org/408273003/diff/50001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGViewportContainer.h (right):

https://codereview.chromium.org/408273003/diff/50001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGViewportContainer.h:45: virtual void
clearPaintInvalidationState() OVERRIDE;
On 2014/07/23 18:47:39, Julien Chaffraix - PST wrote:
> This should be private as we assume people will call the RenderObject one.

Done.

[dsinclair, 2014-07-23 19:20:59]

https://codereview.chromium.org/408273003/diff/70001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGTransformableContainer.cpp (right):

https://codereview.chromium.org/408273003/diff/70001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGTransformableContainer.cpp:100:
m_didTransformToRootUpdate = true;
I don't think we want to do this. We use this flag to determine if we can skip
the tree walk. So, I think what we care about is the result of the last layout.

If the last layout sets this to true, then we can skip the tree walk as the
container will handle the invalidations.

If the last layout sets this to false, then we have to do the tree walk as we
don't know if the container will do the invalidations for us.

This change will, potentially, set this to true on the first layout but then on
the last layout, if it should be false we'll leave the flag as true. So the
container won't issue the invalidation for us, and we'll terminate the tree walk
early. Leaving the subtree as potentially missed invalidations.

[Xianzhu, 2014-07-23 19:39:05]

https://codereview.chromium.org/408273003/diff/70001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGTransformableContainer.cpp (right):

https://codereview.chromium.org/408273003/diff/70001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGTransformableContainer.cpp:100:
m_didTransformToRootUpdate = true;
On 2014/07/23 19:20:59, dsinclair wrote:
> I don't think we want to do this. We use this flag to determine if we can skip
> the tree walk. So, I think what we care about is the result of the last
layout.
>
> If the last layout sets this to true, then we can skip the tree walk as the
> container will handle the invalidations.
>

> If the last layout sets this to false, then we have to do the tree walk as we
> don't know if the container will do the invalidations for us.
> 
> This change will, potentially, set this to true on the first layout but then
on
> the last layout, if it should be false we'll leave the flag as true. So the
> container won't issue the invalidation for us, and we'll terminate the tree
walk
> early. Leaving the subtree as potentially missed invalidations.

I think the full invalidation on transform change should happen in the next
invalidateTreeIfNeeded(). If this is true, it is correct to skip subtree walking
if the flag is set in any layout since the last invalidateTreeIfNeeded().

I didn't find any direct paint invalidation code in svg. However I'm not
familiar with SVG code.

[Xianzhu, 2014-07-23 19:42:33]

Found an issue of the latest change. The flag might be cleared too early before
walking subtree, so there are many extra repaints in the layout test results.

Will try to move clearPaintInvalidationState() after walking subtree in
RenderObject.

[Julien - ping for review, 2014-07-23 20:05:24]

We talked this with Dan and thought we should try to remove the pruning
optimization. The reason it was introduced is because SvgCubics does a lot of
layouts that generated a lot of invalidations.

Now that we have moved to doing invalidations after compositing updates, we
shouldn't do as many tree walks so that would potentially remove the need for
the optimization altogether.

This has the added benefit of also enabling
assertRendererClearedPaintInvalidationState for SVG tree (we currently bail
out).

https://codereview.chromium.org/408273003/diff/70001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGTransformableContainer.cpp (right):

https://codereview.chromium.org/408273003/diff/70001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGTransformableContainer.cpp:100:
m_didTransformToRootUpdate = true;
On 2014/07/23 19:39:04, Xianzhu wrote:
> On 2014/07/23 19:20:59, dsinclair wrote:
> > I don't think we want to do this. We use this flag to determine if we can
skip
> > the tree walk. So, I think what we care about is the result of the last
> layout.
> >
> > If the last layout sets this to true, then we can skip the tree walk as the
> > container will handle the invalidations.
> >
> 
> > If the last layout sets this to false, then we have to do the tree walk as
we
> > don't know if the container will do the invalidations for us.
> > 
> > This change will, potentially, set this to true on the first layout but then
> on
> > the last layout, if it should be false we'll leave the flag as true. So the
> > container won't issue the invalidation for us, and we'll terminate the tree
> walk
> > early. Leaving the subtree as potentially missed invalidations.
> 
> I think the full invalidation on transform change should happen in the next
> invalidateTreeIfNeeded(). If this is true, it is correct to skip subtree
walking
> if the flag is set in any layout since the last invalidateTreeIfNeeded().
> 
> I didn't find any direct paint invalidation code in svg. However I'm not
> familiar with SVG code.

I agree with Xianzhu on this one, it's conceptually wrong to ignore all layouts
but the last one when generating invalidations. This is because now the order of
layouts will have an impact on our invalidations.

[Xianzhu, 2014-07-24 00:49:19]

On 2014/07/23 20:05:24, Julien Chaffraix - PST wrote:
> We talked this with Dan and thought we should try to remove the pruning
> optimization. The reason it was introduced is because SvgCubics does a lot of
> layouts that generated a lot of invalidations.
> 
> Now that we have moved to doing invalidations after compositing updates, we
> shouldn't do as many tree walks so that would potentially remove the need for
> the optimization altogether.
> 

Tried removing the pruning optimization:
1. Some svg layout tests produced many new repaint rects (same as trybot result
of Patch Set 5), though they are all contained in existing big repaint rects.
  (for example, svg/carto.net/window.svg previously had 35 repaint rects, now
has 163)
2. Didn't see visible performance change of SvgCubics.

If 1 is OK, I could prepare a change to disable the pruning.

> This has the added benefit of also enabling
> assertRendererClearedPaintInvalidationState for SVG tree (we currently bail
> out).
> 

This change also enables assertRendererClearedPaintInvalidationState for SVG
tree because we include the didTransformToRootUpdate flag into
paintInvalidationState.

[fs, 2014-07-24 08:43:56]

If the prune can be removed due to reduced frequency of the invalidation tree
walk (I don't see how the cost would be lower though), then that SGTM.

https://codereview.chromium.org/408273003/diff/70001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGTransformableContainer.cpp (right):

https://codereview.chromium.org/408273003/diff/70001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGTransformableContainer.cpp:100:
m_didTransformToRootUpdate = true;
On 2014/07/23 20:05:24, Julien Chaffraix - PST wrote:
> On 2014/07/23 19:39:04, Xianzhu wrote:
> > On 2014/07/23 19:20:59, dsinclair wrote:
> > > I don't think we want to do this. We use this flag to determine if we can
> skip
> > > the tree walk. So, I think what we care about is the result of the last
> > layout.
> > >
> > > If the last layout sets this to true, then we can skip the tree walk as
the
> > > container will handle the invalidations.
> > >
> > 
> > > If the last layout sets this to false, then we have to do the tree walk as
> we
> > > don't know if the container will do the invalidations for us.
> > > 
> > > This change will, potentially, set this to true on the first layout but
then
> > on
> > > the last layout, if it should be false we'll leave the flag as true. So
the
> > > container won't issue the invalidation for us, and we'll terminate the
tree
> > walk
> > > early. Leaving the subtree as potentially missed invalidations.
> > 
> > I think the full invalidation on transform change should happen in the next
> > invalidateTreeIfNeeded(). If this is true, it is correct to skip subtree
> walking
> > if the flag is set in any layout since the last invalidateTreeIfNeeded().
> > 
> > I didn't find any direct paint invalidation code in svg. However I'm not
> > familiar with SVG code.
> 
> I agree with Xianzhu on this one, it's conceptually wrong to ignore all
layouts
> but the last one when generating invalidations. This is because now the order
of
> layouts will have an impact on our invalidations.

If there's N layouts to one repaint, and the container is moved (transform
changed) in one of them, then repainting only the container should be sufficient
because the repaint area of the container is the union of all it's children.
From that PoV accumulating into it seems fine (I think I'd just have |= though
for a smaller change =)). This flag is still used to do funky stuff to text
metrics during layout though (see SVGRenderSupport).