Bug 70570 - Remove RenderTableSection::m_gridRows
Summary: Remove RenderTableSection::m_gridRows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-20 18:46 PDT by Julien Chaffraix
Modified: 2011-10-27 13:05 PDT (History)
2 users (show)

See Also:


Attachments
Proposed removal. (11.79 KB, patch)
2011-10-20 19:28 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (11.72 KB, patch)
2011-10-27 11:11 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2011-10-20 18:46:58 PDT
This field is:
1) confusing
2) an int when it should be an unsigned
3) already contained in m_grid.size()
Comment 1 Julien Chaffraix 2011-10-20 19:28:31 PDT
Created attachment 111889 [details]
Proposed removal.
Comment 2 Darin Adler 2011-10-27 10:20:25 PDT
Comment on attachment 111889 [details]
Proposed removal.

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

> Source/WebCore/rendering/RenderTableSection.cpp:180
> +    if (numRows > (int)m_grid.size()) {

This should be a C++ static_cast, not a C cast.

> Source/WebCore/rendering/RenderTableSection.cpp:183
> +        size_t maxSize = numeric_limits<size_t>::max() / sizeof(RowStruct);
> +        if (static_cast<size_t>(numRows) > maxSize)
> +            return false;

If we plan to return false when a value would overflow what a Vector can hold, the computation about the limits of what fits in a Vector should not be done here, because it involves assumptions about how Vector is implemented. Vector might have a smaller limit than the one here. Instead, we should add a tryGrow function to Vector.

On the other hand, maybe we want a much smaller limit, because I am concerned this lets us create enormous data structures that fit in virtual memory, but put the engine into a very bad state.

> Source/WebCore/rendering/RenderTableSection.cpp:1132
> +    // Although it is possible for our row count to shrink (due to removeChild being called),
> +    // it is more common for the count to stay the same. Let's just reallocate the old
> +    // capacity upfront to avoid re-expanding it one row at a time.
> +    m_grid.reserveCapacity(capacity);

It seems unfortunate to throw away and reallocate here. We should figure out a way to get clearGrid to lower the size and leave the already allocated memory alone, rather than re-reserving capacity. A good way to do that would be to factor clearGrid out into two pieces so you can use the deletion part and then do some other way of emptying the vector that does not throw away the excess capacity, rather than clear().
Comment 3 Julien Chaffraix 2011-10-27 10:58:56 PDT
Comment on attachment 111889 [details]
Proposed removal.

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

>> Source/WebCore/rendering/RenderTableSection.cpp:180
>> +    if (numRows > (int)m_grid.size()) {
> 
> This should be a C++ static_cast, not a C cast.

Will be changed.

>> Source/WebCore/rendering/RenderTableSection.cpp:183
>> +            return false;
> 
> If we plan to return false when a value would overflow what a Vector can hold, the computation about the limits of what fits in a Vector should not be done here, because it involves assumptions about how Vector is implemented. Vector might have a smaller limit than the one here. Instead, we should add a tryGrow function to Vector.
> 
> On the other hand, maybe we want a much smaller limit, because I am concerned this lets us create enormous data structures that fit in virtual memory, but put the engine into a very bad state.

Indeed, this is part of the old code that I kept intact as I did not want to change the behavior.

I would tend to agree with your second line of thoughts. Overflowing a Vector means the table is too huge to be handled in a good manner by the engine anyway. Limiting the column and row counts could help shrink some of data structures for the common cases.

>> Source/WebCore/rendering/RenderTableSection.cpp:1132
>> +    m_grid.reserveCapacity(capacity);
> 
> It seems unfortunate to throw away and reallocate here. We should figure out a way to get clearGrid to lower the size and leave the already allocated memory alone, rather than re-reserving capacity. A good way to do that would be to factor clearGrid out into two pieces so you can use the deletion part and then do some other way of emptying the vector that does not throw away the excess capacity, rather than clear().

I do agree.

To keep the memory, we would actually need a new method on Vector as the size cannot be changed while keeping the buffer around (currently clear also free the buffer). I will file a follow-up bug about that and fix it.
Comment 4 Julien Chaffraix 2011-10-27 11:11:02 PDT
Created attachment 112711 [details]
Patch for landing
Comment 5 WebKit Review Bot 2011-10-27 12:12:57 PDT
Comment on attachment 112711 [details]
Patch for landing

Clearing flags on attachment: 112711

Committed r98614: <http://trac.webkit.org/changeset/98614>
Comment 6 WebKit Review Bot 2011-10-27 12:13:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Julien Chaffraix 2011-10-27 13:05:29 PDT
Follow up bug for deallocating / reallocating the vector: bug 71056.