Bug 100630 – Make rendering tables with <colgroups> twice as fast by avoiding walking the DOM for colgroups 4 times for each cell

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Make rendering tables with <colgroups> twice as fast by avoiding walking the DOM for colgroups 4 times for each cell

[Eric Seidel (no email)]

Created attachment 171160 [details]
Patch

[Eric Seidel (no email)]

This is sadly not an algorithmic fix (it's lame that we even need to lookup these <colgroup> elements so often), it's just making these dumb lookups faster.

Looking at the profile after this fix, we're still spending 21% of our time in slowColElement, we're just now walking this Vector instead of the DOM.

[Ojan Vafai]

Comment on attachment 171160 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171160&action=review

Algorithmically, I wonder if you could save time by creating a colElementIterator class. Looks like at least some of the callers of colElement just iterate through the columns in order. Would be nice to not have to always start from the beginning of the list. Either way, this patch is fine.

> Source/WebCore/rendering/RenderTable.cpp:792
> +RenderTableCol* RenderTable::slowColElement(unsigned col, bool* startEdge, bool* endEdge) const

Not from your patch, but it seems silly to have a method called slowColElement when the only caller is in colElement which just does an early return and then calls this. May as well move the early return into this function and have this be colElement. I guess maybe colElement gets inlined?

[Eric Seidel (no email)]

(In reply to comment #3)
> (From update of attachment 171160 [details])
> Not from your patch, but it seems silly to have a method called slowColElement when the only caller is in colElement which just does an early return and then calls this. May as well move the early return into this function and have this be colElement. I guess maybe colElement gets inlined?

Correct.  I split slowColElement off of colElement in an earlier patch to allow the no-columns (common) case to be inline and fast.  It was a big win on dglazkov's biggrid iirc.

[WebKit Review Bot]

Comment on attachment 171160 [details]
Patch

Clearing flags on attachment: 171160

Committed r132764: <http://trac.webkit.org/changeset/132764>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

I do think some sort of state to prevent having to re-walk the columns every time would be useful.  It's not yet clear to me what the access pattern of this data is yet.

[Eric Seidel (no email)]

This fix may not be necessary in the end.  If we can make collapsed borders not so expensive we won't need this complexity.

[Eric Seidel (no email)]

Reopening to attach new patch.

[Eric Seidel (no email)]

Created attachment 171503 [details]
Patch for landing

[Eric Seidel (no email)]

Sorry, got confused.