CSS variable layout overhead
Reported by
elfog...@gmail.com,
Aug 17
|
||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.99 Safari/537.36 Steps to reproduce the problem: 1. Toggle the table visibility on the attached file 2. Performance times are output in title bar (document.title) 3. Comment out custom properties 4. Repeat toggle and measurements What is the expected behavior? Layout times should not be impacted by adding custom properties (that are never references nor used) What went wrong? The more custom properties we add (even if not used) the worst the layout performance gets. For this example the layout times with 700 custom properties at the root are twice as high. In all other browsers the layout and paint are not impacted by css variables. Did this work before? N/A Does this work in other browsers? Yes Chrome version: 70.0.3524.3 Channel: canary OS Version: OS X 10.13.6 Flash Version:
,
Aug 17
Bisected to cc275c99719dca651110ad31085c6401de79056a "Disable style sharing outside tests." Landed in 61.0.3115.0 Simplified repro: open the attached test.html Expected: two similar numbers (like 150ms) and "GOOD" is shown Observed: the second number is 3-4 times bigger (like 150ms, 450ms) and "BAD" is shown
,
Aug 17
,
Aug 17
,
Aug 17
The bisect in #2 is not really interesting. It shows a general regression that we consciously accepted when removing the style sharing code. With style sharing being largely beneficial for big tables like that in the test-case we would just share few ComputedStyle objects for a bunch of elements without matching or applying rules, which means the presence of custom properties would not affect much.
,
Aug 17
We should not need to re-resolve custom properties on every element when inherited and not applying new custom properties on top of the inherited ones: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/resolver/style_resolver.cc?type=cs&q=leviw&sq=package:chromium&g=0&l=1699-1717 Attaching callgrind output for lwc-200-tables.html
,
Aug 17
This appears to be very low-hanging fruit. At least parts of it.
,
Aug 18
,
Aug 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bc90fa26ebfb929f3aa9652ee2cee23866815066 commit bc90fa26ebfb929f3aa9652ee2cee23866815066 Author: Anders Hartvoll Ruud <andruud@chromium.org> Date: Mon Aug 20 22:30:16 2018 Don't re-resolve custom properties. When we inherit a set of custom properties, without modifying that set, we don't need to resolve those properties again. Also, we only need to resolve at all if any values with var()-references have been observed. This CL simply stores a flag which indicates whether the Style[Non]InheritedVariables instance contains unresolved var()- references or not. This fixes a performance issue where variables specified on :root would be re-resolved for all elements in the tree. Bug: 875123 Change-Id: I1f68b4c78465c1c09a2b4951207f8bf4eb855e45 Reviewed-on: https://chromium-review.googlesource.com/1180209 Commit-Queue: Anders Ruud <andruud@chromium.org> Reviewed-by: Rune Lillesveen <futhark@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Cr-Commit-Position: refs/heads/master@{#584576} [modify] https://crrev.com/bc90fa26ebfb929f3aa9652ee2cee23866815066/third_party/blink/renderer/core/css/css_custom_property_declaration.h [modify] https://crrev.com/bc90fa26ebfb929f3aa9652ee2cee23866815066/third_party/blink/renderer/core/css/property_registration.cc [modify] https://crrev.com/bc90fa26ebfb929f3aa9652ee2cee23866815066/third_party/blink/renderer/core/css/property_registration.h [modify] https://crrev.com/bc90fa26ebfb929f3aa9652ee2cee23866815066/third_party/blink/renderer/core/css/property_registry.h [modify] https://crrev.com/bc90fa26ebfb929f3aa9652ee2cee23866815066/third_party/blink/renderer/core/css/resolver/css_variable_resolver.cc [modify] https://crrev.com/bc90fa26ebfb929f3aa9652ee2cee23866815066/third_party/blink/renderer/core/css/resolver/css_variable_resolver.h [modify] https://crrev.com/bc90fa26ebfb929f3aa9652ee2cee23866815066/third_party/blink/renderer/core/css/resolver/css_variable_resolver_test.cc [modify] https://crrev.com/bc90fa26ebfb929f3aa9652ee2cee23866815066/third_party/blink/renderer/core/css/resolver/style_resolver.cc [modify] https://crrev.com/bc90fa26ebfb929f3aa9652ee2cee23866815066/third_party/blink/renderer/core/style/computed_style.h [modify] https://crrev.com/bc90fa26ebfb929f3aa9652ee2cee23866815066/third_party/blink/renderer/core/style/style_inherited_variables.cc [modify] https://crrev.com/bc90fa26ebfb929f3aa9652ee2cee23866815066/third_party/blink/renderer/core/style/style_inherited_variables.h [modify] https://crrev.com/bc90fa26ebfb929f3aa9652ee2cee23866815066/third_party/blink/renderer/core/style/style_non_inherited_variables.cc [modify] https://crrev.com/bc90fa26ebfb929f3aa9652ee2cee23866815066/third_party/blink/renderer/core/style/style_non_inherited_variables.h
,
Aug 21
,
Aug 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2d7c938e3f35a00f08e509434c74731e759409d6 commit 2d7c938e3f35a00f08e509434c74731e759409d6 Author: Anders Hartvoll Ruud <andruud@chromium.org> Date: Wed Aug 22 08:55:31 2018 Add performance tests for inherited custom properties. Inheriting a set of custom properties without modifying that set should be very cheap. At the time of writing, it is _not_ cheap: for every child element, we keep iterating though the set of inherited variables to check if they need to be resolved (i.e. have their var()-references replaced). There is a separate :root version of the test, because we have a special optimization for custom properties at :root (see StyleInheritedVariables), and I wish to track both :root-optimized performance and non-:root- optimized performance. R=futhark@chromium.org Bug: 875123 Change-Id: I25167713083b0fc0c70cad14a17f3992424526a6 Reviewed-on: https://chromium-review.googlesource.com/1181126 Reviewed-by: Rune Lillesveen <futhark@chromium.org> Reviewed-by: Hayato Ito <hayato@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Commit-Queue: Anders Ruud <andruud@chromium.org> Cr-Commit-Position: refs/heads/master@{#584977} [add] https://crrev.com/2d7c938e3f35a00f08e509434c74731e759409d6/third_party/blink/perf_tests/css/CustomPropertiesNonRootInheritance.html [add] https://crrev.com/2d7c938e3f35a00f08e509434c74731e759409d6/third_party/blink/perf_tests/css/CustomPropertiesRootInheritance.html |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by phanindra.mandapaka@chromium.org
, Aug 17