[RFC] Move StyleTransformData to ComputedStyle top-level

[RFC] Move StyleTransformData to ComputedStyle top-level

This moves the StyleTransformData structure to the top-level of
ComputedStyle (i.e directly referenced from said class.) This is
primarily to avoid the high cost of copying the outer
StyleRareNonInheritedData.

Also move state for:

 * The properties: 'transform-style', 'backface-visibility',
   'perspective' and 'perspective-origin' to StyleTransformData
   as well since they have a strong transform affinity.

 * The derived flag m_hasInlineTransform, since it's directly
   dependent on the presence of a non-'none' 'transform'.

 * m_isStackingContext (up) to ComputedStyle to avoid it invoking
   CoW of a StyleRareNonInheritedData object when one isn't needed.

BUG=670375, 369942

[fs, 2016-11-04 14:05:42]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 14:05:54]

Dry run: 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-11-04 14:28:56]

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

[commit-bot: I haz the power, 2016-11-04 14:28:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[fs, 2016-11-04 14:45:52]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 14:46:10]

Dry run: 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-11-04 16:06:34]

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

[commit-bot: I haz the power, 2016-11-04 16:06:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fs, 2016-11-07 15:12:50]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 15:13:11]

Dry run: 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-11-07 17:38:02]

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

[commit-bot: I haz the power, 2016-11-07 17:38:03]

Dry run: This issue passed the CQ dry run.

[fs, 2016-11-28 15:27:45]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 15:28:05]

Dry run: 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-11-28 17:11:46]

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

[commit-bot: I haz the power, 2016-11-28 17:11:47]

Dry run: This issue passed the CQ dry run.

[fs, 2016-12-02 09:31:00]

Description was changed from

==========
[RFC] Move StyleTransformData to ComputedStyle top-level

Move property state for:

 'transform-style',
 'backface-visibility',
 'perspective' and
 'perspective-origin'

to StyleTransformData as well since they have a strong transform
affinity.

Also move the derived flag m_hasInlineTransform, since it's directly
dependent on the presence of a non-'none' 'transform'.

Also move m_isStackingContext (up) to ComputedStyle to avoid it invoking
CoW of a StyleRareNonInheritedData object when one isn't needed.
==========

to

==========
[RFC] Move StyleTransformData to ComputedStyle top-level

Move property state for:

 'transform-style',
 'backface-visibility',
 'perspective' and
 'perspective-origin'

to StyleTransformData as well since they have a strong transform
affinity.

Also move the derived flag m_hasInlineTransform, since it's directly
dependent on the presence of a non-'none' 'transform'.

Also move m_isStackingContext (up) to ComputedStyle to avoid it invoking
CoW of a StyleRareNonInheritedData object when one isn't needed.

BUG=670375,369942
==========

[fs, 2016-12-02 09:35:22]

Description was changed from

==========
[RFC] Move StyleTransformData to ComputedStyle top-level

Move property state for:

 'transform-style',
 'backface-visibility',
 'perspective' and
 'perspective-origin'

to StyleTransformData as well since they have a strong transform
affinity.

Also move the derived flag m_hasInlineTransform, since it's directly
dependent on the presence of a non-'none' 'transform'.

Also move m_isStackingContext (up) to ComputedStyle to avoid it invoking
CoW of a StyleRareNonInheritedData object when one isn't needed.

BUG=670375,369942
==========

to

==========
[RFC] Move StyleTransformData to ComputedStyle top-level

This moves the StyleTransformData structure to the top-level of
ComputedStyle (i.e directly referenced from said class.) This is
primarily to avoid the high cost of copying the outer
StyleRareNonInheritedData.

Also move property state for:

 'transform-style',
 'backface-visibility',
 'perspective' and
 'perspective-origin'

to StyleTransformData as well since they have a strong transform
affinity.

Move the derived flag m_hasInlineTransform, since it's directly
dependent on the presence of a non-'none' 'transform'.

Move m_isStackingContext (up) to ComputedStyle to avoid it invoking
CoW of a StyleRareNonInheritedData object when one isn't needed.

BUG=670375,369942
==========

[fs, 2016-12-02 09:38:29]

Description was changed from

==========
[RFC] Move StyleTransformData to ComputedStyle top-level

This moves the StyleTransformData structure to the top-level of
ComputedStyle (i.e directly referenced from said class.) This is
primarily to avoid the high cost of copying the outer
StyleRareNonInheritedData.

Also move property state for:

 'transform-style',
 'backface-visibility',
 'perspective' and
 'perspective-origin'

to StyleTransformData as well since they have a strong transform
affinity.

Move the derived flag m_hasInlineTransform, since it's directly
dependent on the presence of a non-'none' 'transform'.

Move m_isStackingContext (up) to ComputedStyle to avoid it invoking
CoW of a StyleRareNonInheritedData object when one isn't needed.

BUG=670375,369942
==========

to

==========
[RFC] Move StyleTransformData to ComputedStyle top-level

This moves the StyleTransformData structure to the top-level of
ComputedStyle (i.e directly referenced from said class.) This is
primarily to avoid the high cost of copying the outer
StyleRareNonInheritedData.

Also move state for:

 * The properties: 'transform-style', 'backface-visibility',
   'perspective' and 'perspective-origin' to StyleTransformData
   as well since they have a strong transform affinity.

 * The derived flag m_hasInlineTransform, since it's directly
   dependent on the presence of a non-'none' 'transform'.

 * m_isStackingContext (up) to ComputedStyle to avoid it invoking
   CoW of a StyleRareNonInheritedData object when one isn't needed.

BUG=670375,369942
==========

[fs, 2016-12-02 13:23:07]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 13:23:18]

Dry run: 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-12-02 14:18:35]

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

[commit-bot: I haz the power, 2016-12-02 14:18:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fs, 2016-12-02 14:59:57]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 15:00:13]

Dry run: 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-12-02 17:22:10]

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

[commit-bot: I haz the power, 2016-12-02 17:22:12]

Dry run: This issue passed the CQ dry run.

[fs, 2016-12-02 21:16:00]

fs@opera.com changed reviewers:
+ esprehn@chromium.org, pdr@chromium.org, sashab@chromium.org

[fs, 2016-12-02 21:16:01]

So the trade-off here should be fairly obvious - one pointer size-increase vs.
less to copy. See [1] for some (granted, not entirely up-to-date) perf numbers.
The test itself could be considered somewhat extreme (almost all elements have
transforms), so that's another factor to consider I guess. PTAL and let me know
what you think.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=369942#c20

[esprehn, 2016-12-02 21:26:18]

So many other things still get you a StyleRareNonInheritedData (ex. will-change,
opacity, ...) that I do wonder how much that change matters. In general sashab@
has been working on generating computed style, then we could experiment with a
number of more clever storage formats. This patch means more memory for everyone
to improve transforms in some very specialized cases (will-change or opacity
defeating this is pretty unfortunate). I think once we finish generating all of
computed style we can get even bigger wins.

It was hard to parse that benchmark output, how much better is this actually on
those SVG benchmarks?

Overall I defer to sashab@ on this though.

[fs, 2016-12-02 21:37:08]

On 2016/12/02 at 21:26:18, esprehn wrote:
> So many other things still get you a StyleRareNonInheritedData (ex.
will-change, opacity, ...) that I do wonder how much that change matters. In
general sashab@ has been working on generating computed style, then we could
experiment with a number of more clever storage formats. This patch means more
memory for everyone to improve transforms in some very specialized cases
(will-change or opacity defeating this is pretty unfortunate). I think once we
finish generating all of computed style we can get even bigger wins.

Yes, this is a pretty "narrow" (focused, if you will) change. What I'm really
looking for I guess is if this is worth caring about (at this point in time), or
if we should eat the regression(s) (which is arguably not the most common thing,
although I suspect some generators generate this type of code.)

> It was hard to parse that benchmark output, how much better is this actually
on those SVG benchmarks?

Looking at the worst offender (Bamboo), it's from 141 to 117, so ~15%
improvement (or, eliminating the regression which is really my main concern
here.)

[sashab, 2016-12-04 23:01:27]

+some people that might be interested in this patch

Although I appreciate what you're trying to do, I have to agree with esprehn
here. The large-ish memory cost is not worth the small speedup we'll get for a
small number of use cases, and you haven't really given enough data to even
understand what that memory impact is.

I could be convinced if we had more data that showed the improvement, such as:
- benchmarks on blink_style.top_25 sites showing update_style speed improvements
- data on how many more/less ComputedStyle/StyleTransformData objects are
created from this on 4-5 average webpages (facebook, wikipedia, amazon, twitter,
pinterest etc)
- data on how this affects style sharing on average webpages

Myself and a few others are working on a project to generate ComputedStyle which
will result in shuffling these DataRefs based on stats collected from real pages
anyway, so this work is not needed.

If you'd like to do some valuable work on ComputedStyle, we could use more data
on how often these pointers are allocated on an average webpage, what properties
change most often on an average webpage or with reset.css frameworks like
Bootstrap. We are also looking to experiment with a dynamic allocation algorithm
that can group properties for a webpage based on which styles the page changes
most often and how many elements share particular properties.

So overall, definitely work in the right direction, but we need to write a
better CS object that allows us to change this stuff without a 200-line patch :)

[sashab, 2016-12-04 23:24:06]

So, at least for the moment, not lgtm.