Bug 100428 - Teach RenderTable how to use Vector::insert and Vector::append instead of its own custom memmove code
Summary: Teach RenderTable how to use Vector::insert and Vector::append instead of its...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 100304
  Show dependency treegraph
 
Reported: 2012-10-25 15:38 PDT by Eric Seidel (no email)
Modified: 2012-11-01 10:43 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.12 KB, patch)
2012-10-25 15:41 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (3.95 KB, patch)
2012-10-25 15:46 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (3.96 KB, patch)
2012-10-25 15:47 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Update per Julien's suggestions (3.92 KB, patch)
2012-10-26 18:43 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch for landing (3.83 KB, patch)
2012-10-30 12:45 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-10-25 15:38:25 PDT
Teach RenderTable how to use Vector::insert and Vector::append instead of its own custom memmove code
Comment 1 Eric Seidel (no email) 2012-10-25 15:41:18 PDT
Created attachment 170748 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-10-25 15:43:26 PDT
This is all part of me working myself up to adding caching of the RenderTableCol pointers in RenderTable to fix bug 100304.
Comment 3 Eric Seidel (no email) 2012-10-25 15:46:18 PDT
Created attachment 170751 [details]
Patch
Comment 4 Eric Seidel (no email) 2012-10-25 15:47:14 PDT
Created attachment 170752 [details]
Patch
Comment 5 Julien Chaffraix 2012-10-26 01:04:35 PDT
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.
Comment 6 Eric Seidel (no email) 2012-10-26 13:58:27 PDT
(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.
Comment 7 Eric Seidel (no email) 2012-10-26 18:43:25 PDT
Created attachment 171067 [details]
Update per Julien's suggestions
Comment 8 Julien Chaffraix 2012-10-30 03:53:35 PDT
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.
Comment 9 Eric Seidel (no email) 2012-10-30 12:45:40 PDT
Created attachment 171505 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-10-30 15:38:17 PDT
Comment on attachment 171505 [details]
Patch for landing

Clearing flags on attachment: 171505

Committed r132949: <http://trac.webkit.org/changeset/132949>
Comment 11 WebKit Review Bot 2012-10-30 15:38:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Darin Adler 2012-11-01 09:53:39 PDT
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.
Comment 13 Eric Seidel (no email) 2012-11-01 10:43:48 PDT
(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.