[Webkit-unassigned] [Bug 64546] Redrawing dirty parts of a large table is very slow

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=64546





--- Comment #15 from Julien Chaffraix <jchaffraix at webkit.org>  2011-09-07 11:44:38 PST ---
(From update of attachment 106582)
View in context: https://bugs.webkit.org/attachment.cgi?id=106582&action=review

Your tests should be self-explanatory (look at some other tests for reference). There is no way for people to know if your tests is passing or failing as there is no explanation as to what you are expecting.

> LayoutTests/fast/table/border-collapsing/cached-cell-append.html:10
> +                // do change

This comment does not add anything in any of the tests. It should be removed.

> LayoutTests/fast/table/border-collapsing/cached-cell-append.html:23
> +                <td style="border:4px solid lime">foo</td>

This text does not add anything. It has also the downside of making your tests platform specific due to font differences.

I think you should remove the text and just set up the width / height explicitly. Hoisting the CSS into a common style sheet would also be nice.

> Source/WebCore/ChangeLog:5
> +        Invalidate cache when cell, row, row group, col, col group or table border it changed.

This goes after the bug description / id or it will likely confuse our tools.

> Source/WebCore/ChangeLog:50
> +        * rendering/style/CollapsedBorderValue.h:

It is nice to detail here the different changes you made to the different files so that people can follow what happened.

> Source/WebCore/rendering/RenderTable.cpp:-510
> -        // from lowest precedence to highest precedence.

You split this comment but missed out part of it which would be neat to keep.

> Source/WebCore/rendering/RenderTable.cpp:536
> +        recalcCollapsedBorders();

Your use case is repainting. However it means that querying border on a border-collapsing table is still slow and will still be after your change. Maybe there is something we could do to avoid that too as we are keeping the borders here anyway. If we can also improve querying the border values through JS, then we could also write a performance test to check that we don't regress performance.

I would also be interested in some numbers as to the improvement you are seeing with this change.

> Source/WebCore/rendering/RenderTable.h:263
> +    CollapsedBorderValues m_collapsedBorders;

We are expanding the size of RenderTable for every type of tables regardless of whether "border-collapse" was defined. I would better see a trade-off where we either introduce a rare map (like for Node or Element) or just a pointer placeholder that lazily gets filled when we really need the CollapsedBorderValues. The choice depends on the distribution of border-collapse value in the wild but at least it should be defined why it is fine to one way or another (as Sam pointed out).

> Source/WebCore/rendering/style/CollapsedBorderValue.h:71
> +typedef Vector<CollapsedBorderValue> CollapsedBorderValues;

I don't see why you removed RenderTableCell::CollapsedBorderStyles and replaced it with that. If it is to avoid blowing off RenderTable memory, it is a wrong change (not to mention that adding it here is definitely not the right place).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.