Bug 44896 – Add culling to RenderSVGContainer

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

Just like RenderPath, we should add culling functionality to RenderSVGContainer.
This involves caching the repaint/stroke/objectBoundingBox, which gives a huge performance benefit, as we don't need to traverse the render tree anymore when asking for these properties.

[Nikolas Zimmermann]

Created attachment 65961 [details]
Patch

[Dirk Schulze]

Comment on attachment 65961 [details]
Patch

r=me

[Nikolas Zimmermann]

Landed in r66418.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66418 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/66418
http://trac.webkit.org/changeset/66419
http://trac.webkit.org/changeset/66420
http://trac.webkit.org/changeset/66421

[Eric Seidel (no email)]

Comment on attachment 65961 [details]
Patch

layouted remains not an english word. :)

[Eric Seidel (no email)]

Comment on attachment 65961 [details]
Patch

Why are there test chagnes if this was just  a perf change?

Please say more in your ChangeLogs, especially if your conversations with the reviewer are not going to be on the bugs. :(

[Eric Seidel (no email)]

(In reply to comment #2)
> (From update of attachment 65961 [details])
> r=me

I really dislike when review comments don't take place in public.  Makes it hard for anyone else to follow along.

You two are both fine engineers, but part of the review process is education.  Both of each other, and the rest of webkit about the code in question.

SVG needs to move towards better integration with the rest of the project, not further off into its own secret world.

[Ryosuke Niwa]

http://trac.webkit.org/changeset/66418 seems to have caused fast/repaint/moving-shadow-on-container.html to fail on Mac.

--- /Volumes/Big/WebKit-BuildSlave/leopard-intel-release-tests/build/layout-test-results/fast/repaint/moving-shadow-on-container-expected.txt	2010-08-30 16:22:26.000000000 -0700
+++ /Volumes/Big/WebKit-BuildSlave/leopard-intel-release-tests/build/layout-test-results/fast/repaint/moving-shadow-on-container-actual.txt	2010-08-30 16:22:26.000000000 -0700
@@ -3,7 +3,7 @@
 layer at (0,0) size 785x616
   RenderBlock {HTML} at (0,0) size 785x616
     RenderBody {BODY} at (8,8) size 769x600
-      RenderSVGRoot {svg} at (8,8) size 468x173
+      RenderSVGRoot {svg} at (8,8) size 478x183
         RenderPath {path} at (8,8) size 78x68 [stroke={[type=SOLID] [color=#000000] [stroke width=10.00]}] [fill={[type=SOLID] [color=#999999]}] [data="M0.00,30.00 L-35.27,48.54 L-28.53,9.27 L-57.06,-18.54 L-17.63,-24.27 L-0.00,-60.00 L17.63,-24.27 L57.06,-18.54 L28.53,9.27 L35.27,48.54 Z"]
         RenderPath {path} at (200,46) size 128x125 [transform={m=((1.00,0.00)(0.00,1.00)) t=(250.00,100.00)}] [stroke={[type=SOLID] [color=#000000] [stroke width=10.00] [dash array={20.00}]}] [fill={[type=SOLID] [color=#999999]}] [data="M0.00,30.00 L-35.27,48.54 L-28.53,9.27 L-57.06,-18.54 L-17.63,-24.27 L-0.00,-60.00 L17.63,-24.27 L57.06,-18.54 L28.53,9.27 L35.27,48.54 Z"]
         RenderPath {path} at (349,46) size 117x122 [transform={m=((1.00,0.00)(0.00,1.00)) t=(400.00,100.00)}] [stroke={[type=SOLID] [color=#000000] [stroke width=10.00] [dash array={20.00}]}] [fill={[type=SOLID] [color=#999999]}] [data="M0.00,25.00 L-29.39,40.45 L-23.78,7.73 L-47.55,-15.45 L-14.69,-20.23 L-0.00,-50.00 L14.69,-20.23 L47.55,-15.45 L23.78,7.73 L29.39,40.45 Z"]

[Eric Seidel (no email)]

Reverted r66418 for reason:

Causing test failures on multiple bots

Committed r66449: <http://trac.webkit.org/changeset/66449>

[Csaba Osztrogonác]

Similar diff on Qt:

--- /home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/layout-test-results/fast/repaint/moving-shadow-on-container-expected.txt	2010-08-30 19:24:41.255174798 -0700
+++ /home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/layout-test-results/fast/repaint/moving-shadow-on-container-actual.txt	2010-08-30 19:24:41.255174798 -0700
@@ -3,7 +3,7 @@
 layer at (0,0) size 784x616
   RenderBlock {HTML} at (0,0) size 784x616
     RenderBody {BODY} at (8,8) size 768x600
-      RenderSVGRoot {svg} at (8,8) size 468x173
+      RenderSVGRoot {svg} at (8,8) size 478x183
         RenderPath {path} at (8,8) size 78x68 [stroke={[type=SOLID] wi

[Dirk Schulze]

(In reply to comment #7)
> (In reply to comment #2)
> > (From update of attachment 65961 [details] [details])
> > r=me
> 
> I really dislike when review comments don't take place in public.  Makes it hard for anyone else to follow along.
> 
> You two are both fine engineers, but part of the review process is education.  Both of each other, and the rest of webkit about the code in question.
> 
> SVG needs to move towards better integration with the rest of the project, not further off into its own secret world.

You're right. We should communicate more over bugs.webkit.org. But I think the ChangeLog is pretty clear about what Niko saw as a performance issue and how he tried to solve the problem.

We can cache boundingRects in SVGContainer with it! That is much better than calculating the union of all rects from the childs over and over again. It also helps us to do the culling much earlier and stop uneccessary rendering of childs (or at least avoids calling paint on them).

The shadow test fast/repaint/moving-shadow-on-container.html just needs an update. We clipped to much of the content before this patch.

Still r+ from me for this patch but with updates of the LayoutTests.

Eric: Do we need a new folder for HTML+SVG tests? Or where these should tests be added? In the SVG part of LayoutTests? The failing test looks a bit misplaced for me.

[Eric Seidel (no email)]

Why did layout tests change if this was just a perf fix?

[Nikolas Zimmermann]

(In reply to comment #7)
> (In reply to comment #2)
> > (From update of attachment 65961 [details] [details])
> > r=me
> 
> I really dislike when review comments don't take place in public.  Makes it hard for anyone else to follow along.
Well, we're sitting at SVG Open, and dicussed it face to face.

> 
> You two are both fine engineers, but part of the review process is education.  Both of each other, and the rest of webkit about the code in question.
> 
> SVG needs to move towards better integration with the rest of the project, not further off into its own secret world.

I am not sure what secret world you are talking about. Better integration with the rest of the project?
Reading that comment really makes me sad. I'm devoting all my time in SVG and no one else (except Dirk, and you) seems to care, since years. Your comment reads like I'm guilty for that.

I agree, that the ChangeLog is short, and I'll fix it up.

[Eric Seidel (no email)]

(In reply to comment #13)
> > I really dislike when review comments don't take place in public.  Makes it hard for anyone else to follow along.
> Well, we're sitting at SVG Open, and dicussed it face to face.

I was wondering how you two were communicating living in separate countries and all. :)

> I am not sure what secret world you are talking about. Better integration with the rest of the project?
> Reading that comment really makes me sad. I'm devoting all my time in SVG and no one else (except Dirk, and you) seems to care, since years. Your comment reads like I'm guilty for that.

I'm sorry to make you sad.  That's not my intention.

> I agree, that the ChangeLog is short, and I'll fix it up.

Thank you.

I should also note, that the rollout was *completely* unrelated to my earlier comments.  Adam attempted to roll out this patch with sheriffbot, due to the test breakages, and since it hung the commit queue, I came in to fix the cq (and the tree) by completing the rollout.

My question above of why this changes behavior if it's just a perf fix remains unanswered, but I assume your updated changelog will cover that.

Thanks again for the patch.  I hope you two are enjoying Paris!

[Nikolas Zimmermann]

(In reply to comment #14)
> > Well, we're sitting at SVG Open, and dicussed it face to face.
> 
> I was wondering how you two were communicating living in separate countries and all. :)
Oh, Dirk is also living in germany, so we share the regular working times (8am-6pm).
You didn't know that? :-)

> > Reading that comment really makes me sad. I'm devoting all my time in SVG and no one else (except Dirk, and you) seems to care, since years. Your comment reads like I'm guilty for that.
> 
> I'm sorry to make you sad.  That's not my intention.
It's okay, I may have overreacted. It's just that I heard the MS crowd has 19 full-time people working on SVG, and we're just two :-)
 
> > I agree, that the ChangeLog is short, and I'll fix it up.
> 
> Thank you.
> 
> I should also note, that the rollout was *completely* unrelated to my earlier comments.  Adam attempted to roll out this patch with sheriffbot, due to the test breakages, and since it hung the commit queue, I came in to fix the cq (and the tree) by completing the rollout.
Okay, thanks for the clarification.
It was completly okay to roll it out, I commited w/o checking the bots, before I went to bed, sorry for that!

> 
> My question above of why this changes behavior if it's just a perf fix remains unanswered, but I assume your updated changelog will cover that.
I will definately try to split the part out, as you already said, the perf fix shouldn't affect layout tests!

> 
> Thanks again for the patch.  I hope you two are enjoying Paris!
Thanks a lot, we're having fun here :-)

[Nikolas Zimmermann]

Created attachment 66186 [details]
Initial patch

Eric, you were absolutely right. The patch still had a bug in it, which caused the pixel test & textual changes.
Note to self: Never land past midnight, after a full conference day. Sorry for that.

We applied the shadow twice in RenderSVGRoot::updateCachedBoundaries, leading to bigger boundaries, and larger clipping area, causing a pixel test change (1px wasn't clipped away anymore in the svg/css/composite-shadow* tests).
No more LayoutTests changes with this patch.

[Dirk Schulze]

Comment on attachment 66186 [details]
Initial patch

r=me. Great that you splitted the patch. Makes it easier to read.

[Nikolas Zimmermann]

Landed in r66598.