Extract RenderSVGModelObject::invalidatePaintIfNeeded()

Extract RenderSVGModelObject::invalidatePaintIfNeeded()

This reduces duplicated code for early returns in original
RenderSVGModelObject::invalidateTreeIfNeeded(), and helps to ensure
all code needed to be executed for original and future early returns.

BTW this fixes the issue caused by early return on
!SVGRenderSupport::parentTransformDidChange(this) that descendants
may miss update of previousPaintInvalidateRect() causing stale
previousPaintInvalidateRect in later invalidation.

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

[Xianzhu, 2014-07-21 16:48:35]

PTAL. This is first of 4 patches split from
https://codereview.chromium.org/398343003/ (Unified invalidation path).

[dsinclair, 2014-07-21 17:05:44]

On 2014/07/21 16:48:35, Xianzhu wrote:
> PTAL. This is first of 4 patches split from
> https://codereview.chromium.org/398343003/ (Unified invalidation path).


The code looks like it's doing the same work as before, how does this fix the
issue with not updating the previousPaintInvalidationRect?

It looks like: svg/repaint/container-repaint.svg is invalidating less now, is it
correct that it doesn't invalidate the top piece?

[Xianzhu, 2014-07-21 17:14:26]

On 2014/07/21 17:05:44, dsinclair wrote:
> On 2014/07/21 16:48:35, Xianzhu wrote:
> > PTAL. This is first of 4 patches split from
> > https://codereview.chromium.org/398343003/ (Unified invalidation path).
> 
> 
> The code looks like it's doing the same work as before, how does this fix the
> issue with not updating the previousPaintInvalidationRect?
> 

There is one difference: previously the early return on 'if
(!SVGRenderSupport::parentTransformDidChange(this))' pruned subtree walking so
descendants might have stale previousPaintInvalidationRect.

> It looks like: svg/repaint/container-repaint.svg is invalidating less now, is
it
> correct that it doesn't invalidate the top piece?

Yes, it's correct. The test moves an SVG object (containing a 100x50 rect) from
y=50 to y=100. It's unnecessary to repaint the top piece.

[dsinclair, 2014-07-21 17:24:54]

On 2014/07/21 17:14:26, Xianzhu wrote:
> On 2014/07/21 17:05:44, dsinclair wrote:
> > The code looks like it's doing the same work as before, how does this fix
the
> > issue with not updating the previousPaintInvalidationRect?
> 
> There is one difference: previously the early return on 'if
> (!SVGRenderSupport::parentTransformDidChange(this))' pruned subtree walking so
> descendants might have stale previousPaintInvalidationRect.
> 

Oh, so we're going to walk all of those children in this case. That may not be
acceptable. We had to put that prune in there because walking the children was
too slow, we had perf regressions on SVGCubics (I think it was) due to that tree
walk.

Can you run the SVG perf tests with and without this change to see if there is
any slowdown?

[Xianzhu, 2014-07-21 21:43:46]

On 2014/07/21 17:24:54, dsinclair wrote:
> On 2014/07/21 17:14:26, Xianzhu wrote:
> > On 2014/07/21 17:05:44, dsinclair wrote:
> > > The code looks like it's doing the same work as before, how does this fix
> the
> > > issue with not updating the previousPaintInvalidationRect?
> > 
> > There is one difference: previously the early return on 'if
> > (!SVGRenderSupport::parentTransformDidChange(this))' pruned subtree walking
so
> > descendants might have stale previousPaintInvalidationRect.
> > 
> 
> Oh, so we're going to walk all of those children in this case. That may not be
> acceptable. We had to put that prune in there because walking the children was
> too slow, we had perf regressions on SVGCubics (I think it was) due to that
tree
> walk.
> 
> Can you run the SVG perf tests with and without this change to see if there is
> any slowdown?

Tested but didn't see obvious performance change.

Percent changes of results are as below (See notes at the end about the meaning
of the numbers):
AzLizardBenjiPark: 5.94%
Bamboo: -0.99%
Cactus: -2.90%
Cowboy: 0.73%
CrawFishGanson: 4.51%
Debian: 12.40%
DropsOnABlade: -7.88%
FlowerFromMyGarden: -1.51%
FoodLeifLodahl: 2.03%
France: -5.72%
FrancoBolloGnomeEzechi: 1.01%
GearFlowers: 8.99%
HarveyRayner: -1.22%
HereGear: 2.03%
MtSaintHelens: -2.53%
Samurai: -8.72%
SierpinskiCarpet: -1.98%
SvgCubics: 7.10%
SvgHitTesting: -0.95%
SvgNestedUse: -2.63%
UnderTheSee: -0.62%
WorldIso: -1.16%
Worldcup: -2.54%
Average: 0.15%

Another run (containing 3 runs with and without patch) showed slight different
results:
AzLizardBenjiPark: 0.44%
Bamboo: -5.63%
Cactus: -5.51%
Cowboy: -0.12%
CrawFishGanson: -8.87%
Debian: 0.43%
DropsOnABlade: 3.57%
FlowerFromMyGarden: 6.57%
FoodLeifLodahl: -0.87%
France: -0.48%
FrancoBolloGnomeEzechi: -6.97%
GearFlowers: -8.26%
HarveyRayner: -1.20%
HereGear: -6.84%
MtSaintHelens: -5.32%
Samurai: 2.91%
SierpinskiCarpet: -1.07%
SvgCubics: -3.43%
SvgHitTesting: -0.52%
SvgNestedUse: -0.26%
UnderTheSee: -2.72%
WorldIso: -7.98%
Worldcup: -0.39%
Average: -2.28%

Notes:
- For each set of the above result, the benchmark was ran with patch and without
patch 3 times;
- Positive number means performance regression
- For 'ms' results, each of the percent numbers is (average_with_patch -
average_without_patch) / average_without_patch
- For 'runs/s' results, the percent number is negative of the above.

[Xianzhu, 2014-07-22 16:06:54]

I think the change about the early return of !parentTransformDidChange is about
correctness. We should fix the issue even if there would be some performance
regression. My test result showed no obvious regression. Just walking the tree
without real invalidation seems cheap.

Now I'm trying to understand the flag and create a test case to show the issue
of the original code.

[fs, 2014-07-22 17:49:41]

On 2014/07/21 17:24:54, dsinclair wrote:
> On 2014/07/21 17:14:26, Xianzhu wrote:
> > On 2014/07/21 17:05:44, dsinclair wrote:
> > > The code looks like it's doing the same work as before, how does this fix
> the
> > > issue with not updating the previousPaintInvalidationRect?
> > 
> > There is one difference: previously the early return on 'if
> > (!SVGRenderSupport::parentTransformDidChange(this))' pruned subtree walking
so
> > descendants might have stale previousPaintInvalidationRect.
> > 
> 
> Oh, so we're going to walk all of those children in this case. That may not be
> acceptable. We had to put that prune in there because walking the children was
> too slow, we had perf regressions on SVGCubics (I think it was) due to that
tree
> walk.

Yes, SvgCubics. It did regress by ~100% or something (IIRC). The tree-walk at
that point did more work than this version appears to do though. I.e. then the
tree-walk would both update state and issue the actual repaint. IIUC this
version only does the former. That pretty much what my ugly-hack-PoC did too, so
this LGTM if dsinclair is convinced =).
 
> Can you run the SVG perf tests with and without this change to see if there is
> any slowdown?

I could also be interesting to remove the pruning-condition
(!parentTransformDidChange) and see how that differs in runtime. Doing that on
SvgCubics ought to give enough of an illustration I think.

[dsinclair, 2014-07-22 17:52:10]

lgtm

[Xianzhu, 2014-07-22 18:19:29]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-07-22 18:20:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/401883004/20001

[Xianzhu, 2014-07-22 19:06:54]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-07-22 19:08:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/401883004/40001

[commit-bot: I haz the power, 2014-07-22 21:01:12]

Change committed as 178694