[css-grid] Fix performance regression in grid layout

[css-grid] Fix performance regression in grid layout

Some of the changes added in crrev.com/415676 made layout of grid containers
a bit slower. That was because it modified some methods in GridTrackSize.h
which is a hot spot. That class is basically a cache of some values and it's
critical to keep it really fast. The aforementioned CL was adding an if
clause to a getter method, and that was causing the slowdown.

BUG=643359
Committed: https://crrev.com/8b54987d15a92a54796a357daf6e021f0ff9a2b6
Cr-Commit-Position: refs/heads/master@{#417001}

[svillar, 2016-09-07 09:30:28]

Description was changed from

==========
[css-grid] Fix performance regression in grid layout

Some of the changes added in crrev.com/415676 made layout of grid containers
a bit slower. That was because it modified some methods in GridTrackSize.h
which is a hot spot. That class is basically a cache of some values and it's
critical to keep it really fast. The aforementioned CL was adding an if
clause to a getter method, and that was causing the slowdown.

BUG=643359
==========

to

==========
[css-grid] Fix performance regression in grid layout

Some of the changes added in crrev.com/415676 made layout of grid containers
a bit slower. That was because it modified some methods in GridTrackSize.h
which is a hot spot. That class is basically a cache of some values and it's
critical to keep it really fast. The aforementioned CL was adding an if
clause to a getter method, and that was causing the slowdown.

BUG=643359
==========

[svillar, 2016-09-07 09:30:29]

svillar@igalia.com changed reviewers:
+ cbiesinger@chromium.org, eae@chromium.org, jfernandez@igalia.com,
rego@igalia.com

[jfernandez, 2016-09-07 10:50:51]

lgtm

[Manuel Rego, 2016-09-07 10:56:47]

lgtm

https://codereview.chromium.org/2318993002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2318993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1147:
DCHECK(trackSize.fitContentTrackBreadth().isLength());
Probably this DCHECK() is not really useful anymore.

https://codereview.chromium.org/2318993002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/GridTrackSize.h (right):

https://codereview.chromium.org/2318993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridTrackSize.h:50: // minmax(auto,
max-content) althought we store the argument for later use in m_maxTrackBreadth.
This comment needs to get updated, we're not storing it in
m_maxTrackBreadth anymore.

https://codereview.chromium.org/2318993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridTrackSize.h:58: ,
m_fitContentTrackBreadth(trackSizeType == FitContentTrackSizing ? length :
GridLength(Length(Fixed)))
Probably using Length(Auto) or Length(Fixed) is enough,
like we do for m_min|maxTrackBreadth.

https://codereview.chromium.org/2318993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridTrackSize.h:81: const GridLength&
maxTrackBreadth() const { return m_maxTrackBreadth; }
Probably you can add here a similar DCHECK() to the one in
fitContentTrackBreadth().

[cbiesinger, 2016-09-07 16:00:36]

lgtm

[svillar, 2016-09-07 16:27:52]

https://codereview.chromium.org/2318993002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2318993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1147:
DCHECK(trackSize.fitContentTrackBreadth().isLength());
On 2016/09/07 10:56:47, Manuel Rego wrote:
> Probably this DCHECK() is not really useful anymore.

Right.

https://codereview.chromium.org/2318993002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/GridTrackSize.h (right):

https://codereview.chromium.org/2318993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridTrackSize.h:50: // minmax(auto,
max-content) althought we store the argument for later use in m_maxTrackBreadth.
On 2016/09/07 10:56:47, Manuel Rego wrote:
> This comment needs to get updated, we're not storing it in
> m_maxTrackBreadth anymore.

Acknowledged.

https://codereview.chromium.org/2318993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridTrackSize.h:50: // minmax(auto,
max-content) althought we store the argument for later use in m_maxTrackBreadth.
On 2016/09/07 10:56:47, Manuel Rego wrote:
> This comment needs to get updated, we're not storing it in
> m_maxTrackBreadth anymore.

Acknowledged.

https://codereview.chromium.org/2318993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridTrackSize.h:58: ,
m_fitContentTrackBreadth(trackSizeType == FitContentTrackSizing ? length :
GridLength(Length(Fixed)))
On 2016/09/07 10:56:47, Manuel Rego wrote:
> Probably using Length(Auto) or Length(Fixed) is enough,
> like we do for m_min|maxTrackBreadth.

Yes, but there is a recent trend to remove the usage of implicit constructors,
that's why I chose that syntax.

https://codereview.chromium.org/2318993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridTrackSize.h:81: const GridLength&
maxTrackBreadth() const { return m_maxTrackBreadth; }
On 2016/09/07 10:56:47, Manuel Rego wrote:
> Probably you can add here a similar DCHECK() to the one in
> fitContentTrackBreadth().

To check what? All the different types have a min and a max so there is no much
to check here.

[svillar, 2016-09-07 16:28:55]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-09-07 16:28:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org, jfernandez@igalia.com, rego@igalia.com
Link to the patchset: https://codereview.chromium.org/2318993002/#ps20001
(title: "Patch for landing")

[commit-bot: I haz the power, 2016-09-07 16:29:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 18:25:57]

Description was changed from

==========
[css-grid] Fix performance regression in grid layout

Some of the changes added in crrev.com/415676 made layout of grid containers
a bit slower. That was because it modified some methods in GridTrackSize.h
which is a hot spot. That class is basically a cache of some values and it's
critical to keep it really fast. The aforementioned CL was adding an if
clause to a getter method, and that was causing the slowdown.

BUG=643359
==========

to

==========
[css-grid] Fix performance regression in grid layout

Some of the changes added in crrev.com/415676 made layout of grid containers
a bit slower. That was because it modified some methods in GridTrackSize.h
which is a hot spot. That class is basically a cache of some values and it's
critical to keep it really fast. The aforementioned CL was adding an if
clause to a getter method, and that was causing the slowdown.

BUG=643359
==========

[commit-bot: I haz the power, 2016-09-07 18:25:58]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-07 18:29:47]

Description was changed from

==========
[css-grid] Fix performance regression in grid layout

Some of the changes added in crrev.com/415676 made layout of grid containers
a bit slower. That was because it modified some methods in GridTrackSize.h
which is a hot spot. That class is basically a cache of some values and it's
critical to keep it really fast. The aforementioned CL was adding an if
clause to a getter method, and that was causing the slowdown.

BUG=643359
==========

to

==========
[css-grid] Fix performance regression in grid layout

Some of the changes added in crrev.com/415676 made layout of grid containers
a bit slower. That was because it modified some methods in GridTrackSize.h
which is a hot spot. That class is basically a cache of some values and it's
critical to keep it really fast. The aforementioned CL was adding an if
clause to a getter method, and that was causing the slowdown.

BUG=643359

Committed: https://crrev.com/8b54987d15a92a54796a357daf6e021f0ff9a2b6
Cr-Commit-Position: refs/heads/master@{#417001}
==========

[commit-bot: I haz the power, 2016-09-07 18:29:48]

Patchset 2 (id:??) landed as
https://crrev.com/8b54987d15a92a54796a357daf6e021f0ff9a2b6
Cr-Commit-Position: refs/heads/master@{#417001}

[Manuel Rego, 2016-09-07 20:11:47]

https://codereview.chromium.org/2318993002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/GridTrackSize.h (right):

https://codereview.chromium.org/2318993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridTrackSize.h:81: const GridLength&
maxTrackBreadth() const { return m_maxTrackBreadth; }
On 2016/09/07 16:27:52, svillar wrote:
> On 2016/09/07 10:56:47, Manuel Rego wrote:
> > Probably you can add here a similar DCHECK() to the one in
> > fitContentTrackBreadth().
> 
> To check what? All the different types have a min and a max so there is no
much
> to check here.

Yes, right didn't realize about that.

I was thinking in doing "DCHECK(m_type != FitContentTrackSizing);"
but I didn't remember that these values were used anyway.

[eae, 2016-09-08 07:50:44]

LGTM