| Summary: | Add culling to RenderSVGContainer | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||||
| Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, eric, kbr, krit, ossy, rniwa, webkit.review.bot, zimmermann | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | PC | ||||||||
| OS: | OS X 10.5 | ||||||||
| Bug Depends on: | 44924, 44955 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Nikolas Zimmermann
2010-08-30 14:38:37 PDT
Created attachment 65961 [details]
Patch
Comment on attachment 65961 [details]
Patch
r=me
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 Comment on attachment 65961 [details]
Patch
layouted remains not an english word. :)
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. :(
(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. 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"] Reverted r66418 for reason: Causing test failures on multiple bots Committed r66449: <http://trac.webkit.org/changeset/66449> 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
(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. Why did layout tests change if this was just a perf fix? (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. (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! (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 :-) 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.
Comment on attachment 66186 [details]
Initial patch
r=me. Great that you splitted the patch. Makes it easier to read.
Landed in r66598. |