Teach RenderTable how to use Vector::insert and Vector::append instead of its own custom memmove code
Created attachment 170748 [details] Patch
This is all part of me working myself up to adding caching of the RenderTableCol pointers in RenderTable to fix bug 100304.
Created attachment 170751 [details] Patch
Created attachment 170752 [details] Patch
Comment on attachment 170752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170752&action=review The direction is fine, not r+'ing due to the lost underflow ASSERT. > Source/WebCore/rendering/RenderTable.cpp:-702 > - ASSERT(oldSpan > firstSpan); I am not happy about losing this ASSERT. You are doing unsigned arithmetic and this would catch underflow in a way the new logic doesn't. Your new ASSERT was covered by the old one but is only a subset of what we were ASSERTing. > Source/WebCore/rendering/RenderTable.cpp:701 > + ASSERT(m_columns[position + 1].span > 0); // Every column must span at least 1! The comment doesn't add much here. > Source/WebCore/rendering/RenderTable.h:132 > + explicit ColumnStruct(unsigned initialSpan = 1) I don't see the interest of the default initialSpan as all call sites pass something.
(In reply to comment #5) > (From update of attachment 170752 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170752&action=review > > Source/WebCore/rendering/RenderTable.h:132 > > + explicit ColumnStruct(unsigned initialSpan = 1) > > I don't see the interest of the default initialSpan as all call sites pass something. Vector::resize() needs an argument-less constructor.
Created attachment 171067 [details] Update per Julien's suggestions
Comment on attachment 171067 [details] Update per Julien's suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=171067&action=review > Source/WebCore/rendering/RenderTable.cpp:722 > + // Save off the index of the new column for use in notifying the sections of this new column. I don't think this comment adds much. > Source/WebCore/rendering/RenderTable.h:132 > + explicit ColumnStruct(unsigned initialSpan = 1) Too bad for the initial value but that's another battle. Thanks for explaining why it is needed.
Created attachment 171505 [details] Patch for landing
Comment on attachment 171505 [details] Patch for landing Clearing flags on attachment: 171505 Committed r132949: <http://trac.webkit.org/changeset/132949>
All reviewed patches have been landed. Closing bug.
Comment on attachment 171505 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=171505&action=review > Source/WebCore/ChangeLog:13 > + insert, append and grow all use the same expandCapacity logic under the covers > + and Vector::insert uses TypeOperations::moveOverlapping which should use memmove > + by default for unknown types. I think this comment is not quite right. For truly arbitrary “unknown” types, Vector can’t use memmove. Vector can only use memmove as an optimization if it knows that is safe. I think it’s possible, maybe even likely, that it can figure this out for ColumnStruct without the RenderTable code having to do something explicitly because of the type traits machinery it uses, but I think the wording we are using here in the comment is misleading and doesn’t mention what it takes to make sure that Vector uses memmove.
(In reply to comment #12) > (From update of attachment 171505 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171505&action=review > > > Source/WebCore/ChangeLog:13 > > + insert, append and grow all use the same expandCapacity logic under the covers > > + and Vector::insert uses TypeOperations::moveOverlapping which should use memmove > > + by default for unknown types. > > I think this comment is not quite right. For truly arbitrary “unknown” types, Vector can’t use memmove. Vector can only use memmove as an optimization if it knows that is safe. I think it’s possible, maybe even likely, that it can figure this out for ColumnStruct without the RenderTable code having to do something explicitly because of the type traits machinery it uses, but I think the wording we are using here in the comment is misleading and doesn’t mention what it takes to make sure that Vector uses memmove. It's possible that I've misunderstood and regressed things. I have not seen this code show up in benchmarks, but I'll keep my eye out for it.