Bug 100428 – Teach RenderTable how to use Vector::insert and Vector::append instead of its own custom memmove code

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Teach RenderTable how to use Vector::insert and Vector::append instead of its own custom memmove code

[Eric Seidel (no email)]

Created attachment 170748 [details]
Patch

[Eric Seidel (no email)]

This is all part of me working myself up to adding caching of the RenderTableCol pointers in RenderTable to fix bug 100304.

[Eric Seidel (no email)]

Created attachment 170751 [details]
Patch

[Eric Seidel (no email)]

Created attachment 170752 [details]
Patch

[Julien Chaffraix]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Created attachment 171067 [details]
Update per Julien's suggestions

[Julien Chaffraix]

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.

[Eric Seidel (no email)]

Created attachment 171505 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 171505 [details]
Patch for landing

Clearing flags on attachment: 171505

Committed r132949: <http://trac.webkit.org/changeset/132949>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Eric Seidel (no email)]

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