[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 #18 from Julien Chaffraix <jchaffraix at webkit.org>  2011-09-07 14:00:28 PST ---
(From update of attachment 106582)
View in context: https://bugs.webkit.org/attachment.cgi?id=106582&action=review

>>> 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.
> 
> OK
> 
>   I've used to add many comments, sometimes just to split code into logical sections.
>   But if having such sections is against of WebKit code style, I can live without them.
> 
>   Returning from Java to C++ I see with sorrow that C++ code has almost no comments and almost everything should be deduced from name, usage, etc.

Note that WebKit style has nothing against comments.

We value important comments. However we usually prefer comments about the 'why' rather than the 'what'. The latter should be clear from the code (and if it is not, then a comment is totally welcome but it is a bad sign).

>>> 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.
> 
> Sorry, I don't understand which part to keep.
>   "from lowest precedence to highest precedence" is included.

I was hinting at the middle "Once we have all the style sorted, we then do individual passes." Even if you kept the gist of the comment, I found that this part helped understand the articulation between the 2 parts you split.
How about for the following comment:

// Using our cached sorted styles, we then do individual passes, painting each style of border from lowest precedence to highest precedence.

>>> 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.
> 
> Hm...
>   I don't see other invocations of RenderTableCell::collectBorderValues() than from RenderTable.
>   RenderTableCell::collapsedStartBorder() is also called only from RenderTableCell during paint.
> 
>   Well, probably I just don't know some trick how it is accessed from JS.
>   Can you give me hint?
>   Is there way to get collapsed border values in JS?

Sure, here is how you can access the border value through JS:

var cell = document.getElementById('thisCell');
getComputedStyle(cell, '').borderLeftWidth

Maybe more importantly, all the virtual border* method on RenderTableCell will still trigger a recomputation and completely miss the cache. The invocations of those function are spread in the rendering code but they are nevertheless important.

>>> 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).
> 
> If table does not use collapsed borders, then we use memory only for empty Vector; I don't know exact memory footprint (how to check this?), but in theory this can be as small as two ints (capacity and size) plus pointer. Is it worth optimizing?

Ah, sorry about that, I thought the Vector's did have a default size of 16, not 0. Just discard what I said.

>>> 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).
> 
> To avoid bigger memory consumption I did change suggested by Sam - removed size 100 for Vector.
> 
>   Actual reason for moving CollapsedBorderStyles from RenderTableCell is that I can not use it in RenderTable.h, because I can not forward declare it.
>   Well, at least I don't know how to do this. See also
>   http://stackoverflow.com/questions/2600385/c-nested-class-forward-declaration-issue
> 
>   Should I move CollapsedBorderValues into RenderTable.h instead?

About CollapsedBorderStyles, you can move it to RenderTable.h. It looks like part of your problem arise from the fact that RenderTableCell / RenderTableSection are doing unneeded #include and could use some forward-declarations. You could redefine the symbol if moving is proving problematic but I think you would need to solve the #include issue then.

I feared setting the size to 0 in CollapsedBorderValues would actually hurt other use cases: when you *do* need to collect borders, you can easily need 100 entries in your Vector. 100 entries is only 25 cells (4 borders per cell). You could mitigate that by pre-allocating the Vector before collecting your borders as you can estimate the size needed.

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