Bug 92800 - page is repainting more than it should when adding a new (offscreen) table element
: page is repainting more than it should when adding a new (offscreen) table el...
Status: NEW
: WebKit
Tables
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 20885 93901 94527
: 92258
  Show dependency treegraph
 
Reported: 2012-07-31 15:50 PST by
Modified: 2013-05-13 09:50 PST (History)


Attachments
test case (needs quartz debug to see the overdraw) (1.41 KB, text/html)
2012-07-31 15:51 PST, Eric Seidel
no flags Details
simpler test case (still requires quartz debug to see the overdraw) (1.41 KB, text/html)
2012-07-31 17:07 PST, Eric Seidel
no flags Details
incomplete wip (10.76 KB, patch)
2012-07-31 20:12 PST, Eric Seidel
no flags Review Patch | Details | Formatted Diff | Diff
wip, expected to fail a few pixel tests (21.56 KB, patch)
2012-07-31 23:56 PST, Eric Seidel
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (1.34 MB, application/zip)
2012-08-01 10:16 PST, WebKit Review Bot
no flags Details
Fix float underpainting, still should fail SVG tests (22.25 KB, patch)
2012-08-01 11:57 PST, Eric Seidel
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (1.55 MB, application/zip)
2012-08-01 14:02 PST, WebKit Review Bot
no flags Details
another wip, should fail fewer tests (21.85 KB, patch)
2012-08-09 16:16 PST, Eric Seidel
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (683.21 KB, application/zip)
2012-08-09 17:17 PST, WebKit Review Bot
no flags Details
Another speculative fix for the EWS bots to chew on (77.09 KB, patch)
2012-08-10 15:02 PST, Eric Seidel
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (342.80 KB, application/zip)
2012-08-10 17:21 PST, WebKit Review Bot
no flags Details
Speculative fix disabling layoutDelta for the moment (77.79 KB, patch)
2012-08-13 16:47 PST, Eric Seidel
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (457.38 KB, application/zip)
2012-08-13 18:25 PST, WebKit Review Bot
no flags Details


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-07-31 15:50:42 PST
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.
------- Comment #1 From 2012-07-31 15:51:08 PST -------
Created an attachment (id=155656) [details]
test case (needs quartz debug to see the overdraw)
------- Comment #2 From 2012-07-31 15:58:42 PST -------
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.
------- Comment #3 From 2012-07-31 15:59:07 PST -------
Btw, it's not the table which is issuing the repaints, it seems it's the cells.
------- Comment #4 From 2012-07-31 15:59:07 PST -------
Btw, it's not the table which is issuing the repaints, it seems it's the cells.
------- Comment #5 From 2012-07-31 16:03:08 PST -------
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)
------- Comment #6 From 2012-07-31 16:06:18 PST -------
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).
------- Comment #7 From 2012-07-31 16:20:51 PST -------
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).
------- Comment #8 From 2012-07-31 16:27:53 PST -------
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.
------- Comment #9 From 2012-07-31 16:30:25 PST -------
(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();
}
------- Comment #10 From 2012-07-31 16:37:34 PST -------
(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. :)
------- Comment #11 From 2012-07-31 16:58:05 PST -------
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.
------- Comment #12 From 2012-07-31 17:04:30 PST -------
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.
------- Comment #13 From 2012-07-31 17:07:42 PST -------
Created an attachment (id=155680) [details]
simpler test case (still requires quartz debug to see the overdraw)
------- Comment #14 From 2012-07-31 17:22:19 PST -------
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. :(
------- Comment #15 From 2012-07-31 17:25:31 PST -------
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. :)
------- Comment #16 From 2012-07-31 17:29:40 PST -------
> 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).
------- Comment #17 From 2012-07-31 20:12:51 PST -------
Created an attachment (id=155710) [details]
incomplete wip
------- Comment #18 From 2012-07-31 20:16:35 PST -------
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.
------- Comment #19 From 2012-07-31 21:03:44 PST -------
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.
------- Comment #20 From 2012-07-31 23:56:33 PST -------
Created an attachment (id=155731) [details]
wip, expected to fail a few pixel tests
------- Comment #21 From 2012-08-01 10:16:20 PST -------
(From update of attachment 155731 [details])
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
------- Comment #22 From 2012-08-01 10:16:25 PST -------
Created an attachment (id=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
------- Comment #23 From 2012-08-01 11:57:03 PST -------
Created an attachment (id=155850) [details]
Fix float underpainting, still should fail SVG tests
------- Comment #24 From 2012-08-01 14:02:47 PST -------
(From update of attachment 155850 [details])
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
------- Comment #25 From 2012-08-01 14:02:52 PST -------
Created an attachment (id=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
------- Comment #26 From 2012-08-06 14:00:54 PST -------
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.
------- Comment #27 From 2012-08-09 16:16:56 PST -------
Created an attachment (id=157573) [details]
another wip, should fail fewer tests
------- Comment #28 From 2012-08-09 16:17:35 PST -------
(From update of attachment 157573 [details])
Didn't mean to set the review flags.
------- Comment #29 From 2012-08-09 16:55:46 PST -------
I expect there to be a few failures still, which I will address tomorrow/this-evening.
------- Comment #30 From 2012-08-09 17:17:07 PST -------
(From update of attachment 157573 [details])
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
------- Comment #31 From 2012-08-09 17:17:12 PST -------
Created an attachment (id=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
------- Comment #32 From 2012-08-10 03:37:26 PST -------
*** Bug 93703 has been marked as a duplicate of this bug. ***
------- Comment #33 From 2012-08-10 03:44:56 PST -------
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
------- Comment #34 From 2012-08-10 15:02:25 PST -------
Created an attachment (id=157816) [details]
Another speculative fix for the EWS bots to chew on
------- Comment #35 From 2012-08-10 17:21:28 PST -------
(From update of attachment 157816 [details])
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
------- Comment #36 From 2012-08-10 17:21:32 PST -------
Created an attachment (id=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
------- Comment #37 From 2012-08-13 16:47:10 PST -------
Created an attachment (id=158146) [details]
Speculative fix disabling layoutDelta for the moment
------- Comment #38 From 2012-08-13 18:25:16 PST -------
(From update of attachment 158146 [details])
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
------- Comment #39 From 2012-08-13 18:25:22 PST -------
Created an attachment (id=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
------- Comment #40 From 2012-08-20 13:00:40 PST -------
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"
------- Comment #41 From 2012-08-20 19:07:32 PST -------
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.
------- Comment #42 From 2012-08-20 19:21:49 PST -------
FYI: The current fix for bug 94527 does cause the overpainting seen in performance-tests/biggrid.html (progressive mode) to go away. :)