Table test is invalidating the wrong rect Not quite sure how this is occurring, but I believe this to be related to larger repaint issues seen with: https://github.com/dglazkov/performance-tests/blob/master/biggrid.html In this reduction, note that every time we add a new table, the first table in the parent div redraws. This is an unnecessary invalidation causing overdrawing.
Created attachment 155656 [details] test case (needs quartz debug to see the overdraw)
I believe the extra draws are coming from the newly added table attempting to invalidate it's "old bounds" after it's initial layout. It's not clear to me what systems we have in place to prevent LayoutRepainter from attempting to invalidate "old bounds" for an object which is doing it's first layout/never painted before.
Btw, it's not the table which is issuing the repaints, it seems it's the cells.
Yes, I've confirmed that the cells which are issuing the repaints are inside a table (which is inside a div), which has no "next" children, thus must be the last table in the set, indicating it's the newest one. Yet it's attempting to invalidate rects like (0,8),(245, 56) (which is actually the full table layout rect)
Actually, the line issuing the repaint is attempting to repaint the "new" bounds. repaintUsingContainer(repaintContainer, newBounds); newBounds: (0,8),(245, 56) Which doesn't make sense, as this table was just added (and should be scrolled offscreen anyway).
Actually, I believe it's the blocks inside the cells which are causing the repaint. The cells are empty, so they should't need to paint anything (except maybe a background, etc.) This is the first layout of the table, so the cells don't actually know where they are yet, it seems. Or at least they're laying out from 0,0. The RenderTable's frameRect at this time is (0,0), (408, 0). The RenderTable's parent div's framerect is (0, 1216), (681, 0) (which looks reasonable for this stage of layout).
It appears that RenderBlock::layoutBlockChild doesn't set the child's logical top until after its laid out the block child. In this case, after the table has been laid out. So this explains why the table is laying itself out at 0,0, including the cells. What's not clear to me is how render objects normally prevent themselves from not invalidating their "previous" paint position when painting for the first time. RenderBlock::layoutBlock has this: LayoutRepainter repainter(*this, everHadLayout() && checkForRepaintDuringLayout()); everHadLayout() is presumably supposed to disable the repaint check for the first layout? In the callstack I'm seeing the repaint call from, the render block inside the table cell has m_everHadLayout == 1. Unclear why.
(In reply to comment #8) > It appears that RenderBlock::layoutBlockChild doesn't set the child's logical top until after its laid out the block child. In this case, after the table has been laid out. > > So this explains why the table is laying itself out at 0,0, including the cells. What's not clear to me is how render objects normally prevent themselves from not invalidating their "previous" paint position when painting for the first time. > > RenderBlock::layoutBlock has this: > > LayoutRepainter repainter(*this, everHadLayout() && checkForRepaintDuringLayout()); > > everHadLayout() is presumably supposed to disable the repaint check for the first layout? > > In the callstack I'm seeing the repaint call from, the render block inside the table cell has m_everHadLayout == 1. Unclear why. Interesting... bool RenderObject::checkForRepaintDuringLayout() const { // FIXME: <https://bugs.webkit.org/show_bug.cgi?id=20885> It is probably safe to also require // m_everHadLayout. Currently, only RenderBlock::layoutBlock() adds this condition. See also // <https://bugs.webkit.org/show_bug.cgi?id=15129>. return !document()->view()->needsFullRepaint() && !hasLayer(); }
(In reply to comment #9) > (In reply to comment #8) > Interesting... > > bool RenderObject::checkForRepaintDuringLayout() const > { > // FIXME: <https://bugs.webkit.org/show_bug.cgi?id=20885> It is probably safe to also require > // m_everHadLayout. Currently, only RenderBlock::layoutBlock() adds this condition. See also > // <https://bugs.webkit.org/show_bug.cgi?id=15129>. > return !document()->view()->needsFullRepaint() && !hasLayer(); > } I should note that fixing that FIXME does not fix this bug. :)
It appears that when RenderTable does a layout, it first tells its sections to layout themselves: for (RenderObject* child = firstChild(); child; child = child->nextSibling()) { if (child->isTableSection()) { child->layoutIfNeeded(); RenderTableSection* section = toRenderTableSection(child); totalSectionLogicalHeight += section->calcRowLogicalHeight(); if (collapsing) section->recalcOuterBorder(); ASSERT(!section->needsLayout()); } else if (child->isRenderTableCol()) { child->layoutIfNeeded(); ASSERT(!child->needsLayout()); } } And then a few lines later, again: for (RenderTableSection* section = topSection(); section; section = sectionBelow(section)) section->layoutRows(); I'm not sure why RenderTable asks its sections to layout twice. The first time, it goes through RenderTableSection::layout() and the second time through RenderTableSection::layoutRows(). both times it ends up calling cell->layoutIfNeeded(). So clearly somehow the cells are being set as needing layout in between the two layout() calls. Still investigating. But this clearly explains the overpainting I'm seeing.
This is the line where the cell gets marked for relayout: if (intrinsicPaddingBefore != oldIntrinsicPaddingBefore || intrinsicPaddingAfter != oldIntrinsicPaddingAfter) cell->setNeedsLayout(true, MarkOnlyThis); inside RenderTableSection::layoutRows(). In this case, the new intrinsic padding is 148 (in layout units), and was previous 0. I'm not entirely sure what that means. I guess this brings us back to: Why do Table Cells need to layout twice? Can we kill the first layout? Maybe we should have TableCells check their parent table for their "checkForRepaintDuringLayout()" status. They'd have to propagate this status down into their children, which could get awkward.
Created attachment 155680 [details] simpler test case (still requires quartz debug to see the overdraw)
I believe this to cause at least 10% of total time spent in Dimitri's benchmark (which is supposed to mimimic what spreadsheets.google.com does): https://github.com/dglazkov/performance-tests/blob/master/biggrid.html In "progressive" mode, it adds one 50x25 cell table at a time. Since that table is about the size of my window, this bug is causing the entire window to repaint every time a table is added. Even though each of these tables beyond the first is completely offscreen. :(
The default version of the benchmark is 10000 by 25. Which means we're doing (10000 / 50 - 1) = 199 extra full-screen repaints. I suspect that's actually waaay more than 10% of the benchmark, even though that was what my back-of-the-hand instruments calculation told me. :)
> I guess this brings us back to: Why do Table Cells need to layout twice? Can we kill the first layout? We need to do the 2 layouts. Long explanation: You need to do a first layout so that you can determine your table columns and rows sizes: * the column part is due to auto table layout as it takes into account all the cell's width to determine a column width. * the row part is because the row height is determined by the highest cell height in the row. Once we have determined the column and row sizes, we need to: * place the cell in its row according to its 'vertical-align' property (that's what intrinsic padding is for). This may change the cell's height and thus could require another layout. * Force the cell to fit in its column width. The code tries to avoid unneeded relayouts (for example the intrinsic padding check you pointed out) but I know that there are some cases where we over-do (fixed table layout is an example where we could be smarter).
Created attachment 155710 [details] incomplete wip
RenderObject says that all 32 bitfields are in use, but that's a LIE. Only 31 are. So I used the last one in this patch. This introduces a new concept of "having painted", which is what checkForRepaintDuringLayout really wants to use instead of everHadLayout() (since we only want to invalidate old paint rects if we ever actually painted!). The current WIP patch won't actually work, since I didn't add all the setEverDidPaint(true) calls that it will eventually need, and likely it ASSERTs in Debug builds as-is. In any case, it makes the test case not over-paint, the question remains if it makes other test cases underpaint. I think if this approach works, that it will be much more accurate than the previous everHadLayout approach. I did not remove everHadLayout as there appear to be some non-repaint usages, but those may be sub-optimal too. Donno yet.
I'm still not sure if the patch is fully correct yet, but the perf numbers from it look amazing. biggrid.html 10000x25 table, progressive: before: FPS: 40 Elapsed: 5535ms after: FPS: 55 Elapsed: 3701ms Will upload a full patch shortly.
Created attachment 155731 [details] wip, expected to fail a few pixel tests
Comment on attachment 155731 [details] wip, expected to fail a few pixel tests Attachment 155731 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13404502 New failing tests: svg/custom/js-late-clipPath-creation.svg fast/repaint/table-outer-border.html svg/hixie/perf/004.xml svg/hixie/perf/005.xml svg/custom/js-late-clipPath-and-object-creation.svg svg/carto.net/window.svg svg/custom/js-late-mask-and-object-creation.svg fast/repaint/float-new-in-block.html fast/repaint/table-row.html fast/repaint/block-layout-inline-children-float-positioned.html fast/repaint/inline-block-resize.html fast/repaint/float-in-new-block-with-layout-delta.html svg/custom/js-late-mask-creation.svg svg/custom/use-detach.svg svg/hixie/perf/006.xml svg/custom/js-late-marker-creation.svg fast/table/border-collapsing/cached-cell-append.html fast/table/border-collapsing/cached-change-row-border-width.html
Created attachment 155828 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 155850 [details] Fix float underpainting, still should fail SVG tests
Comment on attachment 155850 [details] Fix float underpainting, still should fail SVG tests Attachment 155850 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13421176 New failing tests: svg/custom/js-late-clipPath-creation.svg fast/repaint/table-outer-border.html svg/hixie/perf/004.xml svg/hixie/perf/005.xml svg/custom/js-late-clipPath-and-object-creation.svg svg/carto.net/window.svg svg/custom/js-late-mask-and-object-creation.svg fast/repaint/table-row.html fast/repaint/block-layout-inline-children-float-positioned.html fast/repaint/inline-block-resize.html fast/repaint/float-in-new-block-with-layout-delta.html svg/custom/js-late-mask-creation.svg svg/custom/use-detach.svg svg/hixie/perf/006.xml svg/custom/js-late-marker-creation.svg fast/table/border-collapsing/cached-cell-append.html fast/table/border-collapsing/cached-change-row-border-width.html
Created attachment 155880 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
I tried fixing bug 20885, and saw (almost) all of these same failures. My current understanding is that in CSS/HTML land, your parent container manages your first paint and you manage ever successive repaint. That's not how SVG (currently) works, so I may need to fix that in order to fix bug 20885/this bug. An alternate way to fix this bug might be to just have the block position its children before telling them to layout. That wouldn't fix the double-paint in the general case for tables, but would fix this specific case, and fix the problem of off-screen tables causing full-screen repaints. Only on-screen tables would then cause double-painting.
Created attachment 157573 [details] another wip, should fail fewer tests
Comment on attachment 157573 [details] another wip, should fail fewer tests Didn't mean to set the review flags.
I expect there to be a few failures still, which I will address tomorrow/this-evening.
Comment on attachment 157573 [details] another wip, should fail fewer tests Attachment 157573 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13473147 New failing tests: fast/repaint/table-outer-border.html fast/repaint/block-layout-inline-children-float-positioned.html fast/table/border-collapsing/cached-cell-append.html fast/repaint/table-row.html fast/repaint/inline-block-resize.html fast/repaint/float-in-new-block-with-layout-delta.html fast/table/border-collapsing/cached-change-row-border-width.html
Created attachment 157591 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
*** Bug 93703 has been marked as a duplicate of this bug. ***
Very close. Still investigating 3 failures: fast/repaint/block-layout-inline-children-float-positioned.html = IMAGE fast/repaint/table-row.html = IMAGE fast/table/border-collapsing/cached-change-row-border-width.html = IMAGE
Created attachment 157816 [details] Another speculative fix for the EWS bots to chew on
Comment on attachment 157816 [details] Another speculative fix for the EWS bots to chew on Attachment 157816 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13476391 New failing tests: fast/repaint/table-outer-border.html fast/repaint/block-layout-inline-children-float-positioned.html fast/repaint/float-new-in-block.html fast/table/border-collapsing/cached-change-row-border-width.html
Created attachment 157843 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 158146 [details] Speculative fix disabling layoutDelta for the moment
Comment on attachment 158146 [details] Speculative fix disabling layoutDelta for the moment Attachment 158146 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13483950 New failing tests: fast/repaint/table-outer-border.html fast/repaint/block-layout-inline-children-float-positioned.html fast/repaint/float-new-in-block.html fast/repaint/table-cell-move.html fast/repaint/table-extra-bottom-grow.html fast/repaint/bugzilla-3509.html fast/table/border-collapsing/cached-change-row-border-width.html
Created attachment 158177 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
This diff also fixes the bug, but causes other repaint failures: diff --git a/Source/WebCore/rendering/RenderBlock.cpp b/Source/WebCore/rendering/RenderBlock.cpp index 918421b..ce5c003 100755 --- a/Source/WebCore/rendering/RenderBlock.cpp +++ b/Source/WebCore/rendering/RenderBlock.cpp @@ -2261,6 +2261,8 @@ void RenderBlock::handleAfterSideOfBlock(LayoutUnit beforeSide, LayoutUnit after void RenderBlock::setLogicalLeftForChild(RenderBox* child, LayoutUnit logicalLeft, ApplyLayoutDeltaMode applyDelta) { + if (!child->everHadLayout()) + applyDelta = DoNotApplyLayoutDelta; if (isHorizontalWritingMode()) { if (applyDelta == ApplyLayoutDelta) view()->addLayoutDelta(LayoutSize(child->x() - logicalLeft, 0)); @@ -2274,6 +2276,8 @@ void RenderBlock::setLogicalLeftForChild(RenderBox* child, LayoutUnit logicalLef void RenderBlock::setLogicalTopForChild(RenderBox* child, LayoutUnit logicalTop, ApplyLayoutDeltaMode applyDelta) { + if (!child->everHadLayout()) + applyDelta = DoNotApplyLayoutDelta; if (isHorizontalWritingMode()) { if (applyDelta == ApplyLayoutDelta) view()->addLayoutDelta(LayoutSize(0, child->y() - logicalTop)); This is all related to our inability to distinguish "current bounds" vs. "old bounds" repaint requests during layout. The diff above "fixes" the bug by not adding the layout delta during the first repaint, effectively making all "old bound" requests just return "current bounds"
It looks like bug 94527 may fix this. If it does, then I'll close this as a dupe. There is still a win to be had with the everDidPaint() bool, but it's not nearly as big after bug 94527.
FYI: The current fix for bug 94527 does cause the overpainting seen in performance-tests/biggrid.html (progressive mode) to go away. :)