Bug 70570 – Remove RenderTableSection::m_gridRows

• Component: Tables
• Status: RESOLVED FIXED

[Julien Chaffraix]

This field is:
1) confusing
2) an int when it should be an unsigned
3) already contained in m_grid.size()

[Julien Chaffraix]

Created attachment 111889 [details]
Proposed removal.

[Darin Adler]

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

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 112711 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 112711 [details]
Patch for landing

Clearing flags on attachment: 112711

Committed r98614: <http://trac.webkit.org/changeset/98614>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Chaffraix]

Follow up bug for deallocating / reallocating the vector: bug 71056.