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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Sep 7 12:42:28 PDT 2011


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





--- Comment #16 from Konstantin Shcheglov <scheglov at google.com>  2011-09-07 12:42:28 PST ---
(In reply to comment #15)
> (From update of attachment 106582 [details])
> 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.

  I've looked on LayoutTests/fast/table/border-collapsing/002.html and other tests in same folder.
  Some of them have some description, some - not.
  Actually each test has description, for example:
<!-- Invalidate cache: when we change colgroup border color. -->
  Is it not good enough?
  What if I will improve it to say something like this?
---
Calculating collapsed borders for big tables in expensive, so we cache them and recalculate when needed.
Here we change colgroup border color and test that cache is invalidated and painted table has border with correct color.
---


> 
> > 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.


> 
> > 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.

  Aha!
  Thanks.


> 
> > 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.

  OK


> 
> > 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.

  OK


> 
> > 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.


> 
> > 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.

  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?


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

  Is there any known way to test performance?
  Something like get start thread CPU time, request 10 repaints, calculate time difference and ensure that it is faster than some reasonable time?

  For provided test case performance is visible to the unaided eye, probably tens times faster.


> 
> > 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?


> 
> > 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?

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



More information about the webkit-unassigned mailing list