Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2630723002:
Fix blink_perf.paint regression about tables (Closed)
DescriptionFix blink_perf.paint regression about tables
Performance was getting down because vector is accessing very frequently
which
becomes very costly when vector size is very big.
PrimaryCellAt() is using very frequently and it was accessed same vector twice (first
time used by numCols() and another one directly.). So now vector is accessed
once and same reference is used for both statements/operations.
numCols() is accessing the vector for getting the vector size and it
was used
in the loop iteration. So now it calls only once before the loop.
BUG=679530
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2630723002
Cr-Commit-Position: refs/heads/master@{#447786}
Committed: https://chromium.googlesource.com/chromium/src/+/3173daa005795ac280b23e2f89c612f0d93892fb
Patch Set 1 #Patch Set 2 : Undo changes #Patch Set 3 : Remove condition from primaryCellAt() #Patch Set 4 : remove condition from primaryCellAt() and keep numCols() out of loop iteration. #Patch Set 5 : Issues fixed in patch set 4 #
Total comments: 9
Patch Set 6 : Review comments addressed. #
Total comments: 2
Patch Set 7 : Review comments addressed #Patch Set 8 : Rebase #Patch Set 9 : Nits #
Messages
Total messages: 50 (29 generated)
Description was changed from ========== Fix Slimming paint regression BUG=679530 ========== to ========== Fix Slimming paint regression BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Fix Slimming paint regression BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix Slimming paint regression BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
a.suchit@samsung.com changed reviewers: + wangxianzhu@chromium.org
a.suchit@samsung.com changed reviewers: + mstensho@opera.com
Please review.
Description was changed from ========== Fix Slimming paint regression BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix blink_perf.paint regression about tables BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
lgtm I updated the subject.
On 2017/01/20 03:32:23, Xianzhu wrote: > lgtm > > I updated the subject. Is the performance fine in this patch ? I can not check it here.
On 2017/01/20 03:35:17, a.suchit wrote: > On 2017/01/20 03:32:23, Xianzhu wrote: > > lgtm > > > > I updated the subject. > > Is the performance fine in this patch ? > I can not check it here. And I need the lgtm from mstensho@opera.com as well.
The CQ bit was checked by a.suchit@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/20 03:35:17, a.suchit wrote: > On 2017/01/20 03:32:23, Xianzhu wrote: > > lgtm > > > > I updated the subject. > > Is the performance fine in this patch ? > I can not check it here. I'm home now and also can not check it. Will check it tomorrow. Asked perf team about the access restrictions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/20 03:43:44, Xianzhu wrote: > On 2017/01/20 03:35:17, a.suchit wrote: > > On 2017/01/20 03:32:23, Xianzhu wrote: > > > lgtm > > > > > > I updated the subject. > > > > Is the performance fine in this patch ? > > I can not check it here. > > I'm home now and also can not check it. Will check it tomorrow. Asked perf team > about the access restrictions. I can access the results now. There seems no improvement for large-table-background-change-with-invisible-collapsed-borders.html, but seem improvement between 0.x%~4% for other table tests (which might be real improvement or just noise). I suggest to land this CL and see the trend. About performance of large-table-background-change-with-invisible-collapsed-borders.html, I guess there is still new code in inner loops, for example, in LayoutTable::recalcCollapsedBordersInNeeded() -> LayoutTableCell::collectBorderValues() -> LayoutTable::cellBefore/cellAfter/cellAbove/cellBelow. It seems hard to extract more code from the inner loops, so I think we can just accept the performance regression.
On 2017/01/20 05:10:56, Xianzhu wrote: > On 2017/01/20 03:43:44, Xianzhu wrote: > > On 2017/01/20 03:35:17, a.suchit wrote: > > > On 2017/01/20 03:32:23, Xianzhu wrote: > > > > lgtm > > > > > > > > I updated the subject. > > > > > > Is the performance fine in this patch ? > > > I can not check it here. > > > > I'm home now and also can not check it. Will check it tomorrow. Asked perf > team > > about the access restrictions. > > I can access the results now. There seems no improvement for > large-table-background-change-with-invisible-collapsed-borders.html, but seem > improvement between 0.x%~4% for other table tests (which might be real > improvement or just noise). I suggest to land this CL and see the trend. > > About performance of > large-table-background-change-with-invisible-collapsed-borders.html, I guess > there is still new code in inner loops, for example, in > LayoutTable::recalcCollapsedBordersInNeeded() -> > LayoutTableCell::collectBorderValues() -> > LayoutTable::cellBefore/cellAfter/cellAbove/cellBelow. It seems hard to extract > more code from the inner loops, so I think we can just accept the performance > regression. ok fine. @mstensho Please check the changes in this code once before land this patch. All this regression is coming because in the previous patch we have varied the number of cells in each row and which is required to check the number of column condition all the places. As a result, we are getting performance regression but other side we are getting performance improvement in other table test cases with this patch. Please share your opinion in it.
I tested a bit locally (without this CL applied), running the performance test: Before the patch that caused the regression (1060f916d9f7026fa22dd3bb6b563294ec7f8224): avg 89.44450000000003 ms After the patch that caused the regression: avg 91.88000000000002 ms Like I'm writing further below, I did't understand why the primaryCellAt() changes caused such a slow-down, but I'm guessing now that it's all thanks to things not being const. So you end up having to read out m_grid[row].row twice, making it about twice as slow, I guess. If I make the changes to the primaryCell() implementation that I suggest below, I get this: avg 81.18600000000002 ms I.e. way faster than ever! :) That's a bit puzzling, but I suppose it has to do with no longer creating a CellStruct reference. So it looks like we can keep the vector bounds check in primaryCellAt(), if we just make it fast enough. https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right): https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTable.cpp:1493: return section->primaryCellAt(cell->rowIndex(), effCol - 1); This is just clean-up, right? https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1156: unsigned nCols = numCols(r); These are good. I was meaning to point it out in the initial review, but I filed it under nit-picking, so I didn't say anything. :( https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.h (left): https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.h:221: if (effectiveColumn >= numCols(row)) I find it strange that this makes a lot of difference. numCols() looks cheap. Does it help if you rewrite primaryCellAt() as: Row& rowVector = m_grid[row].row; if (effectiveColumn >= rowVector.size()) return nullptr; return rowVector[effectiveColumn].primaryCell(); https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right): https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:202: break; Did you mean "continue;"? We're walking backwards in the vector here. In that case, it would probably be better to initialize |c| as std::min(dirtiedColumns.end(), std::max(nCols - 1, 0)) or something like that instead. https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXTable.cpp (right): https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXTable.cpp:206: int nCols = firstBody->numCols(row); Now there's both numCols and nCols, which is confusing. Maybe you should rename the original numCols to numColsInFirstBody instead, and declare numCols here instead.
https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right): https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTable.cpp:1493: return section->primaryCellAt(cell->rowIndex(), effCol - 1); On 2017/01/20 10:02:04, mstensho wrote: > This is just clean-up, right? yes https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.h (left): https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.h:221: if (effectiveColumn >= numCols(row)) On 2017/01/20 10:02:04, mstensho wrote: > I find it strange that this makes a lot of difference. numCols() looks cheap. > > Does it help if you rewrite primaryCellAt() as: > > Row& rowVector = m_grid[row].row; > if (effectiveColumn >= rowVector.size()) > return nullptr; > return rowVector[effectiveColumn].primaryCell(); Done. https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right): https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:202: break; On 2017/01/20 10:02:04, mstensho wrote: > Did you mean "continue;"? We're walking backwards in the vector here. > > In that case, it would probably be better to initialize |c| as > std::min(dirtiedColumns.end(), std::max(nCols - 1, 0)) or something like that > instead. Yes, It should be continue. Thanks Done. https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXTable.cpp (right): https://codereview.chromium.org/2630723002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXTable.cpp:206: int nCols = firstBody->numCols(row); On 2017/01/20 10:02:04, mstensho wrote: > Now there's both numCols and nCols, which is confusing. Maybe you should rename > the original numCols to numColsInFirstBody instead, and declare numCols here > instead. Done.
Some explanatory text in the description that says what you're doing here would be good. https://codereview.chromium.org/2630723002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right): https://codereview.chromium.org/2630723002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:193: for (unsigned c = dirtiedColumns.end(); c > dirtiedColumns.start(); c--) { unsigned c = std::min(dirtiedColumns.end(), nCols) Instead of the continue thing below.
Description was changed from ========== Fix blink_perf.paint regression about tables BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix blink_perf.paint regression about tables Performance was getting down because vector table was accesses very frequently which becomes very costly when vector size is very big. PrimaryCellAt() was used very frequently and it accessed same vector table twice (first time used by numCols() and another one directly.). So Aceesed the vector table once and same reference was used for both statements/operations. numCols() was accessing the vector table for getting the vector size and it was used in the loop iteration. So reduced the calling of this function by moving it out of the loop iteration. BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Fix blink_perf.paint regression about tables Performance was getting down because vector table was accesses very frequently which becomes very costly when vector size is very big. PrimaryCellAt() was used very frequently and it accessed same vector table twice (first time used by numCols() and another one directly.). So Aceesed the vector table once and same reference was used for both statements/operations. numCols() was accessing the vector table for getting the vector size and it was used in the loop iteration. So reduced the calling of this function by moving it out of the loop iteration. BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix blink_perf.paint regression about tables Performance was getting down because vector table was accesses very frequently which becomes very costly when vector size is very big. PrimaryCellAt() was used very frequently and it accessed same vector table twice (first time used by numCols() and another one directly.). So accessed the vector table once and same reference was used for both statements/operations. numCols() was accessing the vector table for getting the vector size and it was used in the loop iteration. So reduced the calling of this function by moving it out of the loop iteration. BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Fix blink_perf.paint regression about tables Performance was getting down because vector table was accesses very frequently which becomes very costly when vector size is very big. PrimaryCellAt() was used very frequently and it accessed same vector table twice (first time used by numCols() and another one directly.). So accessed the vector table once and same reference was used for both statements/operations. numCols() was accessing the vector table for getting the vector size and it was used in the loop iteration. So reduced the calling of this function by moving it out of the loop iteration. BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix blink_perf.paint regression about tables Performance was getting down because vector was accesses very frequently which becomes very costly when vector size is very big. PrimaryCellAt() was used very frequently and it accessed same vector twice (first time used by numCols() and another one directly.). So accessed the vector once and same reference was used for both statements/operations. numCols() was accessing the vector table for getting the vector size and it was used in the loop iteration. So reduced the calling of this function by moving it out of the loop iteration. BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Fix blink_perf.paint regression about tables Performance was getting down because vector was accesses very frequently which becomes very costly when vector size is very big. PrimaryCellAt() was used very frequently and it accessed same vector twice (first time used by numCols() and another one directly.). So accessed the vector once and same reference was used for both statements/operations. numCols() was accessing the vector table for getting the vector size and it was used in the loop iteration. So reduced the calling of this function by moving it out of the loop iteration. BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix blink_perf.paint regression about tables Performance was getting down because vector was accesses very frequently which becomes very costly when vector size is very big. PrimaryCellAt() was used very frequently and it accessed same vector twice (first time used by numCols() and another one directly.). So accessed the vector once and same reference was used for both statements/operations. numCols() was accessing the vector for getting the vector size and it was used in the loop iteration. So reduced the calling of this function by moving it out of the loop iteration. BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
https://codereview.chromium.org/2630723002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right): https://codereview.chromium.org/2630723002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:193: for (unsigned c = dirtiedColumns.end(); c > dirtiedColumns.start(); c--) { On 2017/01/23 09:24:57, mstensho wrote: > unsigned c = std::min(dirtiedColumns.end(), nCols) > > Instead of the continue thing below. Done.
The CQ bit was checked by a.suchit@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> Performance was getting down because vector was accesses very frequently > which becomes very costly when vector size is very big. Vector size shouldn't matter at all. size() just returns an integer member.
On 2017/01/23 11:43:02, mstensho wrote: > > Performance was getting down because vector was accesses very frequently > > which becomes very costly when vector size is very big. > > Vector size shouldn't matter at all. size() just returns an integer member. vector size means that accessing m_grid[row] again and again if m_grid vector is very big. Right ?
On 2017/01/23 12:28:11, a.suchit wrote: > On 2017/01/23 11:43:02, mstensho wrote: > > > Performance was getting down because vector was accesses very frequently > > > which becomes very costly when vector size is very big. > > > > Vector size shouldn't matter at all. size() just returns an integer member. > > vector size means that accessing m_grid[row] again and again if m_grid vector > is very big. Right ? The number of calls to primaryCellAt() or numCols() is/was a linear function of the vector size in many cases, so in that sense you're right (and you got rid of a few of those, by moving numCols() out of the loop). To me, your text implies that vector access is slower if the vector is big, and that's certainly not the issue. One typo: "because vector was accesses" -> "because vector was accessed" And this: > PrimaryCellAt() was used very frequently It's still used exactly as frequently as before, isn't it? It's just that it's much faster now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Fix blink_perf.paint regression about tables Performance was getting down because vector was accesses very frequently which becomes very costly when vector size is very big. PrimaryCellAt() was used very frequently and it accessed same vector twice (first time used by numCols() and another one directly.). So accessed the vector once and same reference was used for both statements/operations. numCols() was accessing the vector for getting the vector size and it was used in the loop iteration. So reduced the calling of this function by moving it out of the loop iteration. BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix blink_perf.paint regression about tables Performance was getting down because vector is accessing very frequently which becomes very costly when vector size is very big. PrimaryCellAt() is using very frequently and it was accessed same vector twice (first time used by numCols() and another one directly.). So now vector is accessed once and same reference is used for both statements/operations. numCols() is accessing the vector for getting the vector size and it was used in the loop iteration. So now it calls only once before the loop. BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by a.suchit@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
On 2017/01/23 12:43:19, mstensho wrote: @mstenso, Please review it.
lgtm
The CQ bit was checked by a.suchit@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from wangxianzhu@chromium.org Link to the patchset: https://codereview.chromium.org/2630723002/#ps160001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
a.suchit@samsung.com changed reviewers: + aboxhall@chromium.org, dmazzoni@chromium.org, esprehn@chromium.org
lgtm Thanks for the improvements to AXTable
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1486056210358130,
"parent_rev": "16f22212459156cdc38dbe41a31cd86840c89b5a", "commit_rev":
"3173daa005795ac280b23e2f89c612f0d93892fb"}
Message was sent while issue was closed.
Description was changed from ========== Fix blink_perf.paint regression about tables Performance was getting down because vector is accessing very frequently which becomes very costly when vector size is very big. PrimaryCellAt() is using very frequently and it was accessed same vector twice (first time used by numCols() and another one directly.). So now vector is accessed once and same reference is used for both statements/operations. numCols() is accessing the vector for getting the vector size and it was used in the loop iteration. So now it calls only once before the loop. BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix blink_perf.paint regression about tables Performance was getting down because vector is accessing very frequently which becomes very costly when vector size is very big. PrimaryCellAt() is using very frequently and it was accessed same vector twice (first time used by numCols() and another one directly.). So now vector is accessed once and same reference is used for both statements/operations. numCols() is accessing the vector for getting the vector size and it was used in the loop iteration. So now it calls only once before the loop. BUG=679530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2630723002 Cr-Commit-Position: refs/heads/master@{#447786} Committed: https://chromium.googlesource.com/chromium/src/+/3173daa005795ac280b23e2f89c6... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/3173daa005795ac280b23e2f89c6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
