Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2117143003:
Add a fast-path for independent inherited properties (Closed)
|
Created:
4 years, 5 months ago by sashab Modified:
4 years, 4 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-dom_chromium.org, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@computedstyle_cleanup_rename_final_member_fields Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a fast-path for independent inherited properties
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/f24dba9f04dd093aac4298378c671ecd44d0fe97
Cr-Commit-Position: refs/heads/master@{#409143}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Rebase #Patch Set 4 : Rebase #Patch Set 5 : Small rename #
Total comments: 11
Patch Set 6 : Review feedback and removed size check to see size of ComputedStyle on windows #Patch Set 7 : Checked whether the size increased by 1 byte #Patch Set 8 : Put sizeof check back to normal #Patch Set 9 : Added useful sizeof check comment #
Total comments: 39
Patch Set 10 : Review feedback #Patch Set 11 : Review feedback #Patch Set 12 : Rebase #
Total comments: 6
Patch Set 13 : Review feedback #Messages
Total messages: 58 (39 generated)
Description was changed from ========== Add a fast-path for independent inherited properties 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 implement two of these properties: visibility and poitner-events. Benchmarks: https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_... BUG=622138 ========== to ========== Add a fast-path for independent inherited properties 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. Benchmarks: https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_... BUG=622138 ==========
sashab@chromium.org changed reviewers: + meade@chromium.org
Hey eddy, PTAL :)
I'm not sure I understand exactly what's going on, but the obvious questions for me are (which perhaps the answers should go in the CL description): - Have you tested how much extra memory this uses (how would this affect typical websites)? -In the benchmarks, the control group shows average time improvement of -9.45%, is that accurate/valid? What would be affecting that? https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css/invalidation/independent-inheritance-fast-path-multiple-properties.html (right): https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/invalidation/independent-inheritance-fast-path-multiple-properties.html:17: <div id="f"> Is it necessary to do so many levels of nesting here? Would it be sufficient to do 2 or 3? https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.h:653: // that can be directly propagated to the child. I found this a little hard to understand - "the child" is this element right? And this is checking whether the only changed properties are independent (and thus whether a full style recalc is required)? https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:319: } m_isInheritedFlags; It's not clear to me how m_inheritedData and m_isInheritedFlags interact. I think IsInheritedFlags are the independent properties that are inherited from the parent style. If that's the case, would it be clearer to call it m_IndependentlyInheritedData (or Flags?) or something else if I'm misunderstanding?? https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:371: // All properties default to being inherited. All independently inherited properties? https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:404: // child that are not explicitly set in child. You have the word "child" twice :)
Description was changed from ========== Add a fast-path for independent inherited properties 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. Benchmarks: https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_... BUG=622138 ========== to ========== Add a fast-path for independent inherited properties 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. Benchmarks: https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_... BUG=622138 ==========
The CQ bit was checked by sashab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sashab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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...)
The CQ bit was checked by sashab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add a fast-path for independent inherited properties 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. Benchmarks: https://docs.google.com/spreadsheets/d/1mUuJEs8cPWyNTR7tQw27oxq6fDTvWiAwgatf_... BUG=622138 ========== to ========== Add a fast-path for independent inherited properties 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 (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 ==========
The CQ bit was checked by sashab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Eddy, Thanks for the great feedback -- I've taken it into account, as well as what we discussed in-person, and replied inline. Also updated the patch description + ran another benchmark to check speed results (turns out font-size results were random and speed doesn't seem to be affected... if anything the speed has improved which of course, makes no sense. ^_^) See Patch #6 for all systems that use an extra bit for ComputedStyle. See Patch #7 for all systems that increase the size of ComputedStyle by 1 bit (should be the same bots). I've realized why the sizeof() check is the way it is -- it's for exactly this reason -- different systems pack ComputedStyle differently. I left the struct check in-place and added a comment to explain why it is there now. :) LMKWYT! :) Thanks, Sasha https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css/invalidation/independent-inheritance-fast-path-multiple-properties.html (right): https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/invalidation/independent-inheritance-fast-path-multiple-properties.html:17: <div id="f"> On 2016/07/13 at 07:29:12, Eddy wrote: > Is it necessary to do so many levels of nesting here? Would it be sufficient to do 2 or 3? That's a good question! Added comments to explain what each level is for, which forced me to think about it too lol https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.h:653: // that can be directly propagated to the child. On 2016/07/13 at 07:29:13, Eddy wrote: > I found this a little hard to understand - "the child" is this element right? > > And this is checking whether the only changed properties are independent (and thus whether a full style recalc is required)? Yup :) clarified the comment. https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:319: } m_isInheritedFlags; On 2016/07/13 at 07:29:13, Eddy wrote: > It's not clear to me how m_inheritedData and m_isInheritedFlags interact. > > I think IsInheritedFlags are the independent properties that are inherited from the parent style. If that's the case, would it be clearer to call it m_IndependentlyInheritedData (or Flags?) or something else if I'm misunderstanding?? Added a comment to explain the better way to implement this, as discussed :) https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:371: // All properties default to being inherited. On 2016/07/13 at 07:29:13, Eddy wrote: > All independently inherited properties? Thanks!! Done. https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:371: // All properties default to being inherited. On 2016/07/13 at 07:29:13, Eddy wrote: > All independently inherited properties? Thanks!! Done. https://codereview.chromium.org/2117143003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:404: // child that are not explicitly set in child. On 2016/07/13 at 07:29:13, Eddy wrote: > You have the word "child" twice :) Haha thanks :) Nice find
lgtm as far as I can tell. Waiting eagerly hear what people with more experience in this area will say :)
sashab@chromium.org changed reviewers: + esprehn@chromium.org
Ty eddy!! :) esprehn ptal :) (I think rune is on vac)
On 2016/07/14 at 05:52:11, sashab wrote: > Ty eddy!! :) > > esprehn ptal :) (I think rune is on vac) Sure I'll look tomorrow. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/14 at 06:43:05, esprehn wrote: > On 2016/07/14 at 05:52:11, sashab wrote: > > Ty eddy!! :) > > > > esprehn ptal :) (I think rune is on vac) > > Sure I'll look tomorrow. :) Ping! Tomorrow? :)
timloh@chromium.org changed reviewers: + timloh@chromium.org
description "which increases the memory usage for a standard page with ~1000 elements by up to 1kb" is not really accurate, due to packing, sizeof(ComputedStyle) will probably increase by 4 bytes (on 32-bit builds), but the actual change could either be higher or lower depending on allocator implementation details. https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/css_properties.py (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/css_properties.py:25: 'isInheritedSetter': None, Doesn't need to be here if it isn't set from the .in file, also casing is inconsistent with everything else https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSProperties.in:53: // Additionally, a change by an independent property must cause Why? https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1786: newStyle = ComputedStyle::clone(*computedStyle()); clone(oldStyle)? https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.cpp:67: // Since different platforms pack ComputedStyle differently, re-create the same Are you sure this gets packed differently on different platforms? In any case, any difference is more likely due to compiler/architecture rather than platform. https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:413: static void propagateIndependentInheritedProperties(const ComputedStyle& parent, ComputedStyle& child); Why static?
Description was changed from ========== Add a fast-path for independent inherited properties 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 (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 ========== is not really accurate, due to packing, sizeof(ComputedStyle) will probably increase by 4 bytes (on 32-bit builds), but the actual change could either be higher or lower depending on allocator implementation details. Add a fast-path for independent inherited properties 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 ==========
Description was changed from ========== is not really accurate, due to packing, sizeof(ComputedStyle) will probably increase by 4 bytes (on 32-bit builds), but the actual change could either be higher or lower depending on allocator implementation details. Add a fast-path for independent inherited properties 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 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 ==========
The CQ bit was checked by sashab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Done. https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/css_properties.py (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/css_properties.py:25: 'isInheritedSetter': None, On 2016/07/19 at 01:29:31, Timothy Loh wrote: > Doesn't need to be here if it isn't set from the .in file, also casing is inconsistent with everything else Removed. https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSProperties.in:53: // Additionally, a change by an independent property must cause On 2016/07/19 at 01:29:32, Timothy Loh wrote: > Why? Removed? https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1786: newStyle = ComputedStyle::clone(*computedStyle()); On 2016/07/19 at 01:29:32, Timothy Loh wrote: > clone(oldStyle)? Done https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.cpp:67: // Since different platforms pack ComputedStyle differently, re-create the same On 2016/07/19 at 01:29:32, Timothy Loh wrote: > Are you sure this gets packed differently on different platforms? In any case, any difference is more likely due to compiler/architecture rather than platform. Done. https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:413: static void propagateIndependentInheritedProperties(const ComputedStyle& parent, ComputedStyle& child); On 2016/07/19 at 01:29:32, Timothy Loh wrote: > Why static? Done.
https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/css_properties.py (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/css_properties.py:25: 'isInheritedSetter': None, I think you want is_inherited_setter, but timloh@ seems to be saying you don't need it? https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_style_builder.py (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_style_builder.py:62: set_if_none(property, 'isInheritedSetter', 'set' + name + 'IsInherited') is_inherited_setter, the rest of the names here are all hacker_case https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:65: {{set_is_inherited(property)}}(true); what is this code generating that you want to set true, false? It's hard to understand what all is going on with the templates right here https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSProperties.in:48: // - independent I think you want to mention that StyleAdjuster can't mess with properties that take this fast path? https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSProperties.in:54: // ComputedStyle to change (e.g. font-size: 10px; and font-size: 1em; I'm not sure this is a good example, font-size impacts all properties that use relative units, which is why it's a "high priority property". We'd need to store tons of information but the comment doesn't really explain it. https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1763: return change == IndependentInherit if (change != ...) return false; if (!parentComputedStyle()) return false; if (hasAnimations()) return false; ComputedStyle* style = ...; return !style->animations() && !style->transitions(); That style is easier to read and doesn't recompute the same stuff repeatedly. :) https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1765: && computedStyle() computedStyle() contains a bunch of branches, you don't want to call it repeatedly like this https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1786: newStyle = ComputedStyle::clone(*computedStyle()); On 2016/07/19 at 01:29:32, Timothy Loh wrote: > clone(oldStyle)? yeah, don't call computedStyle() repeatedly it's got a bunch of branches in it :) https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1787: ComputedStyle::propagateIndependentInheritedProperties(*parentComputedStyle(), *newStyle); parentComputedStyle() isn't free, I think you want to merge this and the canPropagateInheritedProperties into a function like: bool propagateInheritedProperties(change) that has the early returns and avoids calling it multiple times. then do: if (!propagateInheritedProperties(change)) ... https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1794: StyleRecalcChange localChange = ComputedStyle::stylePropagationDiff(oldStyle.get(), newStyle.get()); do we even need to run any of this now if we took the propagation path above? https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.cpp:84: } m_isInheritedFlags; merge this into m_nonInheritedData and it doesn't get any bigger https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.cpp:208: && oldStyle->nonIndependentInheritedEqual(*newStyle) this is a duplicate check with the inheritedEqual thing below I think? We're comparing a bunch of inherited stuff here twice I guess only when you mess with visibility or pointer-events? I think this avoids it? Maybe you can phrase it better. { StyleRecalcChange change = NoChange; if (!oldStyle->independentInheritedEqual(*newStyle)) { change = Inherit; if (oldStyle->nonIndependentInheritedEqual(*newStyle) && !oldStyle->hasExplicitlyInheritedProperties()) change = IndependentInherit; return change; } } if (!oldStyle->loadingCustomFontsEqual(*newStyle)) return Inherit; https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.cpp:228: if (child.m_isInheritedFlags.m_isPointerEventsInherited) I do wonder how fast this will be if there's a ton of properties in here, we can start simple though. https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:174: return (m_visibility == other.m_visibility) add a comment that these must match the .in file and only properties tagged independent can be here? https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:325: unsigned m_isPointerEventsInherited : 1; no m_ on public fields, ComputedStyle is a huge mess of bad naming, sorry it's confusing :) https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:326: unsigned m_isVisibilityInherited : 1; why can't we put this in NonInheritedFlags to avoid the bloat?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sashab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks guys! Comments inline. https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:65: {{set_is_inherited(property)}}(true); On 2016/07/19 at 03:46:26, esprehn wrote: > what is this code generating that you want to set true, false? It's hard to understand what all is going on with the templates right here It's setting that the property is inherited from its parent value, as opposed to below where its setting that it's not inherited. Sorry if its not super clear, trying to think of a way I can clean it up... https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSProperties.in:48: // - independent On 2016/07/19 at 03:46:26, esprehn wrote: > I think you want to mention that StyleAdjuster can't mess with properties that take this fast path? Thanks :) Added that, and cleaned up this comment too. https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSProperties.in:54: // ComputedStyle to change (e.g. font-size: 10px; and font-size: 1em; On 2016/07/19 at 03:46:26, esprehn wrote: > I'm not sure this is a good example, font-size impacts all properties that use relative units, which is why it's a "high priority property". We'd need to store tons of information but the comment doesn't really explain it. Removed this comment, its not really needed anyway. https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1763: return change == IndependentInherit On 2016/07/19 at 03:46:26, esprehn wrote: > if (change != ...) > return false; > if (!parentComputedStyle()) > return false; > if (hasAnimations()) > return false; > ComputedStyle* style = ...; > return !style->animations() && !style->transitions(); > > That style is easier to read and doesn't recompute the same stuff repeatedly. :) Nice :) Thanks, done https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1765: && computedStyle() On 2016/07/19 at 03:46:26, esprehn wrote: > computedStyle() contains a bunch of branches, you don't want to call it repeatedly like this Yup nice, thanks https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1787: ComputedStyle::propagateIndependentInheritedProperties(*parentComputedStyle(), *newStyle); On 2016/07/19 at 03:46:26, esprehn wrote: > parentComputedStyle() isn't free, I think you want to merge this and the canPropagateInheritedProperties into a function like: > > bool propagateInheritedProperties(change) > > that has the early returns and avoids calling it multiple times. > > then do: > > if (!propagateInheritedProperties(change)) > ... Nice!! Done. Had to pass newStyle because its used later in the function. Could technically get rid of the double-call from oldStyle = mutableComputedStyle() but probably not worth it. :) https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1794: StyleRecalcChange localChange = ComputedStyle::stylePropagationDiff(oldStyle.get(), newStyle.get()); On 2016/07/19 at 03:46:26, esprehn wrote: > do we even need to run any of this now if we took the propagation path above? That's a good question. I've left it this way now so that: 1) The stats counter gets incremented 2) Callback selectors get updated (what is that even?) but that's it. The rest is skipped. If you think neither of those things are worth it then we can early return. https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.cpp:84: } m_isInheritedFlags; On 2016/07/19 at 03:46:26, esprehn wrote: > merge this into m_nonInheritedData and it doesn't get any bigger Woo done. :) https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.cpp:208: && oldStyle->nonIndependentInheritedEqual(*newStyle) On 2016/07/19 at 03:46:26, esprehn wrote: > this is a duplicate check with the inheritedEqual thing below I think? We're comparing a bunch of inherited stuff here twice I guess only when you mess with visibility or pointer-events? > > I think this avoids it? Maybe you can phrase it better. > > { > StyleRecalcChange change = NoChange; > if (!oldStyle->independentInheritedEqual(*newStyle)) { > change = Inherit; > if (oldStyle->nonIndependentInheritedEqual(*newStyle) && !oldStyle->hasExplicitlyInheritedProperties()) > change = IndependentInherit; > return change; > } > } > > if (!oldStyle->loadingCustomFontsEqual(*newStyle)) > return Inherit; Unfortunately that doesn't quite work... See new code. I needed a boolean to store one of them so I just made a boolean for both, unfortunately you need the nonIndependentEqual check twice either way, can't think of a way to fix this... One alternative might be: if (!independentEqual) { if (nonIndependentEqual && !oldStyle->hasExplicitlyInheritedProperties()) return IndependentInherit; return Inherit; } if (!oldStyle->loadingCustomFontsEqual(*newStyle) || !nonIndependentEqual) return Inherit; https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.cpp:228: if (child.m_isInheritedFlags.m_isPointerEventsInherited) On 2016/07/19 at 03:46:26, esprehn wrote: > I do wonder how fast this will be if there's a ton of properties in here, we can start simple though. Yeah, that's why the TODO to generate :) https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:174: return (m_visibility == other.m_visibility) On 2016/07/19 at 03:46:26, esprehn wrote: > add a comment that these must match the .in file and only properties tagged independent can be here? Nice, yup done. Also added a TODO to generate this.. Haha :) https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:325: unsigned m_isPointerEventsInherited : 1; On 2016/07/19 at 03:46:26, esprehn wrote: > no m_ on public fields, ComputedStyle is a huge mess of bad naming, sorry it's confusing :) ?? Oh, they are all called m_, so I just changed that... What is it meant to be called? What style guide are you referring to? If that's the case I'll just change them all in one go https://codereview.chromium.org/2117143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:326: unsigned m_isVisibilityInherited : 1; On 2016/07/19 at 03:46:26, esprehn wrote: > why can't we put this in NonInheritedFlags to avoid the bloat? Yeah, that's a good point. Originally I didn't do that because I had to split the compareEqual() function everywhere, but I've had to do that anyway so now it's worth it. Thanks!! P.S. This is a GREAT!!! idea. Makes the patch way smaller & takes up less space, can't believe I didn't think of it before!! Although a little more confusing with them all being in NonInheritedData, but this is getting generated anyway so who cares. Awesome. :D
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by sashab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping esprehn :)
The CQ bit was checked by sashab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Amazing! lgtm w/ nits. https://codereview.chromium.org/2117143003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2117143003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1790: RefPtr<ComputedStyle> newStyle; can we do: newStyle = propagateInheritedProperties(change); here and below do: if (!newStyle) newStyle = styleForLayoutObject(); the ! inside the if statement is a little bit hard to read https://codereview.chromium.org/2117143003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2117143003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:306: // For each independent inherited property, store a 1 if the store stored https://codereview.chromium.org/2117143003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:313: // to the existing flags take advantage of packing as much as possible to take
The CQ bit was checked by sashab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2117143003/#ps240001 (title: "Review feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Add a fast-path for independent inherited properties 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 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/f24dba9f04dd093aac4298378c671ecd44d0fe97 Cr-Commit-Position: refs/heads/master@{#409143} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/f24dba9f04dd093aac4298378c671ecd44d0fe97 Cr-Commit-Position: refs/heads/master@{#409143}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2213223004/ by rune@opera.com. The reason for reverting is: Caused issues 634254 and 633859..
Message was sent while issue was closed.
WIP in crrev.com/2220873002. Still struggling to fix crbug.com/633859, but I think it has something to do with not propagating through shadow roots. A bugfix week followed by sheriffing has been pretty bad timing but looking forward to getting back to this soon :) https://codereview.chromium.org/2117143003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2117143003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1790: RefPtr<ComputedStyle> newStyle; On 2016/08/02 at 02:34:58, esprehn wrote: > can we do: > > newStyle = propagateInheritedProperties(change); > > here and below do: > > if (!newStyle) > newStyle = styleForLayoutObject(); > > the ! inside the if statement is a little bit hard to read Yup, sg! https://codereview.chromium.org/2117143003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2117143003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:306: // For each independent inherited property, store a 1 if the store On 2016/08/02 at 02:34:58, esprehn wrote: > stored Done https://codereview.chromium.org/2117143003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:313: // to the existing flags take advantage of packing as much as possible On 2016/08/02 at 02:34:58, esprehn wrote: > to take Done, and added full stop to end of sentence.
Message was sent while issue was closed.
WIP in crrev.com/2220873002. Still struggling to fix crbug.com/633859, but I think it has something to do with not propagating through shadow roots. A bugfix week followed by sheriffing has been pretty bad timing but looking forward to getting back to this soon :) |
