Add fast-path for propagated variable changes (WIP)

Add fast-path for propagated variable changes (WIP)

BUG=622138

[sashab, 2016-07-14 06:17:41]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 06:18:00]

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-07-14 06:19:50]

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

[commit-bot: I haz the power, 2016-07-14 06:19:51]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sashab, 2016-07-15 01:26:33]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 01:27:09]

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-07-15 01:30:24]

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

[commit-bot: I haz the power, 2016-07-15 01:30:25]

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

[sashab, 2016-07-15 03:27:30]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 03:27:40]

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-07-15 04:07:59]

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

[commit-bot: I haz the power, 2016-07-15 04:08:01]

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

[sashab, 2016-07-18 06:26:23]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 06:26:32]

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-07-18 06:59:12]

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

[commit-bot: I haz the power, 2016-07-18 06:59:13]

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

[sashab, 2016-07-19 03:39:48]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 03:40:07]

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-07-19 04:23:03]

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

[commit-bot: I haz the power, 2016-07-19 04:23:04]

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

[sashab, 2016-07-19 08:16:31]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 08:16:46]

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-07-19 08:22:05]

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

[commit-bot: I haz the power, 2016-07-19 08:22:06]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sashab, 2016-07-20 01:11:48]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 01:12:37]

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-07-20 02:00:39]

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

[commit-bot: I haz the power, 2016-07-20 02:00:40]

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

[sashab, 2016-08-01 05:10:50]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 05:11: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-08-01 06:03:57]

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

[commit-bot: I haz the power, 2016-08-01 06:03:58]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sashab, 2016-09-26 01:54:05]

sashab@chromium.org changed reviewers:
+ rune@opera.com

[sashab, 2016-09-26 01:54:06]

Hi Rune - this patch is WIP but wondering if I can get your feedback on this
idea. No need to read the code but it's there to provide some context.

I'm trying to get a fast-path for variables so that when you change a variable,
if the variable doesn't actually change any properties, it's skips style recalc
and keeps propagating the variable down. I think I can this to work without
storing a HashSet of which variables are defined in an element, I can just infer
that information from StyleVariableData. Most of this patch is not needed, it's
just me fiddling around :) The base logic looks like this:

In ComputedStyle::stylePropagationDiff:
  If parent.variables and child.variables are different:
    return IndependentInherit

In ComputedStyle::propagateIndependentInheritedProperties:
  For variable in parent:
    If variable not set in child:
      child.variable = parent.variable

The problem with this logic is that StyleVariableData is in
StyleRareInheritedData, and is not copied when a child doesn't set any
variables. Right now:

<div id="outer" style="--x: 1">
	<div id="inner">

Both outer and inner have the same StyleVariableData. But what I want is for
inner to have a StyleVariableData with no variables defined, that way I can
easily see what it inherits from outer.

Does that make sense? Is this the best way to do what I want? I thought it was
worth getting some ideas. :)

Thanks,

Sash

[rune, 2016-09-26 09:47:22]

On 2016/09/26 at 01:54:06, sashab wrote:
> Hi Rune - this patch is WIP but wondering if I can get your feedback on this
idea. No need to read the code but it's there to provide some context.
> 
> I'm trying to get a fast-path for variables so that when you change a
variable, if the variable doesn't actually change any properties, it's skips
style recalc and keeps propagating the variable down. I think I can this to work
without storing a HashSet of which variables are defined in an element, I can
just infer that information from StyleVariableData. Most of this patch is not
needed, it's just me fiddling around :) The base logic looks like this:
> 
> In ComputedStyle::stylePropagationDiff:
>   If parent.variables and child.variables are different:
>     return IndependentInherit

Variable may be referenced by dependent properties, so you'd need to check that?

Also, if variables are referenced by non-inherited properties for the child?

Re-reading what you said above, the independent inherit can only happen if none
of the variables are referenced in other values. That's typically a Houdini
use-case, right?

> In ComputedStyle::propagateIndependentInheritedProperties:
>   For variable in parent:
>     If variable not set in child:
>       child.variable = parent.variable


> The problem with this logic is that StyleVariableData is in
StyleRareInheritedData, and is not copied when a child doesn't set any
variables. Right now:
> 
> <div id="outer" style="--x: 1">
> 	<div id="inner">
> 
> Both outer and inner have the same StyleVariableData. But what I want is for
inner to have a StyleVariableData with no variables defined, that way I can
easily see what it inherits from outer.

I haven't studied the code for variables, and didn't understand why you wanted
the inner one have no variables defined. Doesn't that mean you'd have to look up
the computed style chain while resolving variables?

[sashab, 2016-12-05 01:47:42]

sashab@chromium.org changed reviewers:
+ napper@chromium.org

[sashab, 2016-12-05 01:47:43]

+napper - fyi, this is a partially-working patch to get a variable change to
propagate without needing a full style recalc. Someone (or myself) could pick
this up and get this working, medium to hard difficulty :)