Add a fast-path for independent inherited properties

Add a fast-path for independent inherited properties

(Note: This patch was reverted in crrev.com/2213223004 because of issues
crbug.com/633859 and crbug.com/634254. These issues were caused from
setting isInherited to false instead of true for the StyleBuilder
generated inherit functions, and a slotting bug from skipping child
recalc in HTMLSlotElement and InsertionPoints, which have both now been
fixed in this patch & have tests added.)

Add a fast-path for inherited properties which do not depend on and do
not affect any other properties on ComputedStyle. When these properties
are modified in a parent element, set them directly on ComputedStyle and
skip doing a full recalc for elements only affected by this change.

Also implemented two of these properties: visibility and pointer-events,
storing an extra 2 bits per ComputedStyle. This increases the size of
ComputedStyle by 1 byte on Windows and some Android builds (due to
aligned fields), which increases the memory usage for a standard page
with ~1000 elements by up to 1kb (although potentially up to 4/8kb on
32/64 bit builds due to packing, although this depends on the allocator
implementation details) but realistically less since style sharing only
creates one ComputedStyle object for each unique style.

Benchmarks show a speed increase of up to 2x for setting these
properties on the root element of a typical web page (Facebook, Twitter,
Pinterest, Amazon, Wikipedia) and letting the change propagate directly
onto the child ComputedStyle objects, rather than doing a full style
recalc.

Initial Benchmarks:
https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_g--B4w/edit#gid=1597242813

Follow-up Benchmarks:
https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_g--B4w/edit#gid=918856082

BUG=622138
Committed: https://crrev.com/d0c57905368c76aba662dbda3b4e13cb9bd454bf
Cr-Commit-Position: refs/heads/master@{#413406}

[sashab, 2016-08-08 01:10:38]

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

[commit-bot: I haz the power, 2016-08-08 01:10:42]

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-08 02:26:02]

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

[commit-bot: I haz the power, 2016-08-08 02:26:03]

Dry run: This issue passed the CQ dry run.

[sashab, 2016-08-18 04:47:14]

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

[sashab, 2016-08-18 04:47:15]

Hi Rune - me again :) This patch...

This was LGTMd by esprehn@ and landed. The only 2 changes are the ones I've
commented on to fix the filed bugs. One I'm sure about and one I'm not-so-sure.
PTAL and let me know what you think :)

Thanks,

Sasha

https://codereview.chromium.org/2220873002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl
(right):

https://codereview.chromium.org/2220873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:81:
{{set_is_inherited(property)}}(true);
Fix for hidden tabs problem - crbug.com/634254

https://codereview.chromium.org/2220873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right):

https://codereview.chromium.org/2220873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:142: change = Inherit;
Fix for shadow roots problem - crbug.com/633859. I can't recreate a minimal test
case for this bug and I'm still not sure I fully understand why it's happening.
I think it's because propagateInheritedProperties() assumes we inherit styles
from the parent element, but in a shadow root this isn't the case.

An alternative fix for this would probably be to change
propagateInheritedProperties to check if the parent is a shadow root, and not
run the fast path in this case. Either way we've effectively disabling
IndependentInherit changes across ShadowRoot boundaries, which I guess makes
sense since we shouldn't be propagating changes across to shadow doms from the
main dom anyway.

[rune, 2016-08-18 08:06:49]

https://codereview.chromium.org/2220873002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl
(right):

https://codereview.chromium.org/2220873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:65:
{{set_is_inherited(property)}}(true);
Shouldn't this be false? Last time I looked I thought initial and inherit were
both flipped.

https://codereview.chromium.org/2220873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right):

https://codereview.chromium.org/2220873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:142: change = Inherit;
On 2016/08/18 04:47:15, sashab wrote:
> Fix for shadow roots problem - crbug.com/633859. I can't recreate a minimal
test
> case for this bug and I'm still not sure I fully understand why it's
happening.
> I think it's because propagateInheritedProperties() assumes we inherit styles
> from the parent element, but in a shadow root this isn't the case.
> 
> An alternative fix for this would probably be to change
> propagateInheritedProperties to check if the parent is a shadow root, and not
> run the fast path in this case. Either way we've effectively disabling
> IndependentInherit changes across ShadowRoot boundaries, which I guess makes
> sense since we shouldn't be propagating changes across to shadow doms from the
> main dom anyway.

I think I know what's happening. We traverse the full dom in tree-of-trees order
and not flat-tree order. Inheritance happens in the flat-tree order. For
instance:

== tree-of-trees:

<div id="host">
  <:shadow-root>
    <span>
      <slot></slot>
    </span>
  <:shadow-root>
  <div id="c1"></div>
  <div id="c2"></div>
</div>

== flat-tree:

<div id="host">
  <span>
    <div id="c1"></div>
    <div id="c2"></div>
  </span>
</div>


So, inheritance should happen from span to the distributed c1/c2 children. When
we recalculate style for a shadow host, we recalculate style for the shadow
tree(s) first. When we reach an insertion point (<slot> for v1, <content> for
v0), we check the StyleRecalcChange value and mark the distributed nodes (here
c1/c2) for recalc if the change is >= Inherit
(HTMLSlotElement::willRecalcStyle() and InsertionPoint::willRecalcStyle()).

Propagation should be fine down the shadow tree's light children, but the light
tree children of the host cannot use propagated inherit based on its light tree
parent. In fact, that's sub-optimal today because we use the StyleRecalcChange
of the shadow host when we figure out if we need to recalc the distributed
nodes. If the recalc of #host in the example above yields an Inherit, we
recalculate the style of c1/c2, but if the span recalc ends up in NoInherit or
NoChange, we shouldn't need to recalc for inheritance for c1/c2.

I think the conclusion for your changes, is that it should work to change the
implementation of HTMLSlotElement::willRecalcStyle and
InsertionPoint::willRecalcStyle to set LocalStyleChange for IndependentInherit.


A test-case that should trigger the bug is to change an independently inherited
property on the span in the example above and see that it is inherited into the
c1/c2 child(ren).

[sashab, 2016-08-19 05:24:47]

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

[commit-bot: I haz the power, 2016-08-19 05:24:59]

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

[sashab, 2016-08-19 05:27:35]

Description was changed from

==========
Add a fast-path for independent inherited properties

(Note: This patch was reverted in crrev.com/2213223004 because of issues
crbug.com/633859 and crbug.com/634254. These issues were caused from
setting isInherited to false instead of true for the StyleBuilder
generated inherit functions, which has now been fixed in this patch.)

Add a fast-path for inherited properties which do not depend on and do
not affect any other properties on ComputedStyle. When these properties
are modified in a parent element, set them directly on ComputedStyle and
skip doing a full recalc for elements only affected by this change.

Also implemented two of these properties: visibility and pointer-events,
storing an extra 2 bits per ComputedStyle. This increases the size of
ComputedStyle by 1 byte on Windows and some Android builds (due to
aligned fields), which increases the memory usage for a standard page
with ~1000 elements by up to 1kb (although potentially up to 4/8kb on
32/64 bit builds due to packing, although this depends on the allocator
implementation details) but realistically less since style sharing only
creates one ComputedStyle object for each unique style.

Benchmarks show a speed increase of up to 2x for setting these
properties on the root element of a typical web page (Facebook, Twitter,
Pinterest, Amazon, Wikipedia) and letting the change propagate directly
onto the child ComputedStyle objects, rather than doing a full style
recalc.

Initial Benchmarks:
https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_...

Follow-up Benchmarks:
https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_...

BUG=622138
==========

to

==========
Add a fast-path for independent inherited properties

(Note: This patch was reverted in crrev.com/2213223004 because of issues
crbug.com/633859 and crbug.com/634254. These issues were caused from
setting isInherited to false instead of true for the StyleBuilder
generated inherit functions, and a slotting bug from skipping child
recalc in HTMLSlotElement and InsertionPoints, which have both now been
fixed in this patch.)

Add a fast-path for inherited properties which do not depend on and do
not affect any other properties on ComputedStyle. When these properties
are modified in a parent element, set them directly on ComputedStyle and
skip doing a full recalc for elements only affected by this change.

Also implemented two of these properties: visibility and pointer-events,
storing an extra 2 bits per ComputedStyle. This increases the size of
ComputedStyle by 1 byte on Windows and some Android builds (due to
aligned fields), which increases the memory usage for a standard page
with ~1000 elements by up to 1kb (although potentially up to 4/8kb on
32/64 bit builds due to packing, although this depends on the allocator
implementation details) but realistically less since style sharing only
creates one ComputedStyle object for each unique style.

Benchmarks show a speed increase of up to 2x for setting these
properties on the root element of a typical web page (Facebook, Twitter,
Pinterest, Amazon, Wikipedia) and letting the change propagate directly
onto the child ComputedStyle objects, rather than doing a full style
recalc.

Initial Benchmarks:
https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_...

Follow-up Benchmarks:
https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_...

BUG=622138
==========

[sashab, 2016-08-19 05:34:07]

Description was changed from

==========
Add a fast-path for independent inherited properties

(Note: This patch was reverted in crrev.com/2213223004 because of issues
crbug.com/633859 and crbug.com/634254. These issues were caused from
setting isInherited to false instead of true for the StyleBuilder
generated inherit functions, and a slotting bug from skipping child
recalc in HTMLSlotElement and InsertionPoints, which have both now been
fixed in this patch.)

Add a fast-path for inherited properties which do not depend on and do
not affect any other properties on ComputedStyle. When these properties
are modified in a parent element, set them directly on ComputedStyle and
skip doing a full recalc for elements only affected by this change.

Also implemented two of these properties: visibility and pointer-events,
storing an extra 2 bits per ComputedStyle. This increases the size of
ComputedStyle by 1 byte on Windows and some Android builds (due to
aligned fields), which increases the memory usage for a standard page
with ~1000 elements by up to 1kb (although potentially up to 4/8kb on
32/64 bit builds due to packing, although this depends on the allocator
implementation details) but realistically less since style sharing only
creates one ComputedStyle object for each unique style.

Benchmarks show a speed increase of up to 2x for setting these
properties on the root element of a typical web page (Facebook, Twitter,
Pinterest, Amazon, Wikipedia) and letting the change propagate directly
onto the child ComputedStyle objects, rather than doing a full style
recalc.

Initial Benchmarks:
https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_...

Follow-up Benchmarks:
https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_...

BUG=622138
==========

to

==========
Add a fast-path for independent inherited properties

(Note: This patch was reverted in crrev.com/2213223004 because of issues
crbug.com/633859 and crbug.com/634254. These issues were caused from
setting isInherited to false instead of true for the StyleBuilder
generated inherit functions, and a slotting bug from skipping child
recalc in HTMLSlotElement and InsertionPoints, which have both now been
fixed in this patch & have tests added.)

Add a fast-path for inherited properties which do not depend on and do
not affect any other properties on ComputedStyle. When these properties
are modified in a parent element, set them directly on ComputedStyle and
skip doing a full recalc for elements only affected by this change.

Also implemented two of these properties: visibility and pointer-events,
storing an extra 2 bits per ComputedStyle. This increases the size of
ComputedStyle by 1 byte on Windows and some Android builds (due to
aligned fields), which increases the memory usage for a standard page
with ~1000 elements by up to 1kb (although potentially up to 4/8kb on
32/64 bit builds due to packing, although this depends on the allocator
implementation details) but realistically less since style sharing only
creates one ComputedStyle object for each unique style.

Benchmarks show a speed increase of up to 2x for setting these
properties on the root element of a typical web page (Facebook, Twitter,
Pinterest, Amazon, Wikipedia) and letting the change propagate directly
onto the child ComputedStyle objects, rather than doing a full style
recalc.

Initial Benchmarks:
https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_...

Follow-up Benchmarks:
https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_...

BUG=622138
==========

[sashab, 2016-08-19 05:34:31]

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

[commit-bot: I haz the power, 2016-08-19 05:34:45]

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

[sashab, 2016-08-19 05:35:25]

Thanks Rune -- both bugs fixed properly & added layout tests for them & an
additional layout test for the initial keyword. PTAL, hopefully all good this
time! :)

+esprehn for fyi :)

https://codereview.chromium.org/2220873002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl
(right):

https://codereview.chromium.org/2220873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:65:
{{set_is_inherited(property)}}(true);
On 2016/08/18 at 08:06:49, rune wrote:
> Shouldn't this be false? Last time I looked I thought initial and inherit were
both flipped.

Oops, nice catch! Not sure how that happened, yes, this should definitely be
false

https://codereview.chromium.org/2220873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right):

https://codereview.chromium.org/2220873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:142: change = Inherit;
On 2016/08/18 at 08:06:49, rune wrote:
> On 2016/08/18 04:47:15, sashab wrote:
> > Fix for shadow roots problem - crbug.com/633859. I can't recreate a minimal
test
> > case for this bug and I'm still not sure I fully understand why it's
happening.
> > I think it's because propagateInheritedProperties() assumes we inherit
styles
> > from the parent element, but in a shadow root this isn't the case.
> > 
> > An alternative fix for this would probably be to change
> > propagateInheritedProperties to check if the parent is a shadow root, and
not
> > run the fast path in this case. Either way we've effectively disabling
> > IndependentInherit changes across ShadowRoot boundaries, which I guess makes
> > sense since we shouldn't be propagating changes across to shadow doms from
the
> > main dom anyway.
> 
> I think I know what's happening. We traverse the full dom in tree-of-trees
order and not flat-tree order. Inheritance happens in the flat-tree order. For
instance:
> 
> == tree-of-trees:
> 
> <div id="host">
>   <:shadow-root>
>     <span>
>       <slot></slot>
>     </span>
>   <:shadow-root>
>   <div id="c1"></div>
>   <div id="c2"></div>
> </div>
> 
> == flat-tree:
> 
> <div id="host">
>   <span>
>     <div id="c1"></div>
>     <div id="c2"></div>
>   </span>
> </div>
> 
> 
> So, inheritance should happen from span to the distributed c1/c2 children.
When we recalculate style for a shadow host, we recalculate style for the shadow
tree(s) first. When we reach an insertion point (<slot> for v1, <content> for
v0), we check the StyleRecalcChange value and mark the distributed nodes (here
c1/c2) for recalc if the change is >= Inherit
(HTMLSlotElement::willRecalcStyle() and InsertionPoint::willRecalcStyle()).
> 
> Propagation should be fine down the shadow tree's light children, but the
light tree children of the host cannot use propagated inherit based on its light
tree parent. In fact, that's sub-optimal today because we use the
StyleRecalcChange of the shadow host when we figure out if we need to recalc the
distributed nodes. If the recalc of #host in the example above yields an
Inherit, we recalculate the style of c1/c2, but if the span recalc ends up in
NoInherit or NoChange, we shouldn't need to recalc for inheritance for c1/c2.
> 
> I think the conclusion for your changes, is that it should work to change the
implementation of HTMLSlotElement::willRecalcStyle and
InsertionPoint::willRecalcStyle to set LocalStyleChange for IndependentInherit.
> 
> 
> A test-case that should trigger the bug is to change an independently
inherited property on the span in the example above and see that it is inherited
into the c1/c2 child(ren).

Thanks for the excellent summary Rune! You were absolutely right -- and I
understand why as well -- added a test and validated that this fix also fixes
the user manager bug.

In hindsight I'm surprised I didn't notice this when looking for usages of
StyleRecalcChange::Inherit.

Updated the CL description too. :)

[commit-bot: I haz the power, 2016-08-19 07:10:24]

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

[commit-bot: I haz the power, 2016-08-19 07:10:25]

Dry run: This issue passed the CQ dry run.

[rune, 2016-08-19 08:30:57]

lgtm

[sashab, 2016-08-22 00:35:16]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2016-08-22 00:35:30]

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-22 02:03:38]

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

[commit-bot: I haz the power, 2016-08-22 02:03:40]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sashab, 2016-08-22 03:03:23]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2016-08-22 03:03:32]

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-22 04:52:39]

Committed patchset #4 (id:60001)

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

Description was changed from

==========
Add a fast-path for independent inherited properties

(Note: This patch was reverted in crrev.com/2213223004 because of issues
crbug.com/633859 and crbug.com/634254. These issues were caused from
setting isInherited to false instead of true for the StyleBuilder
generated inherit functions, and a slotting bug from skipping child
recalc in HTMLSlotElement and InsertionPoints, which have both now been
fixed in this patch & have tests added.)

Add a fast-path for inherited properties which do not depend on and do
not affect any other properties on ComputedStyle. When these properties
are modified in a parent element, set them directly on ComputedStyle and
skip doing a full recalc for elements only affected by this change.

Also implemented two of these properties: visibility and pointer-events,
storing an extra 2 bits per ComputedStyle. This increases the size of
ComputedStyle by 1 byte on Windows and some Android builds (due to
aligned fields), which increases the memory usage for a standard page
with ~1000 elements by up to 1kb (although potentially up to 4/8kb on
32/64 bit builds due to packing, although this depends on the allocator
implementation details) but realistically less since style sharing only
creates one ComputedStyle object for each unique style.

Benchmarks show a speed increase of up to 2x for setting these
properties on the root element of a typical web page (Facebook, Twitter,
Pinterest, Amazon, Wikipedia) and letting the change propagate directly
onto the child ComputedStyle objects, rather than doing a full style
recalc.

Initial Benchmarks:
https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_...

Follow-up Benchmarks:
https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_...

BUG=622138
==========

to

==========
Add a fast-path for independent inherited properties

(Note: This patch was reverted in crrev.com/2213223004 because of issues
crbug.com/633859 and crbug.com/634254. These issues were caused from
setting isInherited to false instead of true for the StyleBuilder
generated inherit functions, and a slotting bug from skipping child
recalc in HTMLSlotElement and InsertionPoints, which have both now been
fixed in this patch & have tests added.)

Add a fast-path for inherited properties which do not depend on and do
not affect any other properties on ComputedStyle. When these properties
are modified in a parent element, set them directly on ComputedStyle and
skip doing a full recalc for elements only affected by this change.

Also implemented two of these properties: visibility and pointer-events,
storing an extra 2 bits per ComputedStyle. This increases the size of
ComputedStyle by 1 byte on Windows and some Android builds (due to
aligned fields), which increases the memory usage for a standard page
with ~1000 elements by up to 1kb (although potentially up to 4/8kb on
32/64 bit builds due to packing, although this depends on the allocator
implementation details) but realistically less since style sharing only
creates one ComputedStyle object for each unique style.

Benchmarks show a speed increase of up to 2x for setting these
properties on the root element of a typical web page (Facebook, Twitter,
Pinterest, Amazon, Wikipedia) and letting the change propagate directly
onto the child ComputedStyle objects, rather than doing a full style
recalc.

Initial Benchmarks:
https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_...

Follow-up Benchmarks:
https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_...

BUG=622138

Committed: https://crrev.com/d0c57905368c76aba662dbda3b4e13cb9bd454bf
Cr-Commit-Position: refs/heads/master@{#413406}
==========

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

Patchset 4 (id:??) landed as
https://crrev.com/d0c57905368c76aba662dbda3b4e13cb9bd454bf
Cr-Commit-Position: refs/heads/master@{#413406}