Bug 71056 - RenderTableSection::recalcCells should not free its grid
Summary: RenderTableSection::recalcCells should not free its grid
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-27 13:04 PDT by Julien Chaffraix
Modified: 2011-10-28 11:04 PDT (History)
2 users (show)

See Also:


Attachments
Avoid free'ing the m_grid vector, refactored |row| to be part of RowStruct and made the 'reset' logic more apparent. (10.32 KB, patch)
2011-10-27 19:12 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for EWS analysis. (10.42 KB, patch)
2011-10-28 09:43 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (10.41 KB, patch)
2011-10-28 10:40 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-27 13:04:50 PDT
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.
Comment 1 Julien Chaffraix 2011-10-27 19:12:20 PDT
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.
Comment 2 Darin Adler 2011-10-27 19:47:37 PDT
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.
Comment 3 Julien Chaffraix 2011-10-28 09:35:33 PDT
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.
Comment 4 Julien Chaffraix 2011-10-28 09:43:26 PDT
Created attachment 112879 [details]
Patch for EWS analysis.
Comment 5 Julien Chaffraix 2011-10-28 10:23:36 PDT
Comment on attachment 112879 [details]
Patch for EWS analysis.

Let's see if the cq will pick this up.
Comment 6 WebKit Review Bot 2011-10-28 10:28:55 PDT
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
Comment 7 Julien Chaffraix 2011-10-28 10:40:10 PDT
Created attachment 112881 [details]
Patch for landing
Comment 8 WebKit Review Bot 2011-10-28 11:04:29 PDT
Comment on attachment 112881 [details]
Patch for landing

Clearing flags on attachment: 112881

Committed r98738: <http://trac.webkit.org/changeset/98738>
Comment 9 WebKit Review Bot 2011-10-28 11:04:34 PDT
All reviewed patches have been landed.  Closing bug.