Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1695193006:
Performance test for deeply nested tables inside multicol. (Closed)
|
Created:
4 years, 10 months ago by mstensho (USE GERRIT) Modified:
4 years, 2 months ago CC:
chromium-reviews, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPerformance test for deeply nested tables inside multicol.
This is similar to printing deeply nested tables, and we have serious
performance issues with this.
Landing the performance test separately from the actual fix, so that we can
observe the improvement when the fix eventually lands.
BUG=487026
Committed: https://crrev.com/f1064efe79837bd304344cc1125ec8ec8c7ff898
Cr-Commit-Position: refs/heads/master@{#426461}
Patch Set 1 #Patch Set 2 : Unprefix multicol properties. #
Total comments: 5
Patch Set 3 : Change width instead of toggling display:none, shorter container, avoid line layout, avoid painting. #Messages
Total messages: 17 (7 generated)
mstensho@opera.com changed reviewers: + rune@opera.com
My computer can currently run this test about 5 times per second. I'm working on a fix that should make it at least a hundred times faster.
We should be careful not adding tests that could become noisy. It would be good if you could do a callgrind to see if this test will be sensitive to code changes after your fix. Someone with more knowledge about table layout than me should review.
Message was sent while issue was closed.
Description was changed from ========== Performance test for deeply nested tables inside multicol. This is similar to printing deeply nested tables, and we have serious performance issues with this. Landing the performance test separately from the actual fix, so that we can observe the improvement when the fix eventually lands. BUG=487026 ========== to ========== Performance test for deeply nested tables inside multicol. This is similar to printing deeply nested tables, and we have serious performance issues with this. Landing the performance test separately from the actual fix, so that we can observe the improvement when the fix eventually lands. BUG=487026 ==========
mstensho@opera.com changed reviewers: + eae@chromium.org
mstensho@opera.com changed reviewers: + dgrogan@chromium.org
I don't think this test would be particularly noisy, but what do I know... Anyway, I expect the fragmentation performance improvement that this one measures to be submitted pretty soon, so I'm asking for another review.
On 2016/10/19 20:50:23, mstensho wrote: > I don't think this test would be particularly noisy, but what do I know... > > Anyway, I expect the fragmentation performance improvement that this one > measures to be submitted pretty soon, so I'm asking for another review. What's interesting is what it measures after your change. It looks likely that it will mostly measure layout tree construction and table layout, so that seems fine to me.
https://codereview.chromium.org/1695193006/diff/20001/third_party/WebKit/Perf... File third_party/WebKit/PerformanceTests/Layout/multicol/deeply-nested-tables.html (right): https://codereview.chromium.org/1695193006/diff/20001/third_party/WebKit/Perf... third_party/WebKit/PerformanceTests/Layout/multicol/deeply-nested-tables.html:7: <div id="target" style="display:none; columns:3; column-fill:auto; width:40em; height:41.5em; line-height:2em;"> 41.5em seems arbitrary, or is it picked to fit the contents below? https://codereview.chromium.org/1695193006/diff/20001/third_party/WebKit/Perf... third_party/WebKit/PerformanceTests/Layout/multicol/deeply-nested-tables.html:60: description: "Measures performance of multicol layout in deeply nested tables.", Is the time spent in layout tree building a lot smaller than layout even after your perf fix? Does it make sense to focus this test on layout by e.g. changing widths instead of rebuilding the layout tree on every iteration?
https://codereview.chromium.org/1695193006/diff/20001/third_party/WebKit/Perf... File third_party/WebKit/PerformanceTests/Layout/multicol/deeply-nested-tables.html (right): https://codereview.chromium.org/1695193006/diff/20001/third_party/WebKit/Perf... third_party/WebKit/PerformanceTests/Layout/multicol/deeply-nested-tables.html:7: <div id="target" style="display:none; columns:3; column-fill:auto; width:40em; height:41.5em; line-height:2em;"> On 2016/10/19 23:03:00, rune wrote: > 41.5em seems arbitrary, or is it picked to fit the contents below? Pretty arbitrary, although it's kind of nice that 41.5em % line height != 0, so that we get pagination struts. Then again, there's not even enough content here to use more than one column. I'll make it shorter. 13em seems like the lucky number. https://codereview.chromium.org/1695193006/diff/20001/third_party/WebKit/Perf... third_party/WebKit/PerformanceTests/Layout/multicol/deeply-nested-tables.html:60: description: "Measures performance of multicol layout in deeply nested tables.", On 2016/10/19 23:03:00, rune wrote: > Is the time spent in layout tree building a lot smaller than layout even after > your perf fix? Tree building should be unaffected by my fix. > Does it make sense to focus this test on layout by e.g. changing widths instead > of rebuilding the layout tree on every iteration? This is a very good point. Done. I also removed the need for line layout in the upcoming patch. Unbreakable blocks serve the same purpose.
lgtm https://codereview.chromium.org/1695193006/diff/20001/third_party/WebKit/Perf... File third_party/WebKit/PerformanceTests/Layout/multicol/deeply-nested-tables.html (right): https://codereview.chromium.org/1695193006/diff/20001/third_party/WebKit/Perf... third_party/WebKit/PerformanceTests/Layout/multicol/deeply-nested-tables.html:60: description: "Measures performance of multicol layout in deeply nested tables.", On 2016/10/20 09:43:02, mstensho wrote: > On 2016/10/19 23:03:00, rune wrote: > > Is the time spent in layout tree building a lot smaller than layout even after > > your perf fix? > > Tree building should be unaffected by my fix. My point was if the test really measures layout tree building as much as layout after your fix.
The CQ bit was checked by mstensho@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Performance test for deeply nested tables inside multicol. This is similar to printing deeply nested tables, and we have serious performance issues with this. Landing the performance test separately from the actual fix, so that we can observe the improvement when the fix eventually lands. BUG=487026 ========== to ========== Performance test for deeply nested tables inside multicol. This is similar to printing deeply nested tables, and we have serious performance issues with this. Landing the performance test separately from the actual fix, so that we can observe the improvement when the fix eventually lands. BUG=487026 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Performance test for deeply nested tables inside multicol. This is similar to printing deeply nested tables, and we have serious performance issues with this. Landing the performance test separately from the actual fix, so that we can observe the improvement when the fix eventually lands. BUG=487026 ========== to ========== Performance test for deeply nested tables inside multicol. This is similar to printing deeply nested tables, and we have serious performance issues with this. Landing the performance test separately from the actual fix, so that we can observe the improvement when the fix eventually lands. BUG=487026 Committed: https://crrev.com/f1064efe79837bd304344cc1125ec8ec8c7ff898 Cr-Commit-Position: refs/heads/master@{#426461} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f1064efe79837bd304344cc1125ec8ec8c7ff898 Cr-Commit-Position: refs/heads/master@{#426461} |
