Bug 71056 – RenderTableSection::recalcCells should not free its grid

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Julien Chaffraix]

Following bug 70570, recalcCells is now free'ing and reallocating the row vector (m_grid) when it used just to reuse it.

It is wasteful so let's try to get the previous behavior back.

[Julien Chaffraix]

Created attachment 112806 [details]
Avoid free'ing the m_grid vector, refactored |row| to be part of RowStruct and made the 'reset' logic more apparent.

[Darin Adler]

Comment on attachment 112806 [details]
Avoid free'ing the m_grid vector, refactored |row| to be part of RowStruct and made the 'reset' logic more apparent.

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

Patch does not apply. I am going to say review+ but not sure about that aspect of things.

> Source/WebCore/rendering/RenderTableSection.cpp:77
>  RenderTableSection::~RenderTableSection()
>  {
> -    clearGrid();
> +    m_grid.clear();
>  }

This is not needed in a destructor. The Vector will be destroyed automatically, so no need to clear it. You can just remove the call to clearGrid entirely and not replace it with anything.

> Source/WebCore/rendering/RenderTableSection.cpp:1159
> +        m_grid[row].row.clear();
> +        m_grid[row].row.resize(effectiveColumnCount);

If we do need to clear and then re-allocate, we can at least factor out one unnecessary branch by using grow instead of resize.

[Julien Chaffraix]

Comment on attachment 112806 [details]
Avoid free'ing the m_grid vector, refactored |row| to be part of RowStruct and made the 'reset' logic more apparent.

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

> Patch does not apply. I am going to say review+ but not sure about that aspect of things.

Good catch, that's my fault as I did not rebaseline after yesterday landings. I will submit a new patch for EWS analysis to make sure I don't break anything.

>> Source/WebCore/rendering/RenderTableSection.cpp:77
>>  }
> 
> This is not needed in a destructor. The Vector will be destroyed automatically, so no need to clear it. You can just remove the call to clearGrid entirely and not replace it with anything.

Will be removed.

>> Source/WebCore/rendering/RenderTableSection.cpp:1159
>> +        m_grid[row].row.resize(effectiveColumnCount);
> 
> If we do need to clear and then re-allocate, we can at least factor out one unnecessary branch by using grow instead of resize.

We don't strictly need the whole re-allocation to per the comment above. We could get away with individually resetting each CellStruct. I am not sure the added complexity is worth the effort for now.

[Julien Chaffraix]

Created attachment 112879 [details]
Patch for EWS analysis.

[Julien Chaffraix]

Comment on attachment 112879 [details]
Patch for EWS analysis.

Let's see if the cq will pick this up.

[WebKit Review Bot]

Comment on attachment 112879 [details]
Patch for EWS analysis.

Rejecting attachment 112879 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

NOBODY (OOPS!) found in /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/10240090

[Julien Chaffraix]

Created attachment 112881 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 112881 [details]
Patch for landing

Clearing flags on attachment: 112881

Committed r98738: <http://trac.webkit.org/changeset/98738>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.