Refactor StyleVariableData to fallback on root map for performance reasons.

Refactor StyleVariableData to fallback on root map.

A StyleVariableData object is a 'root' when it is constructed
empty rather than as a copy of another StyleVariableData. This
change points all derived StyleVariableData objects back to
their root, and delegates lookups rather than copying variable
values.

This is a performance motivated change. Adding custom properties
to a node used to require that the inherited properties object
from the parent node be copied. This coupled with a standard
practice of creating several hundred custom properties on the
root node resulted in expensive style recalculation (e.g. more
than 50ms on desktop to create 1000 child custom properties).

With this change incorporated, root node custom properties
always form a fallback dictionary that is never copied, so
creating 1000 child custom properties costs approximately
the same as creating none.

BUG=592770
Committed: https://crrev.com/816a8ffc2122e85bd9de4f5e0a30378170c7ca99
Cr-Commit-Position: refs/heads/master@{#386004}

[shans, 2016-04-04 08:13:57]

shans@chromium.org changed reviewers:
+ timloh@chromium.org

[Timothy Loh, 2016-04-05 03:13:37]

The patch title and description need to make it more clear that this is an
optimization and provide some information about how it affects performance in
relevant cases.

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.cpp:32: #include
"core/css/CSSVariableData.h"
is this needed?

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/StyleVariableData.cpp (right):

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleVariableData.cpp:35: return
adoptRef(new StyleVariableData(*this));
No point moving this if it didn't change

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleVariableData.cpp:47: 
extra whitespace here

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleVariableData.cpp:59: 
more extra whitespace here

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

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleVariableData.h:30: // Note that this
method is slow as a new map is sometimes constructed.
Isn't this always making a new map?

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleVariableData.h:31:
PassOwnPtr<HashMap<AtomicString, RefPtr<CSSVariableData>>> getVariables() const
This function is too big to be in the header, move it to the .cpp file

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleVariableData.h:44: StyleVariableData()
this probably didn't need to change, RefPtr's default ctor initializes to
nullptr anyway

[shans, 2016-04-08 00:55:49]

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.cpp:32: #include
"core/css/CSSVariableData.h"
On 2016/04/05 at 03:13:37, Timothy Loh wrote:
> is this needed?

Yeah. OwnPtr defines a deconstructor, which means we can't just forward-declare
CSSVariableData.

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/StyleVariableData.cpp (right):

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleVariableData.cpp:35: return
adoptRef(new StyleVariableData(*this));
On 2016/04/05 at 03:13:37, Timothy Loh wrote:
> No point moving this if it didn't change

Done.

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleVariableData.cpp:47: 
On 2016/04/05 at 03:13:37, Timothy Loh wrote:
> extra whitespace here

Done.

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleVariableData.cpp:59: 
On 2016/04/05 at 03:13:37, Timothy Loh wrote:
> more extra whitespace here

Done.

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

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleVariableData.h:30: // Note that this
method is slow as a new map is sometimes constructed.
On 2016/04/05 at 03:13:37, Timothy Loh wrote:
> Isn't this always making a new map?

Uh .. yeah.

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleVariableData.h:31:
PassOwnPtr<HashMap<AtomicString, RefPtr<CSSVariableData>>> getVariables() const
On 2016/04/05 at 03:13:37, Timothy Loh wrote:
> This function is too big to be in the header, move it to the .cpp file

Done.

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleVariableData.h:44: StyleVariableData()
On 2016/04/05 at 03:13:37, Timothy Loh wrote:
> this probably didn't need to change, RefPtr's default ctor initializes to
nullptr anyway

I changed this because m_root wasn't being initialized (and was causing a bunch
of the layout tests to crash).

[shans, 2016-04-08 00:58:52]

Description was changed from

==========
Refactor StyleVariableData to fallback on root map.

A StyleVariableData object is a 'root' when it is constructed
empty rather than as a copy of another StyleVariableData. This
change points all derived StyleVariableData objects back to
their root, and delegates lookups rather than copying variable
values.

BUG=592770
==========

to

==========
Refactor StyleVariableData to fallback on root map.

A StyleVariableData object is a 'root' when it is constructed
empty rather than as a copy of another StyleVariableData. This
change points all derived StyleVariableData objects back to
their root, and delegates lookups rather than copying variable
values.

This is a performance motivated change. Adding custom properties
to a node used to require that the inherited properties object
from the parent node be copied. This coupled with a standard
practice of creating several hundred custom properties on the
root node resulted in expensive style recalculation (e.g. more
than 50ms on desktop to create 1000 child custom properties).

With this change incorporated, root node custom properties 
always form a fallback dictionary that is never copied, so
creating 1000 child custom properties costs approximately
the same as creating none.

BUG=592770
==========

[Timothy Loh, 2016-04-08 03:42:06]

lgtm

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

https://codereview.chromium.org/1858553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleVariableData.h:44: StyleVariableData()
On 2016/04/08 00:55:49, shans wrote:
> On 2016/04/05 at 03:13:37, Timothy Loh wrote:
> > this probably didn't need to change, RefPtr's default ctor initializes to
> nullptr anyway
> 
> I changed this because m_root wasn't being initialized (and was causing a
bunch
> of the layout tests to crash).

:-|

[shans, 2016-04-08 04:20:48]

The CQ bit was checked by shans@chromium.org

[commit-bot: I haz the power, 2016-04-08 04:20:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1858553002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1858553002/20001

[commit-bot: I haz the power, 2016-04-08 04:22:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-08 04:22:49]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[shans, 2016-04-08 05:07:13]

The CQ bit was checked by shans@chromium.org

[shans, 2016-04-08 05:07:14]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1858553002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-04-08 05:07:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1858553002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1858553002/40001

[commit-bot: I haz the power, 2016-04-08 06:11:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-08 06:12:55]

Description was changed from

==========
Refactor StyleVariableData to fallback on root map.

A StyleVariableData object is a 'root' when it is constructed
empty rather than as a copy of another StyleVariableData. This
change points all derived StyleVariableData objects back to
their root, and delegates lookups rather than copying variable
values.

This is a performance motivated change. Adding custom properties
to a node used to require that the inherited properties object
from the parent node be copied. This coupled with a standard
practice of creating several hundred custom properties on the
root node resulted in expensive style recalculation (e.g. more
than 50ms on desktop to create 1000 child custom properties).

With this change incorporated, root node custom properties 
always form a fallback dictionary that is never copied, so
creating 1000 child custom properties costs approximately
the same as creating none.

BUG=592770
==========

to

==========
Refactor StyleVariableData to fallback on root map.

A StyleVariableData object is a 'root' when it is constructed
empty rather than as a copy of another StyleVariableData. This
change points all derived StyleVariableData objects back to
their root, and delegates lookups rather than copying variable
values.

This is a performance motivated change. Adding custom properties
to a node used to require that the inherited properties object
from the parent node be copied. This coupled with a standard
practice of creating several hundred custom properties on the
root node resulted in expensive style recalculation (e.g. more
than 50ms on desktop to create 1000 child custom properties).

With this change incorporated, root node custom properties
always form a fallback dictionary that is never copied, so
creating 1000 child custom properties costs approximately
the same as creating none.

BUG=592770

Committed: https://crrev.com/816a8ffc2122e85bd9de4f5e0a30378170c7ca99
Cr-Commit-Position: refs/heads/master@{#386004}
==========

[commit-bot: I haz the power, 2016-04-08 06:12:59]

Patchset 3 (id:??) landed as
https://crrev.com/816a8ffc2122e85bd9de4f5e0a30378170c7ca99
Cr-Commit-Position: refs/heads/master@{#386004}