[gc] Make ComputedStyle & friends GarbageCollected. [chromium/src : main]
Michael Lippautz (Gerrit)
Attention is currently required from: Ian Kilpatrick.
1 comment:
File third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder.h:
Patch Set #44, Line 193:
HeapVectorHere and for all `HeapVector`: In case they are only used in hot paths temporarily, `.clear()` can eagerly free the backing store without need for GC. So if you know some hot paths that construct lots of vectors, you may be able to avoid some GC pressure.
To view, visit change 4346534. To unsubscribe, or for help writing mail filters, visit settings.
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Ian Kilpatrick.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1683730aa60000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Ian Kilpatrick.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/13457172a60000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Ian Kilpatrick.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14ec7f21a60000
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
Ian Kilpatrick would like Kentaro Hara, Rune Lillesveen, Anton Bikineev, Michael Lippautz and Anders Hartvoll Ruud to review this change.
[gc] Make ComputedStyle & friends GarbageCollected.
This patch moves ComputedStyle (and sub-objects) onto the gc-heap.
This comes with around a ~3% regression in MotionMark leaves
(presumably due to additional GC pressure). I'm able to reduce this
to ~1.8% by moving some "hot" fields into different objects.
There should be no functional behaviour change.
Change-Id: I604f6ec1e4c08dd4006bc1416e40cfd5594e5363
---
M third_party/blink/renderer/build/scripts/core/style/make_computed_style_base.py
M third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.cc.tmpl
M third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.h.tmpl
M third_party/blink/renderer/build/scripts/templates/fields/group.tmpl
M third_party/blink/renderer/core/animation/animation_utils.cc
M third_party/blink/renderer/core/animation/compositor_animations_test.cc
M third_party/blink/renderer/core/animation/css/css_animation_update.cc
M third_party/blink/renderer/core/animation/css/css_animation_update.h
M third_party/blink/renderer/core/animation/css/css_animations.cc
M third_party/blink/renderer/core/animation/css/css_animations.h
M third_party/blink/renderer/core/animation/css_length_interpolation_type.cc
M third_party/blink/renderer/core/animation/keyframe_effect_model_test.cc
M third_party/blink/renderer/core/animation/scroll_timeline_util_test.cc
M third_party/blink/renderer/core/animation/transition_keyframe.cc
M third_party/blink/renderer/core/css/container_query_evaluator_test.cc
M third_party/blink/renderer/core/css/css_math_expression_node_test.cc
M third_party/blink/renderer/core/css/css_properties.json5
M third_party/blink/renderer/core/css/font_face_set_document.cc
M third_party/blink/renderer/core/css/post_style_update_scope.cc
M third_party/blink/renderer/core/css/post_style_update_scope.h
M third_party/blink/renderer/core/css/properties/css_property_test.cc
M third_party/blink/renderer/core/css/properties/longhands/custom_property_test.cc
M third_party/blink/renderer/core/css/resolver/element_resolve_context.cc
M third_party/blink/renderer/core/css/resolver/element_resolve_context.h
M third_party/blink/renderer/core/css/resolver/element_style_resources.cc
M third_party/blink/renderer/core/css/resolver/font_builder_test.cc
M third_party/blink/renderer/core/css/resolver/matched_properties_cache.cc
M third_party/blink/renderer/core/css/resolver/matched_properties_cache.h
M third_party/blink/renderer/core/css/resolver/matched_properties_cache_test.cc
M third_party/blink/renderer/core/css/resolver/style_adjuster.cc
M third_party/blink/renderer/core/css/resolver/style_builder_test.cc
M third_party/blink/renderer/core/css/resolver/style_cascade_test.cc
M third_party/blink/renderer/core/css/resolver/style_resolver.cc
M third_party/blink/renderer/core/css/resolver/style_resolver.h
M third_party/blink/renderer/core/css/resolver/style_resolver_state.cc
M third_party/blink/renderer/core/css/resolver/style_resolver_state.h
M third_party/blink/renderer/core/css/resolver/style_resolver_test.cc
M third_party/blink/renderer/core/css/style_engine.cc
M third_party/blink/renderer/core/css/style_engine_test.cc
M third_party/blink/renderer/core/display_lock/display_lock_context.cc
M third_party/blink/renderer/core/display_lock/display_lock_context.h
M third_party/blink/renderer/core/dom/document.cc
M third_party/blink/renderer/core/dom/document.h
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/dom/first_letter_pseudo_element.cc
M third_party/blink/renderer/core/dom/first_letter_pseudo_element.h
M third_party/blink/renderer/core/dom/layout_tree_builder.cc
M third_party/blink/renderer/core/dom/layout_tree_builder.h
M third_party/blink/renderer/core/dom/node.cc
M third_party/blink/renderer/core/dom/node.h
M third_party/blink/renderer/core/dom/node_rare_data.cc
M third_party/blink/renderer/core/dom/node_rare_data.h
M third_party/blink/renderer/core/dom/pseudo_element.cc
M third_party/blink/renderer/core/dom/pseudo_element.h
M third_party/blink/renderer/core/dom/text.cc
M third_party/blink/renderer/core/highlight/highlight_style_utils.cc
M third_party/blink/renderer/core/highlight/highlight_style_utils.h
M third_party/blink/renderer/core/highlight/highlight_style_utils_test.cc
M third_party/blink/renderer/core/html/canvas/canvas_font_cache.cc
M third_party/blink/renderer/core/html/canvas/canvas_font_cache.h
M third_party/blink/renderer/core/html/forms/date_time_edit_element.cc
M third_party/blink/renderer/core/html/forms/date_time_edit_element.h
M third_party/blink/renderer/core/html/forms/internal_popup_menu.cc
M third_party/blink/renderer/core/html/forms/menu_list_inner_element.cc
M third_party/blink/renderer/core/html/forms/menu_list_inner_element.h
M third_party/blink/renderer/core/html/forms/select_type.cc
M third_party/blink/renderer/core/html/forms/text_control_inner_elements.cc
M third_party/blink/renderer/core/html/forms/text_control_inner_elements.h
M third_party/blink/renderer/core/html/html_embed_element_test.cc
M third_party/blink/renderer/core/html/html_html_element.cc
M third_party/blink/renderer/core/html/html_html_element.h
M third_party/blink/renderer/core/html/html_plugin_element.cc
M third_party/blink/renderer/core/html/html_plugin_element.h
M third_party/blink/renderer/core/html/shadow/meter_shadow_element_test.cc
M third_party/blink/renderer/core/html/shadow/progress_shadow_element_test.cc
M third_party/blink/renderer/core/inspector/inspector_dom_snapshot_agent.cc
M third_party/blink/renderer/core/inspector/inspector_dom_snapshot_agent.h
M third_party/blink/renderer/core/layout/custom_scrollbar.cc
M third_party/blink/renderer/core/layout/custom_scrollbar.h
M third_party/blink/renderer/core/layout/flexible_box_algorithm.cc
M third_party/blink/renderer/core/layout/flexible_box_algorithm.h
M third_party/blink/renderer/core/layout/layout_block.cc
M third_party/blink/renderer/core/layout/layout_block_flow.cc
M third_party/blink/renderer/core/layout/layout_block_flow.h
M third_party/blink/renderer/core/layout/layout_box.cc
M third_party/blink/renderer/core/layout/layout_box.h
M third_party/blink/renderer/core/layout/layout_object.cc
M third_party/blink/renderer/core/layout/layout_object.h
M third_party/blink/renderer/core/layout/layout_object_hot.cc
M third_party/blink/renderer/core/layout/layout_text.cc
M third_party/blink/renderer/core/layout/layout_text.h
M third_party/blink/renderer/core/layout/list_marker.cc
M third_party/blink/renderer/core/layout/ng/flex/ng_flex_layout_algorithm.cc
M third_party/blink/renderer/core/layout/ng/grid/ng_grid_line_resolver.h
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_box_state.h
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_break_token.cc
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_break_token.h
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder.cc
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder.h
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder_test.cc
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.cc
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.h
M third_party/blink/renderer/core/layout/ng/inline/ng_line_box_fragment_builder.h
M third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.cc
M third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.h
M third_party/blink/renderer/core/layout/ng/inline/ng_line_info.h
M third_party/blink/renderer/core/layout/ng/inline/ng_line_truncator.h
M third_party/blink/renderer/core/layout/ng/layout_ng_ruby_run.cc
M third_party/blink/renderer/core/layout/ng/ng_box_fragment_builder.h
M third_party/blink/renderer/core/layout/ng/ng_fragment_builder.h
M third_party/blink/renderer/core/layout/ng/ng_ink_overflow.cc
M third_party/blink/renderer/core/layout/ng/ng_layout_algorithm.h
M third_party/blink/renderer/core/layout/ng/ng_length_utils_test.cc
M third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.cc
M third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.h
M third_party/blink/renderer/core/layout/ng/ng_relative_utils_test.cc
M third_party/blink/renderer/core/layout/ng/table/layout_ng_table.cc
M third_party/blink/renderer/core/layout/ng/table/layout_ng_table.h
M third_party/blink/renderer/core/layout/ng/table/layout_ng_table_cell.cc
M third_party/blink/renderer/core/layout/ng/table/layout_ng_table_row.cc
M third_party/blink/renderer/core/layout/ng/table/layout_ng_table_section.cc
M third_party/blink/renderer/core/layout/ng/table/ng_table_borders.cc
M third_party/blink/renderer/core/layout/ng/table/ng_table_borders.h
M third_party/blink/renderer/core/layout/ng/table/ng_table_layout_algorithm.cc
M third_party/blink/renderer/core/layout/ng/table/ng_table_node.cc
M third_party/blink/renderer/core/layout/ng/table/ng_table_node.h
M third_party/blink/renderer/core/layout/overflow_model.h
M third_party/blink/renderer/core/layout/style_retain_scope.h
M third_party/blink/renderer/core/layout/style_retain_scope_test.cc
M third_party/blink/renderer/core/layout/text_autosizer.cc
M third_party/blink/renderer/core/page/print_context.cc
M third_party/blink/renderer/core/paint/ng/ng_decorating_box.h
M third_party/blink/renderer/core/paint/ng/ng_highlight_painter.cc
M third_party/blink/renderer/core/paint/ng/ng_highlight_painter.h
M third_party/blink/renderer/core/paint/ng/ng_inline_paint_context.h
M third_party/blink/renderer/core/paint/outline_painter_test.cc
M third_party/blink/renderer/core/paint/paint_layer.cc
M third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc
M third_party/blink/renderer/core/style/anchor_specifier_value.h
M third_party/blink/renderer/core/style/build.gni
M third_party/blink/renderer/core/style/computed_style.cc
M third_party/blink/renderer/core/style/computed_style.h
M third_party/blink/renderer/core/style/computed_style_diff_functions.json5
M third_party/blink/renderer/core/style/computed_style_extra_fields.json5
M third_party/blink/renderer/core/style/computed_style_test.cc
M third_party/blink/renderer/core/style/data_ref.h
M third_party/blink/renderer/core/style/fill_layer.cc
M third_party/blink/renderer/core/style/fill_layer.h
M third_party/blink/renderer/core/style/list_style_type_data.h
M third_party/blink/renderer/core/style/member_copy.h
M third_party/blink/renderer/core/style/style_base_data.cc
M third_party/blink/renderer/core/style/style_base_data.h
M third_party/blink/renderer/core/style/style_cached_data.h
D third_party/blink/renderer/core/style/style_filter_data.cc
D third_party/blink/renderer/core/style/style_filter_data.h
M third_party/blink/renderer/core/style/style_highlight_data.cc
M third_party/blink/renderer/core/style/style_highlight_data.h
M third_party/blink/renderer/core/svg/svg_element.cc
M third_party/blink/renderer/core/svg/svg_element.h
M third_party/blink/renderer/core/svg/svg_element_rare_data.cc
M third_party/blink/renderer/core/svg/svg_element_rare_data.h
M third_party/blink/renderer/core/view_transition/view_transition_pseudo_element_base.cc
M third_party/blink/renderer/core/view_transition/view_transition_pseudo_element_base.h
M third_party/blink/renderer/modules/animationworklet/worklet_animation_test.cc
M third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.cc
M third_party/blink/renderer/modules/csspaint/nativepaint/background_color_paint_definition_test.cc
M third_party/blink/renderer/modules/formatted_text/formatted_text.cc
M third_party/blink/renderer/modules/formatted_text/formatted_text_run.cc
169 files changed, 1,195 insertions(+), 1,408 deletions(-)
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
To view, visit change 4346534. To unsubscribe, or for help writing mail filters, visit settings.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Commit Message:
Patch Set #49, Line 11:
This comes with around a ~3% regression in MotionMark leavesWhy are we doing this, if all we get is a performance regression? Does it come with increased maintainability? Fewer bugs? Easier-to-reason-about code? Future possible optimizations?
If not, we should probably simply… not do it, I guess? There's no law saying everything has to gravitate towards Oilpan. The commit message definitely needs a “why” here :-)
I assume this is MotionMark on M1; how does it do on other platforms?
To view, visit change 4346534. To unsubscribe, or for help writing mail filters, visit settings.
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/10c0e757a60000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1119f463a60000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
😿 Job failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/134f489fa60000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
😿 Job failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/120ac15da60000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
😿 Job failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/120e8b5da60000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/140ffd8fa60000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1286eecfa60000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/10c9d653a60000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1434db4fa60000
Anders Hartvoll Ruud (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
Patch set 50:Code-Review +1
6 comments:
Commit Message:
Patch Set #49, Line 11:
This comes with around a ~3% regression in MotionMark leaves
Why are we doing this, if all we get is a performance regression? Does it come with increased mainta […]
Agree that this can read as "Do X -- X sucks, though", at least to those not fully indoctrinated in Oilpan. We should be able to write *something* positive in the message. If there's no specific reason, then we could minimally write "the regression is worth it due to <insert *generic* Oilpan reasons>".
Although from an e-mail thread elsewhere it looks like performance is more interesting than this CL message suggests currently.
---
My thinking:I do think it's important to be able to add GC'd stuff to ComputedStyle in the future without the looming risk of GC<->RefCounted cycles, and I expect this to become more important as the CSSWG invents new "creative" concepts. (See e.g. ScopedCSSName, which manually needs to avoid a cycle). Therfore it doesn't matter too much to me personally if we don't get enormous benefits from this NOW - it's more about long term health.
Patchset:
lgtm
We'll want to trace through to some more things in a follow up. (StyleVariables)
File third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.cc.tmpl:
Patch Set #50, Line 15:
struct SameSizeVerifierForComputedStyleBase {Because `SameSizeAsComputedStyle` also covers it, right?
File third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.h.tmpl:
Patch Set #50, Line 300:
blink::NodeSpaceIf I recall from previous patchsets, you tried a separate space as well, right? Didn't work out?
File third_party/blink/renderer/build/scripts/templates/fields/group.tmpl:
{% if field.wrapper_pointer_name == 'Member' %}
visitor->Trace({{field.name}});Can TraceIfNeeded not handle this case?
File third_party/blink/renderer/core/style/computed_style_extra_fields.json5:
default_value: "StyleHighlightData()",
field_group: "inherited->highlight-data",Ugh, this generates `class StyleHighlightDataData` which contains a `StyleHighlightData`. Probably the members of `StyleHighlightData` be listed as fields directly in this json5, but that's another CL.
To view, visit change 4346534. To unsubscribe, or for help writing mail filters, visit settings.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
Here is the style recalc test on an x86 machine (Alder Lake). Some of the subtests are regressing heavily:
Initial style (µs) Before After Perf 95% CI (BCa)
=================== ========= ========= ======= =================
ECommerce 4135 4088 +1.1% [ +0.2%, +2.0%]
Encyclopedia 35686 36942 -3.4% [ -4.4%, -2.6%]
Extension 51657 50307 +2.7% [ +2.1%, +3.3%]
News 18281 17908 +2.1% [ +1.2%, +3.0%]
Search 5854 5764 +1.6% [ +0.5%, +2.4%]
Social1 11791 11576 +1.9% [ +0.8%, +2.8%]
Social2 7232 6925 +4.4% [ +3.5%, +5.3%]
Sports 22001 22460 -2.0% [ -3.0%, -1.1%]
Video 16605 16835 -1.4% [ -2.2%, -0.4%]
Geometric mean +0.7% [ +0.1%, +1.2%]Recalc style (µs) Before After Perf 95% CI (BCa)
=================== ========= ========= ======= =================
ECommerce 4555 4551 +0.1% [ -1.5%, +0.8%]
Encyclopedia 24182 25643 -5.7% [ -6.7%, -4.9%]
Extension 45248 45050 +0.4% [ -0.1%, +0.9%]
News 11950 15777 -24.3% [-25.0%, -23.5%]
Search 2515 2494 +0.8% [ -0.2%, +1.7%]
Social1 6970 6943 +0.4% [ -0.4%, +1.3%]
Social2 5401 5376 +0.5% [ -0.5%, +4.3%]
Sports 8867 9825 -9.8% [-10.4%, -8.9%]
Video 9168 9792 -6.4% [ -7.3%, -5.6%]
Geometric mean -5.2% [ -5.7%, -4.9%]I'll run the same test on M1 shortly. I can also test the Alexa1000 set if you'd like, although it only covers front pages and takes a fair bit longer to run.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
Here is the style recalc test on an x86 machine (Alder Lake). […]
M1 is much happier; there's still a strong regression on the News test, but the others are uniformly and clearly positive:
Initial style (µs) Before After Perf 95% CI (BCa)
=================== ========= ========= ======= =================
ECommerce 5009 4672 +7.2% [ +6.8%, +7.7%]
Encyclopedia 45582 45020 +1.2% [ +0.7%, +1.6%]
Extension 69149 64955 +6.5% [ +5.9%, +6.9%]
News 21893 20835 +5.1% [ +4.6%, +5.5%]
Search 6534 6203 +5.3% [ +4.1%, +6.2%]
Social1 13963 13323 +4.8% [ +4.3%, +5.3%]
Social2 11738 11066 +6.1% [ +5.5%, +7.0%]
Sports 24640 24123 +2.1% [ +1.3%, +2.7%]
Video 17636 17362 +1.6% [ +1.0%, +2.2%]
Geometric mean +4.4% [ +4.1%, +4.7%]
Recalc style (µs) Before After Perf 95% CI (BCa)
=================== ========= ========= ======= =================
ECommerce 6523 6207 +5.1% [ +4.7%, +5.5%]
Encyclopedia 38616 37202 +3.8% [ +3.1%, +4.3%]
Extension 65591 62038 +5.7% [ +5.3%, +6.2%]
News 15369 16793 -8.5% [ -9.0%, -7.8%]
Search 3386 3150 +7.5% [ +6.3%, +8.6%]
Social1 9005 8542 +5.4% [ +4.9%, +6.2%]
Social2 7396 6963 +6.2% [ +5.4%, +6.7%]
Sports 14206 14073 +0.9% [ -0.1%, +1.5%]
Video 11807 11643 +1.4% [ +0.9%, +2.1%]
Geometric mean +3.0% [ +2.7%, +3.2%]Still, a 24% regression on one test on x86 is far outside what I've ever seen. (These are real-world pages, FWIW, while MotionMark is a fairly odd synthetic test that doesn't correlate well with the loads we are seeing in the wild)
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
M1 is much happier; there's still a strong regression on the News test, but the others are uniformly […]
On a quick profile check of what's going on with x86; it seems there's code size bloat, which prevents a lot of inlining (it doesn't seem like the icache is the problem, just the sheer number of extra barriers and checks that need to be made when everything is on Oilpan). Then add extra GC costs on top of that, and we get into problems. Note that this is a preliminary analysis, so it doesn't come with guarantees that it's correct :-) But inlining decisions certainly have changed, and we certainly have more code now. On the positive side, refcount management cost goes almost to zero, of course.
It's not too surprising that MotionMark doesn't see this, because there are large parts of style that it hardly touches at all.
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anton Bikineev, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
On a quick profile check of what's going on with x86; it seems there's code size bloat, which preven […]
Thanks for that. I'll try running the benchmark today, and try a couple of things.
Do I just run:
third_party/blink/renderer/core/css/scripts/style_perftest_snap_page ?
(is there a 1-pager somewhere for how to use?)I'll try two things.
1) I'm wondering if the large drop on "News" is due to a GC running. I'll try adding a DisallowGarbageCollectionScope just to test this thesis. We might be taking a earlier GC now.
2) Can add ALWAYS_INLINE to the getters + a few other things to see if that makes a difference. Suspect that this might be mitigated with a PGO build eventually, but easy to check.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
Thanks for that. I'll try running the benchmark today, and try a couple of things. […]
There's no one-pager, but there's a 15-pager at: https://docs.google.com/document/d/1zQcRcy35iu2J2CR3meiwzhoAgt2Eug7wtFh8RPex2rs/edit#heading=h.e3g1d4rt2oda
The oneliner, however, is simple:
./out/Release/blink_perf_tests --gtest_filter=StyleCalcPerfTest.\*
style_perftest_snap_page is to create new test sets (i.e., you have some live page that you want to snapshot, so that you can benchmark it using the style perftest); you don't need it in your case.
It's entirely possible that the News test is being hit by a GC, yes; but if so, that's indicative of an actual problem, not just a test artifact. We do a full forced GC before each subtest, so if it suddenly starts doing a major GC in the middle of a style recalc, that means it's created so much garbage that it can't survive without a mid-frame GC even in the optimal case. (Note that for the x86 run, I've pinned to one core for reproducibility, especially since Alder Lake has both large and small cores, so you won't get real background GC if that's what it's attempting; it will use the same core. Of course, background GC still consumes power. I didn't do this for M1 since I have no idea how to do pinning under macOS, and this may explain some of the difference.)
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
Ian Kilpatrick uploaded patch set #51 to this change.
[gc] Make ComputedStyle & friends GarbageCollected.
This patch moves ComputedStyle (and sub-objects) onto the gc-heap.
The performance profile of this patch is "complex". :).
For the motionmark-benchmark (non-PGO as so many signatures change due
to this patch) we roughly have:
+-------------+----------------+----------------+---------------+
| MotionMark | M1[1][2] | Win10[3][4] | Pixel4[5][6] |
+-------------+----------------+----------------+---------------+
| Leaves | -1.2%, -2.2% | NA, -1.1% | +4.3%, +3.6% |
| Multiply | | +1.2%, +1.6% | +3.4%, +3.7% |
| Design | +6.3%, +6.0% | +4.4%, +5.8% | |
| Paths | | -1.0%, NA | |
| Suites | -1.1%, NA | | |
+-------------+----------------+----------------+---------------+
[1] https://pinpoint-dot-chromeperf.appspot.com/job/14ec7f21a60000
[2] https://pinpoint-dot-chromeperf.appspot.com/job/13457172a60000
[3] https://pinpoint-dot-chromeperf.appspot.com/job/1286eecfa60000
[4] https://pinpoint-dot-chromeperf.appspot.com/job/140ffd8fa60000
[5] https://pinpoint-dot-chromeperf.appspot.com/job/1434db4fa60000
[6] https://pinpoint-dot-chromeperf.appspot.com/job/10c9d653a60000
This is (broadly speaking) a net-win. (E.g. M1 ~+0.5%).
There is no significant difference on Speedometer.
https://pinpoint-dot-chromeperf.appspot.com/job/10c0e757a60000
To view, visit change 4346534. To unsubscribe, or for help writing mail filters, visit settings.
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
Ian Kilpatrick uploaded patch set #52 to this change.
[gc] Make ComputedStyle & friends GarbageCollected.
This patch moves ComputedStyle (and sub-objects) onto the gc-heap for
security reasons.
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
8 comments:
Commit Message:
Patch Set #49, Line 11:
This comes with around a ~3% regression in MotionMark leaves
Agree that this can read as "Do X -- X sucks, though", at least to those not fully indoctrinated in […]
Yeah this initial commit message was written a while ago (should have re-reviewed it one I sent it out initially).
Made it slightly more rosy. There likely will be progressions+regressions with this patch.
Patchset:
Yup got the styleperf_test to work.
It's entirely possible that the News test is being hit by a GC, yes;
Yeah its a GC. Adding either:
```
--- a/third_party/blink/renderer/core/css/style_perftest.cc
+++ b/third_party/blink/renderer/core/css/style_perftest.cc
@@ -195,8 +195,10 @@ static StylePerfResult MeasureStyleForDumpedPage(
page->GetDocument().GetStyleEngine().MarkAllElementsForStyleRecalc(
StyleChangeReasonForTracing::Create("test"));
+ // ThreadState::Current()->CollectAllGarbageForTesting();
{
+ // cppgc::subtle::NoGarbageCollectionScope scope(ThreadState::Current()->heap_handle());
base::ElapsedTimer style_timer;
page->GetDocument().UpdateStyleAndLayoutTreeForThisDocument();
result.recalc_style_time = style_timer.Elapsed();
```Mitigates the difference (at least on my M1 Mac - currently don't have a linux box setup).
@mlippautz - What are the rough heuristists for when a GC triggers?
We do a full forced GC before each subtest, so if it suddenly starts doing a major GC in the middle of a style recalc, that means it's created so much garbage that it can't survive without a mid-frame GC even in the optimal case.
So now we get into how realistic this benchmark is :). E.g. this is (I think) invalidating all the styles, and then re-creating them all. Which seems rare on a typical page? (E.g. more likley to invalidate a subset).
@mlippautz/haraken might have thoughts here.
As an aside I've got a prototype which should substantially reduce the size of ComputedStyle on most pages (in my testing ComputedStyle is relatively large, e.g. averages 600kB for one instance on typical pages).
Its not entirely ready yet, but I might try and rebase it onto this change. (Part of this was GC'ing computed style makes the destructors trivial which helps a bunch).
Note that for the x86 run, I've pinned to one core for reproducibility, especially since Alder Lake has both large and small cores, so you won't get real background GC if that's what it's attempting; it will use the same core. Of course, background GC still consumes power. I didn't do this for M1 since I have no idea how to do pinning under macOS, and this may explain some of the difference.)
My information might be out of date, but from my rough understanding (at least on Android type devices) an additional core running doesn't double the power consumption, its a far smaller multiplier. So pinning to a single core isn't a direct 1:1 comparison here. Again - information might be out of date, but battery usage much more dependent on how often the core(s) go quiet, vs. how many cores are active.
Are you able to pin to just the P-cores or E-cores on AlderLake?
Patchset:
@mlip...@chromium.org - I can move StyleInheritedData over in this patch if it'd help.
File third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.cc.tmpl:
Patch Set #50, Line 15:
struct SameSizeVerifierForComputedStyleBase {Because `SameSizeAsComputedStyle` also covers it, right?
Thats right. This seemed superfluous to me.
File third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.h.tmpl:
Patch Set #50, Line 300:
blink::NodeSpaceIf I recall from previous patchsets, you tried a separate space as well, right? Didn't work out?
Yeah - the options here are:
Default, NodeSpace, LayoutObjectSpace, and a new ComputedStyleSpace (if we create one).NodeSpace from my testing seemed the best, but was planning on submitting as this initially, then following up with tests for what the other spaces do.
(So many moving pieces in this patch difficult to decide on this).
File third_party/blink/renderer/build/scripts/templates/fields/group.tmpl:
{% if field.wrapper_pointer_name == 'Member' %}
visitor->Trace({{field.name}});Can TraceIfNeeded not handle this case?
It can - would need to be:
```
{% if field.wrapper_pointer_name %}
TraceIfNeeded<{{field.wrapper_type_name}}<{{field.type_name}}>>::Trace(visitor, {{field.name}});
{% elif not field.size %}
TraceIfNeeded<{{field.type_name}}>::Trace(visitor, {{field.name}});
{% endif %}
```Up to you if you think above is easier to read.
E.g. declare_storage is fun:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/build/scripts/templates/fields/field.tmpl;l=135-143?q=wrapper_pointer_name&ss=chromium
File third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder.h:
Patch Set #44, Line 193:
HeapVector
Here and for all `HeapVector`: In case they are only used in hot paths temporarily, `. […]
Thanks - unfortunately none of the vectors in this patch are particularly hot. (This one in particular ends up as empty (no need to clear).
File third_party/blink/renderer/core/style/computed_style_extra_fields.json5:
default_value: "StyleHighlightData()",
field_group: "inherited->highlight-data",
Ugh, this generates `class StyleHighlightDataData` which contains a `StyleHighlightData`. […]
Yeah that's right. The field_group is needed for the access-bit on the builder. The highlight-data had non-trivial usage.
... 2 hours later.
Let me just send a patch to remove this. Its really not needed.
To view, visit change 4346534. To unsubscribe, or for help writing mail filters, visit settings.
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
lgtm […]
May need to move over in this patch anyway given: https://bugs.chromium.org/p/chromium/issues/detail?id=1413955#c20
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
So now we get into how realistic this benchmark is :). E.g. this is (I think) invalidating all the styles, and then re-creating them all. Which seems rare on a typical page? (E.g. more likley to invalidate a subset).
If nothing else, it's a million times more realistic than MotionMark is :-) And yes, it's essentially doing two full calcs instead of one, but that's really just a difference of degree, not a huge qualitative difference. (We try to avoid full recalcs, but they certainly happen; especially in some cases around insertion of new stylesheets, we currently get invalidation of *.*. We might be looking at some of those cases in Q3, but we will never get entirely rid of full recalcs; e.g., when someone resizes the browser, there are cases we can't really avoid them.)
In any case, even if I run with e.g. --style-recalc-iterations=100 (which means you don't fall off one specific GC cliff, just look at the overall cost of GC), it's much slower. Even something like 5% overall is something we have to spend significant engineering resources to claw back.
Are you able to pin to just the P-cores or E-cores on AlderLake?
Yes, we can pin to a subset of the cores, though at the cost of higher variance (some cores have higher max boost frequency than others, so test result will vary with which core you happen to be scheduled on). I can make that test at some point.
TBH I think we can live with some GC cost; my assumption is that the V8 team will continue to invest in Oilpan (please correct me if I'm wrong there) and eventually end up with less invasive GCs. I'm more worried for how it affects the rest of the code wrt. size and speed; e.g., at some point, we found that something as innocent as sorting pointers ended up being hit because Member<> needs GC barriers inserted on every std::swap, and similarly, I see pointer decompression taking its toll nearly _everywhere_ in our code. This is a thin film of perf overhead that lies across the entire code, and not restricted to a single hotspot; that's the kind of cost I don't think will go away by itself or be easy to mitigate, so that's the kind I'd like to understand. E.g., questions like: Is the code size increase (and apparent perf hit) here due to more or less inlining?
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
In any case, even if I run with e.g. --style-recalc-iterations=100 (which means you don't fall off one specific GC cliff, just look at the overall cost of GC), it's much slower. Even something like 5% overall is something we have to spend significant engineering resources to claw back.
How did you test this? A simple run on M1 yielded:
~1500ms (GC) vs. ~1550ms (ToT).
I'm more worried for how it affects the rest of the code wrt. size and speed; e.g., at some point, we found that something as innocent as sorting pointers ended up being hit because Member<> needs GC barriers inserted on every std::swap, and similarly, I see pointer decompression taking its toll nearly everywhere in our code.
So... - for pointer-compression in this patch I made extensive use of UncompressedMember to mitigate some of the perf/binary size issues. (This was a real perf issue for motionmark at least).
Is there a place that I missed that is causing the binary/instruction bloat? (I'm a little confused b/c as is this reduces the Android Binary Size quite a bit, likely due to no more refs/deref).
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
How did you test this? A simple run on M1 yielded:
It was on Alder Lake. I didn't test M1 because building on macOS is so painfully slow (I only really do it when I want to double-check something, or when we are specifically chasing MotionMark on M1).
Is there a place that I missed that is causing the binary/instruction bloat?
I haven't had time to do anything like a full investigation. I don't know of any specific single places; my assumption is that it's spread out over the entire binary, and that it's not only about pointer compression.
(I'm a little confused b/c as is this reduces the Android Binary Size quite a bit, likely due to no more refs/deref).
This was again measured on x86 (well, x86-64); official build but without PGO:
bigscreen:~/chromium/src> size out/Release/blink_perf_tests* text data bss dec hex filename
98058379 3392220 2134904 103585503 62c96df out/Release/blink_perf_tests
97941931 3392220 2134888 103469039 62acfef out/Release/blink_perf_tests_oldThat's 116448 bytes more code (.text segment). I would assume Windows is the same, but I haven't tested. The content_shell target is similar:
bigscreen:~/chromium/src> size out/Release/content_shell*
text data bss dec hex filename
162618313 5872084 2245808 170736205 a2d3a4d out/Release/content_shell
162502697 5872084 2245936 170620717 a2b772d out/Release/content_shell_old115616 bytes more. I'd assume the full chrome target shows roughly the same.
Android is built with optimize-for-size (-Os, though with some .cc files manually marked “hot” and with different flags), which is very different from the optimize-for-speed-but-balanced on desktop (-O2 plus some flags to limit the cross-module inliner, AFAIK), so it's no wonder they get different results. That isn't to say that desktop won't get hurt when you try to cram too much code into the instruction caches, though (and that's the kind of effect you won't notice in narrow benchmarks; we'd need to look at actual FCP, which I guess is hard).
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
> How did you test this? A simple run on M1 yielded: […]
I suspect the binary size increase is in part due to all the additional Trace methods needed. I believe at least that an the UncompressedMembers shouldn't really create that much additional binary size for this case (only for assign-like operations).
Anders Hartvoll Ruud (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
Patch set 52:Code-Review +1
3 comments:
Patchset:
May need to move over in this patch anyway given: https://bugs.chromium. […]
Acknowledged
File third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.h.tmpl:
Patch Set #50, Line 300:
blink::NodeSpace
Yeah - the options here are: […]
Acknowledged
File third_party/blink/renderer/build/scripts/templates/fields/group.tmpl:
{% if field.wrapper_pointer_name == 'Member' %}
visitor->Trace({{field.name}});
It can - would need to be: […]
Ah, right. That's not really better. :-)
Kentaro Hara (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
Patch set 52:Code-Review +1
12 comments:
Patchset:
I scanned all the files -- from the oilpan implementation perspective, LGTM!
I haven't checked all destructors. Please double-check that there is no destructor that is touching the converted Members.
File third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.h.tmpl:
Patch Set #52, Line 300:
using Space = blink::NodeSpace;I think we want to use blink::LayoutObjectSpace since ComputedStyle is coupled with LayoutObjects.
(A separate question: Can we merge these spaces into one?)
File third_party/blink/renderer/core/css/resolver/style_resolver.h:
Patch Set #52, Line 335:
subtle::UncompressedMember<const ComputedStyle> initial_style_for_img_;Are these two pointers performance-sensitive enough to use UncompressedMember?
File third_party/blink/renderer/core/dom/node_rare_data.h:
Patch Set #52, Line 132:
subtle::UncompressedMember<const ComputedStyle> computed_style_;This increases the size of NodeRareData. Is it really worth using UncompressedMember?
c.f., we are using Member for layout_object_.
File third_party/blink/renderer/core/dom/node_rare_data.cc:
Patch Set #52, Line 49:
Member<void*> member_[4];Maybe you need to change this expectation? I'm wondering why you didn't hit the ASSERT_SIZE below.
File third_party/blink/renderer/core/layout/layout_object.h:
Patch Set #52, Line 4381:
subtle::UncompressedMember<const ComputedStyle> style_;If performance matters for this pointer, maybe we should consider using UncompressedMember for other fields (e.g., parent_, previous_, next_). A follow-up CL is fine.
File third_party/blink/renderer/core/layout/ng/grid/ng_grid_line_resolver.h:
Patch Set #52, Line 134:
Persistent<const ComputedStyle> style_;I'm afraid this will create a reference cycle. Can we use a Member?
File third_party/blink/renderer/core/paint/paint_layer.cc:
Patch Set #52, Line 2199:
old_style ? old_style->Filter() != new_style.Filter() ||Is this an intentional change?
Patch Set #52, Line 2219:
old_style ? old_style->BackdropFilter() != new_style.BackdropFilter()Ditto.
File third_party/blink/renderer/core/style/anchor_specifier_value.h:
Patch Set #52, Line 51:
CORE_EXPORT void Trace(Visitor*) const;Is this really needed?
File third_party/blink/renderer/core/style/computed_style.h:
Patch Set #39, Line 329:
if (freelist_ != nullptr) {The freelist optimization seems to be where one part of the regression on MotionMark has crept in.
I'm happy this hack is gone :)
File third_party/blink/renderer/core/style/list_style_type_data.h:
Patch Set #52, Line 24:
CORE_EXPORT void Trace(Visitor*) const;I'm not sure if we need this.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
I suspect the binary size increase is in part due to all the additional Trace methods needed. […]
On a quick check with nm, it seems there is a net increase of 115 ::Trace( functions in the binary. Do we believe they are averaging a kilobyte a piece? nm --size is confused due to function cloning, but I sampled a few of the new ones for disassembly, and they were more like 5–100 bytes each.
Michael Lippautz (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Rune Lillesveen.
2 comments:
File third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.h.tmpl:
Patch Set #52, Line 190:
void TraceAfterDispatch(Visitor* visitor) const {Why do we need a `TraceAfterDispatch()`? That's generally only used for switching types in `Trace()` when class save themselves a vtable pointer and use existing bits to construct a type switch.
Patch Set #52, Line 300:
using Space = blink::NodeSpace;
I think we want to use blink::LayoutObjectSpace since ComputedStyle is coupled with LayoutObjects. […]
The better objects are clustered, the better the utilization of free space. When in doubt, we could just leave the default space and try afterwards.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Rune Lillesveen.
1 comment:
Patchset:
On a quick check with nm, it seems there is a net increase of 115 ::Trace( functions in the binary. […]
As a sanity check: It cannot all be about GC. I took my Zen 3 at home (so no big/little shenanigans, 16 cores all available for the test), disabled GC entirely on both sides (using NoGarbageCollectionScope; so this should be a grossly unfair test to the patch's advantage) and the Video test still got slower on recalc:
Recalc style (µs) Before After Perf 95% CI (BCa)
=================== ========= ========= ======= =================
Video 16881 17329 -2.6% [ -3.1%, -2.1%]
Curiously, I don't think it's 100% successful in stopping the concurrent mark thread from running, if I am to believe the profiler. But again, there should be plenty of spare cores to run it on.
I ran on an Intel machine again, where I have access to Processor Trace (which is much more precise for short runs than anything else out there; this happens to be a Kaby Lake), and after some manual diffing, these are the most interesting differences from the Video subtest (one calc + recalc, restricted to code run during style only, main thread only):
Before After
5.23% 5.81% +0.58% void blink::ElementRuleCollector::CollectMatchingRulesForListInternal<false>(base::span<blink::RuleData const, 18446744073709551615ul>, blink::MatchRequest const&, blink::RuleSet const*, int, blink::SelectorChecker const&, blink::ElementRuleCollector::PartRequest*)
2.71% 2.84% +0.13% blink::ElementRuleCollector::CollectMatchingRules(blink::MatchRequest const&)
0.71% 1.50% +0.79% cppgc::internal::WriteBarrier::DijkstraMarkingBarrierSlow(void const*)
0.48% 1.05% +0.57% cppgc::internal::BasePage::heap() const
0.79% 0.07% -0.72% base::subtle::RefCountedBase::ReleaseImpl() const
0.17% 0.76% +0.59% cppgc::internal::MakeGarbageCollectedTraitInternal::Allocate(cppgc::AllocationHandle&, unsigned long, unsigned short, cppgc::CustomSpaceIndex)
0.81% 0.71% -0.10% partition_alloc::PartitionRoot::Free(void*)
0.39% 0.10% -0.29% malloc.cfi
1.46% 0.34% -1.12% base::subtle::RefCountedBase::AddRefImpl() const
0.64% 0.29% -0.35% allocator_shim::internal::PartitionMalloc(allocator_shim::AllocatorDispatch const*, unsigned long, void*) [clone .cfi]
0.25% 0.08% -0.17% partition_alloc::ThreadCache::FillBucket(unsigned long)
0.00% 0.22% +0.22% void cppgc::internal::WriteBarrier::CombinedWriteBarrierSlow<(cppgc::internal::WriteBarrierSlotType)1>(void const*)
Note in particular:- I don't know why CollectMatchingRulesForListInternal would get more expensive; I don't think it's an inlining effect, since the function is the same size on both sides.
- DijkstraMarkingBarrierSlow() is significant. Same with CombinedWriteBarrierSlow().
- I don't know what cppgc::internal::BasePage::heap() does, but it seems to be a relevant factor.
- Oilpan allocation appears to be more expensive than PA allocation. And notably, we hardly save any time at all in Free().
- Refcounting cost goes way down, of course.
So overall, it seems that even if we take out GC costs entirely, Oilpan overhead > PartitionAlloc overhead? But that for most of the tests, I guess the refcounting overhead cancels that out.
Michael Lippautz (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
As a sanity check: It cannot all be about GC. I took my Zen 3 at home (so no big/little shenanigans, 16 cores all available for the test), disabled GC entirely on both sides (using NoGarbageCollectionScope; so this should be a grossly unfair test to the patch's advantage) and the Video test still got slower on recalc:
Recalc style (µs) Before After Perf 95% CI (BCa)
=================== ========= ========= ======= =================
Video 16881 17329 -2.6% [ -3.1%, -2.1%]Curiously, I don't think it's 100% successful in stopping the concurrent mark thread from running, if I am to believe the profiler. But again, there should be plenty of spare cores to run it on.
I ran on an Intel machine again, where I have access to Processor Trace (which is much more precise for short runs than anything else out there; this happens to be a Kaby Lake), and after some manual diffing, these are the most interesting differences from the Video subtest (one calc + recalc, restricted to code run during style only, main thread only):
Before After
5.23% 5.81% +0.58% void blink::ElementRuleCollector::CollectMatchingRulesForListInternal<false>(base::span<blink::RuleData const, 18446744073709551615ul>, blink::MatchRequest const&, blink::RuleSet const*, int, blink::SelectorChecker const&, blink::ElementRuleCollector::PartRequest*)
2.71% 2.84% +0.13% blink::ElementRuleCollector::CollectMatchingRules(blink::MatchRequest const&)
0.71% 1.50% +0.79% cppgc::internal::WriteBarrier::DijkstraMarkingBarrierSlow(void const*)
0.48% 1.05% +0.57% cppgc::internal::BasePage::heap() const
0.79% 0.07% -0.72% base::subtle::RefCountedBase::ReleaseImpl() const
0.17% 0.76% +0.59% cppgc::internal::MakeGarbageCollectedTraitInternal::Allocate(cppgc::AllocationHandle&, unsigned long, unsigned short, cppgc::CustomSpaceIndex)
0.81% 0.71% -0.10% partition_alloc::PartitionRoot::Free(void*)
0.39% 0.10% -0.29% malloc.cfi
1.46% 0.34% -1.12% base::subtle::RefCountedBase::AddRefImpl() const
0.64% 0.29% -0.35% allocator_shim::internal::PartitionMalloc(allocator_shim::AllocatorDispatch const*, unsigned long, void*) [clone .cfi]
0.25% 0.08% -0.17% partition_alloc::ThreadCache::FillBucket(unsigned long)
0.00% 0.22% +0.22% void cppgc::internal::WriteBarrier::CombinedWriteBarrierSlow<(cppgc::internal::WriteBarrierSlotType)1>(void const*)
Note in particular:- I don't know why CollectMatchingRulesForListInternal would get more expensive; I don't think it's an inlining effect, since the function is the same size on both sides.
- DijkstraMarkingBarrierSlow() is significant. Same with CombinedWriteBarrierSlow().
That's the write barrier that PA doesn't need to pay for: Can you figure out where it mostly comes from? If there's a hot site, maybe we can improve it.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
> As a sanity check: It cannot all be about GC. […]
You are straining my understanding of the profiler, but I will give it a shot :-)
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
You are straining my understanding of the profiler, but I will give it a shot :-)
I've tried looking at this. Best I can tell, this mostly happens when creating ComputedStyle in various forms. For instance, here isthe entire subset of callers of one part of the tree (I've removed a lot of template spew to make it more readable):
- 0,97% CombinedWriteBarrierSlow<(WriteBarrierSlotType)1> (inlined)
- 0,73% DijkstraWriteBarrierPolicy::AssigningBarrier<(WriteBarrierSlotType)1> (inlined)
+ 0,13% BasicMember<ComputedStyleBase::StyleBoxData>::AssigningWriteBarrier (inlined)
+ 0,11% BasicMember<ComputedStyleBase::StyleBackgroundData>::AssigningWriteBarrier (inlined)
+ 0,09% BasicMember<ComputedStyleBase::StyleRareNonInheritedUsageLessThan14PercentData>::AssigningWriteBarrier (inlined)
+ 0,07% BasicMember<ComputedStyleBase::StyleSurroundData>::AssigningWriteBarrier (inlined)
+ 0,06% BasicMember<ComputedStyleBase::StyleInheritedData>::AssigningWriteBarrier (inlined)
+ 0,06% BasicMember<ComputedStyle const>::AssigningWriteBarrier (inlined)
+ 0,05% BasicMember<ComputedStyleBase::StyleVisualData>::AssigningWriteBarrier (inlined)
+ 0,04% BasicMember<ComputedStyleBase::StyleRareInheritedUsageLessThan64PercentData>::AssigningWriteBarrier (inlined)
+ 0,03% BasicMember<ComputedStyleBase::StyleFontData>::AssigningWriteBarrier (inlined)
+ 0,02% BasicMember<ComputedStyleBase::StyleGeometryData>::AssigningWriteBarrier (inlined)
+ 0,02% BasicMember<ComputedStyleBase::StyleStopData>::AssigningWriteBarrier (inlined)
+ 0,01% BasicMember<ComputedStyleBase::StyleFillData>::AssigningWriteBarrier (inlined)
+ 0,01% BasicMember<ComputedStyleBase::StyleMiscData>::AssigningWriteBarrier (inlined)So that's all ComputedStyle subobjects, perhaps unsurprisingly. (The fact that this specific test uses StyleBoxData more than others doesn't mean much for other tests, I'd assume.) Looking at each of the subobject types seems to indicate that most of them originate either in StyleResolver::ApplyMatchedCache() or StyleResolver::InitStyleAndApplyInheritance(). Not so strange, perhaps, since these are the two main places we get ComputedStyle objects from in the first place?
The other part of the tree is WriteBarrierSlotType enum 0 (compressed pointers). It looks like this:
- 1,14% CombinedWriteBarrierSlow<(WriteBarrierSlotType)0> (inlined)
- 0,80% DijkstraWriteBarrierPolicy::AssigningBarrier<(WriteBarrierSlotType)0> (inlined)
+ 0,46% BasicMember<StyleRule>::AssigningWriteBarrier (inlined)
+ 0,11% BasicMember<NodeData>::AssigningWriteBarrier (inlined)
+ 0,07% BasicMember<ComputedStyle const>::AssigningWriteBarrier (inlined) + 0,05% BasicMember<PaintLayer>::AssigningWriteBarrier (inlined)
+ 0,03% BasicMember<CSSPropertyValueSet>::AssigningWriteBarrier (inlined)
+ 0,02% BasicMember<ElementData>::AssigningWriteBarrier (inlined)
+ 0,02% BasicMember<LayoutObject>::AssigningWriteBarrier (inlined)
+ 0,01% BasicMember<CachedMatchedProperties>::AssigningWriteBarrier (inlined)
+ 0,01% BasicMember<PaintLayerScrollableArea>::AssigningWriteBarrier (inlined)
Here we have more of a long tail. I believe that most of these were already there before your patch, so I haven't looked much into them, but e.g. StyleRule comes from RuleMap::Compact().I don't know if you can do anything useful with this, but at least it's the data as far as I've been able to find out.
Michael Lippautz (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
I've tried looking at this. Best I can tell, this mostly happens when creating ComputedStyle in various forms. For instance, here isthe entire subset of callers of one part of the tree (I've removed a lot of template spew to make it more readable):
Let me hook in here as you probably know best if this applies: We don't emit write barriers on initializing stores. In other words, constructing a Member and setting its value in the initializer list is cheap. Assigning it later requires the write barrier.
```
struct Foo : GCed<Foo> {
Member<Bar> bar_;
Foo(Bar* bar) : bar_(bar) { // no write barrier
bar_ = bar; // write barrier
}
}
```
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
> I've tried looking at this. […]
I don't know how this code works, but I think Ian does, because he was the one that introduced this builder pattern :-) Together with @and...@chromium.org.
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anton Bikineev, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
I don't know how this code works, but I think Ian does, because he was the one that introduced this […]
Yeah this is part of the complex memory structure of computed style.
ComputedStyle shares a bunch of its internal slots with each other to reduce memory consumption.
It's effectively:```
DataRef<T> {
T& Access(bool& access_flag) {
if (!access_flag)
data_ = data_->Copy();
return data_;
}
Member<T> data_;
}ComputedStyle {
DataRef<GroupA> group_a_;
DataRef<GroupB> group_b_;
ComputedStyle(const ComputedStyle& other); // 99.9% of the some styles are created by invoking the copy constructor from an "initial" style.
}
```Then we get to setting the members on ComputedStyle which looks like:
```
ComputedStyleBuilder::SetSomething(int something) {
if (style_->group_a_->something_ != something)
style_->group_a_.Access(group_a_access_flag_).something_ = something;
}
```Above - "GroupB" will still be shared with the "initial" style, but "GroupA" wont.
ComputedStyle at the moment isn't optimal FWIW. In local testing an average ComputedStyle is ~600B. We can reduce this a little bit by repacking the groups but in reality is should be closer to ~100B on average.
Groups can be nested within each other which is likely causing part of the issue at the moment.
E.g. can have:```
SetSomething(int something) {
if ()
style_->group_1_.Access(flag1)->group_1a.Access(flag1a)->group_1aa.Access(flag1aa).something_ = something;
}
```I KNOW. FUN.
As a side note I've been working on a "compressed" ComputedStyle which *change* these class of problems. It still has some performance issues however.
(the TL;DR of the compressed ComputedStyle is to use a bitset + vector for storing most fields - which would mean that it'll be much smaller memory, and for the objects currently nested 3-fields deep faster on more limited CPUs).
@Michael Lippautz - I 90% sure that within DataRef::Access we know that the object is retained by another object (everything starts off with ComputedStyle::CreateInitialStyleSingleton() ). Is it possible to exploit that fact here?
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
Yeah this is part of the complex memory structure of computed style. […]
A related question: If the previous Member is nullptr, can we avoid the barrier? If so, perhaps we could start off with all the subobjects as nullptr (instead of copied from the parent), and then only fill them in in either Access() or when we finalize the object. That way, we will only ever have nullptr-to-set transitions, never set-to-set transitions.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
Yeah this is part of the complex memory structure of computed style.
ComputedStyle shares a bunch of its internal slots with each other to reduce memory consumption.
This part I knew; I thought something had changed with ComputedStyleBuilder. But perhaps less changed than I had assumed.
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anton Bikineev, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
A related question: If the previous Member is nullptr, can we avoid the barrier? If so, perhaps we could start off with all the subobjects as nullptr (instead of copied from the parent), and then only fill them in in either Access() or when we finalize the object. That way, we will only ever have nullptr-to-set transitions, never set-to-set transitions.
Re nullptr: not easily. This is actually where one of the slow-downs comes from compressed ComputedStyle. E.g. accessors:
```
// currently
ComputedStyleBase::GetSomething() const {
return group_1_->group_2_->something_;
}
// nullptrs
ComputedStyleBase::GetSomething() const {
if (group_1_ && group_1_->group_2_) {
return group_1_->group_2_->something_;
}
return InitialSomething();
}
// bitset
ComputedStyleBase::GetSomething() const {
if (bitset_.Has(kSomething)) {
return storage_[bitset_.PopCount(kSomething)].something_;
}
return InitialSomething();
}
```That additional branch for the bitset one *can* be ok as the compiler will typically re-write:
```
if (style.Something() != InitialSomething()) {
// Do something with Something().
}
```
As:
```
if (style.bitset_.Has(kSomething)) {
if (style.storage_[bitset_.PopCount(kSomething)].something_ != InitialSomething()) { // this branch becomes predicted as never initial.
// and other check do something with Something();
}
}
```This only easily works if the return type is nullptr, or similar. But a lot of accessor callsites are expecting a ref, this can be changed however.
This part I knew; I thought something had changed with ComputedStyleBuilder. But perhaps less changed than I had assumed.
Mostly it should have been a no-op change (modulo changing some non-hot paths to keep the ComputedStyle immutable).
ComputedStyleBuilder (at the moment) is just a zero-cost wrapper, but allows other stuff now.
Michael Lippautz (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
> A related question: If the previous Member is nullptr, can we avoid the barrier? If so, perhaps we […]
- Avoiding the barrier (marking overall) by exploiting the fact that there's other retaining references: `UntracedMember` but only really use it when really perf critical. This is essentially turning off the GC for a reference.
- nullptr setter: As far as `Member` is concerned it will have a null check early on in the barrier that even works on compressed values. There's minimal overhead but definitely an early bailut. `= nullptr` is furthermore treated as `Clear()` and has no barrier checks at all.
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anton Bikineev, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
Avoiding the barrier
Specifically here we know that the existing object in the Member is being retained elsewhere, but the new object is not. Can we avoid it in this circumstance?
Michael Lippautz (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
> Avoiding the barrier […]
Our barrier is always concerned about the newly set object, so no, the information that the existing object is otherwise reachable cannot be used.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
Our barrier is always concerned about the newly set object, so no, the information that the existing […]
Just to be clear; this means that going from ptr to nullptr is cheap (avoids the barrier), but nullptr to ptr is the same cost as ptr1 to ptr2 (has barrier)? But not-constructed to ptr is cheap, presumably because we know Trace() cannot run before the object is done constructing?
Michael Lippautz (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
Just to be clear; this means that going from ptr to nullptr is cheap (avoids the barrier), but nullptr to ptr is the same cost as ptr1 to ptr2 (has barrier)? But not-constructed to ptr is cheap, presumably because we know Trace() cannot run before the object is done constructing?
Yes
- ptr -> nullptr avoids barrier
- nullptr -> ptr == ptr1 -> ptr2 has barrier
- in construction ptr is cheap because we handle the object conservatively (no Trace() execution as you said)
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
> Just to be clear; this means that going from ptr to nullptr is cheap (avoids the barrier), but nul […]
I ran with Alder Lake again, all 8 big cores available, and unfortunately, it's still hitting a GC cliff on style recalc:
```
Initial style (µs) Before After Perf 95% CI (BCa)
=================== ========= ========= ======= =================
ECommerce 4073 4020 +1.3% [ +0.4%, +2.1%]
Encyclopedia 32374 33609 -3.7% [ -4.5%, -2.7%]
Extension 51133 49785 +2.7% [ +2.1%, +3.4%]
News 17793 17236 +3.2% [ +2.1%, +4.2%]
Search 5695 5689 +0.1% [ -0.9%, +1.2%]
Social1 11524 11185 +3.0% [ +2.1%, +3.9%]
Social2 7060 6856 +3.0% [ +2.1%, +3.8%]
Sports 18868 19405 -2.8% [ -3.6%, -1.9%]
Video 16040 15802 +1.5% [ +0.8%, +2.3%]
Geometric mean +0.9% [ +0.3%, +1.5%]
Recalc style (µs) Before After Perf 95% CI (BCa)
=================== ========= ========= ======= =================
ECommerce 4469 4457 +0.3% [ -0.4%, +0.9%]
Encyclopedia 23489 25713 -8.6% [ -9.4%, -7.9%]
Extension 44175 43765 +0.9% [ +0.3%, +1.6%]
News 11577 12789 -9.5% [-10.4%, -8.6%]
Search 2469 2467 +0.1% [ -1.1%, +1.0%]
Social1 6796 6713 +1.2% [ +0.4%, +2.0%]
Social2 5227 5327 -1.9% [ -2.6%, -1.2%]
Sports 8643 9635 -10.3% [-10.9%, -9.6%]
Video 9183 9774 -6.0% [ -7.6%, -5.1%]
Geometric mean -3.9% [ -4.3%, -3.4%]
```So seemingly, it wasn't a single-core issue; the cliff is smaller but still pretty strong. (The effect is less pronounced on small-cores-only, but I don't think we care.)
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
I ran with Alder Lake again, all 8 big cores available, and unfortunately, it's still hitting a GC c […]
While profiling, I noticed something about the multi-threaded marking that I'm curious about. It seems that while we are calculating style on the main thread, ThreadPoolForeground (which I guess runs the parallel marker thread(s)) spends some time actually launching threads, fiddling with the work queue and locking and so on—and also allocating memory using PartitionAlloc, which I'm wondering maybe if blocks the main thread on a PA mutex sometimes?
My question is: Is this an artifact of the test setup? In a normal renderer situation, would this thread pool already be set up and going when style began, and we wouldn't be seeing this? (If so, can we pre-start them before the actual test somehow, so that it becomes more equal to a real situation?) Or are the threads indeed going up and down on-demand, so that this is a potentially real performance effect?
Michael Lippautz (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
While profiling, I noticed something about the multi-threaded marking that I'm curious about. It seems that while we are calculating style on the main thread, ThreadPoolForeground (which I guess runs the parallel marker thread(s)) spends some time actually launching threads, fiddling with the work queue and locking and so on—and also allocating memory using PartitionAlloc, which I'm wondering maybe if blocks the main thread on a PA mutex sometimes?
My question is: Is this an artifact of the test setup? In a normal renderer situation, would this thread pool already be set up and going when style began, and we wouldn't be seeing this? (If so, can we pre-start them before the actual test somehow, so that it becomes more equal to a real situation?) Or are the threads indeed going up and down on-demand, so that this is a potentially real performance effect?
This is generally the renderer architecture that also runs in production. The first setup may be amplified in your scenario but otherwise this is what users should also see.
As you noticed: Concurrent markers definitely allocate (for worklist and other metadata). We merely use malloc/new for that which means that everything is piped through to PA at this point. PA has a thread pool now which it uses for malloc() (I think), so I think for most part there should be no problem.
If you think you see an issue somewhere with PA locking please bring it up with PA folks. There's a known issue (at least to us) with ArrayBuffer handling which uses separate PA partitions and due to the PA setup cannot use a thread pool there. Anything using malloc() should afaik be fast though.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
> While profiling, I noticed something about the multi-threaded marking that I'm curious about. […]
Thanks; that means there _may_ be a PA issue somewhere (I guess the thread caches are not that full for a completely fresh thread?), but let's hold off going down that path until we know for sure it's a thing.
I made a gross hack to test the theory that the barriers are an issue, by simply removing them. For DataRef<T> (which, in the patch, holds an UncompressedMember<T>), I wrote a new operator=(const DataRef<T>&) that simply uses placement new on the UncompressedMember instead of assigning, so that it skips the barrier entirely. (Obviously, this is not suitable for a real binary!) Then I disabled GC by means of a NoGarbageCollectionScope on both sides. This yields (Zen 3, multiple cores allowed):
```
Recalc style (µs) Before After Perf 95% CI (BCa)
=================== ========= ========= ======= =================
Video 16482 15938 +3.4% [ +3.1%, +3.8%]
Geometric mean +3.4% [ +3.1%, +3.8%]
```which means that if we can get rid of (most of) these barriers by means of refactoring ComputedStyleBuilder, then we are left with no mystery regressions; only the GC itself.
I'll make a full test with barriers removed soon-ish, hopefully later today. :-)
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
Thanks; that means there _may_ be a PA issue somewhere (I guess the thread caches are not that full […]
Here's a test of old version (scoped_refptr) versus new version with barriers hacked out (so GC + no barriers). This is Zen 3, not Alder Lake (I'm traveling), but the results are encouraging:
Initial style (µs) Before After Perf 95% CI (BCa)
=================== ========= ========= ======= =================
ECommerce 6571 6435 +2.1% [ +1.2%, +2.9%]
Encyclopedia 53603 54161 -1.0% [ -1.8%, -0.1%]
Extension 92068 86677 +6.2% [ +5.4%, +7.1%]
News 29428 28030 +5.0% [ +4.0%, +6.2%]
Search 8317 8088 +2.8% [ +2.1%, +3.8%]
Social1 17605 16983 +3.7% [ +2.8%, +4.5%]
Social2 11672 11327 +3.0% [ +2.2%, +4.0%]
Sports 28409 28722 -1.1% [ -1.9%, -0.3%]
Video 24342 23749 +2.5% [ +1.6%, +4.1%]
Geometric mean +2.6% [ +2.0%, +3.3%]
Recalc style (µs) Before After Perf 95% CI (BCa)
=================== ========= ========= ======= =================
ECommerce 8172 7869 +3.9% [ +3.0%, +4.8%]
Encyclopedia 45385 45237 +0.3% [ -0.4%, +1.2%]
Extension 88122 84464 +4.3% [ +3.6%, +5.2%]
News 22627 23714 -4.6% [ -5.4%, -3.8%]
Search 4428 4251 +4.2% [ +3.5%, +5.0%]
Social1 13050 12550 +4.0% [ +3.1%, +5.0%]
Social2 9579 9282 +3.2% [ +2.2%, +4.1%]
Sports 17086 17591 -2.9% [ -3.6%, -2.0%]
Video 16969 16980 -0.1% [ -0.8%, +0.7%]
Geometric mean +1.3% [ +0.7%, +2.0%]We still have some losses (News and Sports, presumably due to GCs going), but not _nearly_ as bad, and they're offset by the wins. So if we can mitigate the barrier issues, we're probably fine overall.
There's one question I've pondered, though: How do we do memory-wise? I assume we're worse on peak memory usage (since the structures should be largely the same size, and Oilpan has 8 extra bytes of overhead), but by how much? Have we ever looked at it, and how do we know what is acceptable or not?
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
There's one question I've pondered, though: How do we do memory-wise? I assume we're worse on peak memory usage (since the structures should be largely the same size, and Oilpan has 8 extra bytes of overhead), but by how much? Have we ever looked at it, and how do we know what is acceptable or not?
I took a look through Heaptrack, and even though this is a tricky topic (our hooks are not very strong, and even less so when moving from one allocator to the other), it looks pretty bad. Some style perf subtests are basically net-zero, but some are regressing badly. E.g., the News (again!) test total memory increases by ~2MB (this is ~15% increase of the process as a whole, which includes the DOM and some layout stuff). I don't fully understand why; it looks like a lot of it comes from ApplyValue, doing sub-calls like SetBoxSizing, SetLeft etc. (each losing maybe 50 kB or so).
This is not temporary garbage being created which we can reclaim later; the result persists even if I force a full GC-for-testing just before the dropping the page (i.e., at the end of the test) and then sleep for ten seconds.
Do you have any idea what would cause this?
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
> There's one question I've pondered, though: How do we do memory-wise? I assume we're worse on peak […]
FWIW, I tried going back from UncompressedMember to Member, and it cuts about half a megabyte off that test. So it would help, but it would not mitigate the increase I'm seeing.
Michael Lippautz (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
FWIW, I tried going back from UncompressedMember to Member, and it cuts about half a megabyte off th […]
You can enable `cppgc_enable_object_names = true` to get C++ class names as types for DevTools heap snapshots. That also shows you where `Persistent` references are created (source location). That should show you exactly what is retained why.
If you cannot use the UI to take a snapshot, you can also get a snapshot using `--js-flags=--enable-natives-syntax` and `%TakeHeapSnapshot('filename.heapsnapshot')` from JS. This does require `--no-sandbox`.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
You can enable `cppgc_enable_object_names = true` to get C++ class names as types for DevTools heap […]
I don't understand what this is an answer to. I'm not using devtools at all, nor am I looking into Persistent, nor am I trying to debug a leak.
Michael Lippautz (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
I don't understand what this is an answer to. […]
You are trying to figure out why you have a 2MB overall regression, right?
If that's Oilpan-managed memory, then you can use DevTools (the heap snapshot) to debug that.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
You are trying to figure out why you have a 2MB overall regression, right? […]
It used to be PA-memory, now it is Oilpan memory. (I don't have devtools; I'm not running a browser and I don't run any JavaScript.) I see exactly where it is allocated in Heaptrack, my question is _why_ it goes up. (And to be clear, again, this is not a leak, it is about more memory being allocated in the first place, which I don't understand why would happen.) I don't understand how a Oilpan snapshot would help me do that, especially since the “before” side doesn't involve Oilpan.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
It used to be PA-memory, now it is Oilpan memory. […]
Re avoiding barriers: I've worked on this for a while, and I think I have a solution that would allow us to set most subgroups directly in the constructor. It's not completely done yet (there are some bugs around e.g. pseudo-elements), but I think it would effectively solve the barrier issues. Most of the style team (including myself) is going on vacation for the rest of July, but if you want to play with it, there's a proof-of-concept CL at:
https://chromium-review.googlesource.com/c/chromium/src/+/4667195/7
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
Re avoiding barriers: I've worked on this for a while, and I think I have a solution that would allo […]
That CL is now reviewed and submitted, so it might be interesting to rebase and then we can see how well it works in the benchmarks. (Of course, you will have a fair amount of conflicts.)
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anton Bikineev, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
That CL is now reviewed and submitted, so it might be interesting to rebase and then we can see how […]
Awesome - notified by merge conflicts :). I've worked on this a little today (rebases + compiles). But I'll work on it tomorrow as well (I think I've dropped some stuff from the patch).
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1743725f160000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/11cbb9ee160000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
😿 Job failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/13ee96b6160000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/10b23ea9160000
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
9 comments:
Patchset:
Awesome - notified by merge conflicts :). I've worked on this a little today (rebases + compiles). […]
StyleCalcPerfTest.News is pretty sad still.
File third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.h.tmpl:
Patch Set #52, Line 190:
void TraceAfterDispatch(Visitor* visitor) const {Why do we need a `TraceAfterDispatch()`? That's generally only used for switching types in `Trace()` […]
The inheritance hierarchy is:
ComputedStyle : public ComputedStyleBase {}
ComputedStyleBase : public GarbageCollected<> {}(and currently no vtable). Needs a trace after dispatch to avoid vtable.
File third_party/blink/renderer/core/css/resolver/style_resolver.h:
Patch Set #52, Line 335:
subtle::UncompressedMember<const ComputedStyle> initial_style_for_img_;Are these two pointers performance-sensitive enough to use UncompressedMember?
Yeah they are in the hotpath for creating computed-styles.
File third_party/blink/renderer/core/layout/layout_object.h:
Patch Set #52, Line 4381:
subtle::UncompressedMember<const ComputedStyle> style_;If performance matters for this pointer, maybe we should consider using UncompressedMember for other […]
style_ has different access patterns to parent_, previous_, next_. style is access everywhere, where-as tree traversals are comparatively rare.
File third_party/blink/renderer/core/layout/ng/grid/ng_grid_line_resolver.h:
Patch Set #52, Line 134:
Persistent<const ComputedStyle> style_;I'm afraid this will create a reference cycle. […]
Not easily at the moment - this class is used on both the stack and part of allocated objects.
It shouldn't create a cycle (ComputedStyle objects don't reference any layout like objects).
File third_party/blink/renderer/core/paint/paint_layer.cc:
Patch Set #52, Line 2199:
old_style ? old_style->Filter() != new_style.Filter() ||Is this an intentional change?
Yes - previously filter had a wrapper around a Persistent to keep things simpler. This is no longer needed.
Patch Set #52, Line 2219:
old_style ? old_style->BackdropFilter() != new_style.BackdropFilter()Ditto.
See above.
File third_party/blink/renderer/core/style/anchor_specifier_value.h:
Patch Set #52, Line 51:
CORE_EXPORT void Trace(Visitor*) const;Is this really needed?
Done
File third_party/blink/renderer/core/style/list_style_type_data.h:
Patch Set #52, Line 24:
CORE_EXPORT void Trace(Visitor*) const;I'm not sure if we need this.
blink_tests compile is sad without this.
To view, visit change 4346534. To unsubscribe, or for help writing mail filters, visit settings.
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
📍 Job complete.
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1615529b160000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/11456472160000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17f85dd4960000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1578d4ac960000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/11a7ff14960000
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
StyleCalcPerfTest.News is pretty sad still.
Yes, seemingly :-/
I've confirmed that it is indeed still ~10% here; I've ran a quick profile and can't see anything obvious, but due to weather conditions I'm WFH-ing for a couple of days and don't have access to the machine I'd usually profile on. So it will probably be a little before I can try to dive into this.
An interesting tidbit on my Zen 3 is this, from perf stat (before, then after):
528 135 967 cycles
423 445 stalled-cycles-backend # 0,08% backend cycles idle
586 410 525 cycles
11 095 450 stalled-cycles-backend # 1,89% backend cycles idle
There's huge variance between runs, but I'd like to understand what happens here...
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
Yes, seemingly :-/ […]
I did a preliminary check on another machine. Here are the gainers for the main thread only (there are huge increases in the thread pool that are not shown here, but let's count them as “free” for the time being):
0.21% 0.65% +0.44% cppgc::internal::MakeGarbageCollectedTraitInternal::Allocate
----- 0.53% +0.53% cppgc::internal::MarkingStateBase::MarkAndPush
----- 0.40% +0.40% cppgc::internal::WriteBarrier::CombinedWriteBarrierSlow<(cppgc::internal::WriteBarrierSlotType)0>
0.21% 0.39% +0.18% blink::ThreadStateStorage::Current
----- 0.38% +0.38% cppgc::internal::WriteBarrier::DijkstraMarkingBarrierSlow
0.11% 0.34% +0.23% cppgc::internal::BasePage::heapSo we are still on the barriers as a suspect. The good news is that the barriers from the MPC path are entirely gone now. That's what we wanted, and it's good to see that the patch did its job. Here are the main sources of calling COmbinedWriteBarrierSlow (on UncompressedMembers; there's also some compressed ones):
- 0,56% cppgc::internal::WriteBarrier::CombinedWriteBarrierSlow<(cppgc::internal::WriteBarrierSlotType)1>
+ 0,15% blink::NodeData::SetComputedStyle
+ 0,07% blink::LayoutObject::SetStyle
+ 0,06% blink::ComputedStyleBuilderBase::SetFont
+ 0,03% blink::ComputedStyleBuilderBase::SetForcesStackingContext
+ 0,03% blink::ComputedStyleBuilderBase::SetBoxSizing
+ 0,02% blink::ComputedStyleBuilderBase::SetInternalVisitedColor
+ 0,02% blink::ComputedStyleBuilderBase::SetColor
+ 0,02% blink::ComputedStyleBuilderBase::MutableTransitionsInternal
0,01% blink::ComputedStyleBuilderBase::MutableBackgroundInternal
+ 0,01% blink::ComputedStyleBuilderBase::SetInternalVisitedBackgroundColorSo this is more what we'd expect? The individual Set* can still invoke copy-on-write, which requires a barrier. Perhaps we can get rid of that up-front when constructing the object? (I know you'd like to move to a builder entirely, but I think we can get halfway there by marking the access bits ahead of time and then doing the COW in the constructor. Would be an interesting experiment.) I guess we just have to bite the bullet wrt. NodeData and LayoutObject's setters.
Is there a way to turn off GC entirely, so that we can check whether the concurrent marking is interfering somehow? Wondering if it might be eating through some shared resource like the L1 cache.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
I did a preliminary check on another machine. […]
Of course, it was mentioned earlier in the thread… with Ian's patch to disable GC entirely on both sides, we're in plus (initial style + recalc):
News 30878 29718 +3.9% [ +3.4%, +4.4%]
News 21849 21429 +2.0% [ +1.4%, +2.5%]I still don't know if this is about _concurrent_ marking being in the way, and I'm not sure if I have a good way of knowing.
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1510e254960000
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
2 comments:
Patchset:
So this is more what we'd expect? The individual Set* can still invoke copy-on-write, which requires a barrier. Perhaps we can get rid of that up-front when constructing the object? (I know you'd like to move to a builder entirely, but I think we can get halfway there by marking the access bits ahead of time and then doing the COW in the constructor. Would be an interesting experiment.) I guess we just have to bite the bullet wrt. NodeData and LayoutObject's setters.
Yeah NodeData/LayoutObject aren't able to disappear.
I tried a smaller incremental step by replacing the access bits with raw-pointers if we have our own group of that type.
There is still a write barrier when assigning a top-level group to the computed-style, but when mutating a field will copy-construct in reverse order with no barrier.
See diff: https://chromium-review.googlesource.com/c/chromium/src/+/4346534/54..57
Should remove the barriers for nested fields.
File third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.h.tmpl:
Patch Set #52, Line 300:
using Space = blink::NodeSpace;The better objects are clustered, the better the utilization of free space. […]
Move to the default space for the moment.
To view, visit change 4346534. To unsubscribe, or for help writing mail filters, visit settings.
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
😿 Job failed.
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/149c1fac960000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15b4b8a2960000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
😿 Job failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/154c1738960000
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
> So this is more what we'd expect? The individual Set* can still invoke copy-on-write, which requir […]
I don't think I understand how this works at all. How are you allowed to have raw pointers to GC-ed objects?
I've done some looking, and it really seems that the concurrent marking is interfering heavily with the style computation. But I'm not sure through what exact mechanism.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
I've done some looking, and it really seems that the concurrent marking is interfering heavily with the style computation. But I'm not sure through what exact mechanism.
Some documentation of my musings: https://docs.google.com/document/d/1Bu77mrLr6fWvLphBH8L4brr-3ys0svrMQFZoE2hJOKI/edit
This isn't perfect since I have to go home now (some of the graphs are run with slightly different versions of my script), but I think it basically shows that we can live without the News test going positive.
We still have the RAM usage to deal with, though. And I wouldn't say no to having News positive either :-)
(FWIW, this is with patchset 54, not 57, since I don't understand 57 :-) )
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
> I've done some looking, and it really seems that the concurrent marking is interfering heavily wit […]
@mlip...@chromium.org, a few questions for you based on the doc above:
1. Is there something we can call to simulate a normal GC that would happen during a small pause? CollectGarbageForTesting() seems unrealistically thorough, since it does full GCs several times (which would be bad if we want to time it, for one); is there anything slightly lighter?
2. From the graphs, it seems concurrent GC runs very often; during the course of a 11 ms style recalc, it actually appears to trigger _hundreds_ of times, doing very little work every time (but causing the slow barrier path to crop up very frequently). Is this intentional? Is there a way we can make it a bit less trigger-happy?
Ian Kilpatrick (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
I don't think I understand how this works at all. How are you allowed to have raw pointers to GC-ed objects?
Raw pointers on the stack are ok (and that's what this is exploiting) https://chromium.googlesource.com/chromium/src/+/master/third_party/blink/renderer/platform/heap/BlinkGCAPIReference.md#stack_allocated
(anything that is STACK_ALLOCATED, e.g. the builder can have raw pointers).
(FWIW, this is with patchset 54, not 57, since I don't understand 57 :-) )
Let me revert to PS-54. PS-57 was a little hacky for my liking, and only really helped for one class of write-barrier.
Michael Lippautz (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
> I don't think I understand how this works at all. […]
1. normal GC cannot be programmatically called from Blink, no. We generally don't expose these internals.
2. You need to clarify what trigger means. A single GC may run longer than 11ms wall time with many small slices that make progress. That's still just a single GC call.I doubt that the GC itself triggers 100s of times in one 11ms slice. Once started we want to make progress to avoid active write barriers.I am currently OOO -- should we schedule a VC in when I am back to discuss how to proceed here?
To view, visit change 4346534. To unsubscribe, or for help writing mail filters, visit settings.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
1. normal GC cannot be programmatically called from Blink, no. We generally don't expose these internals.
Not even for testing purposes?
2. You need to clarify what trigger means. A single GC may run longer than 11ms wall time with many small slices that make progress. That's still just a single GC call.I doubt that the GC itself triggers 100s of times in one 11ms slice. Once started we want to make progress to avoid active write barriers.
Trigger as in: It goes from doing nothing, to doing marking, to doing nothing again. Let me give an example from one of the traces. At some point, we have a ThreadPoolForeground that sits in the kernel waiting for work (I've cut the stack trace for brevity, but it ends in WorkerThread::RunWorker):
ThreadPoolForeg 3863092 [002] 5978508.374605: 19 cycles:
64f7640 pthread_cond_timedwait@plt+0x0 (/home/sesse/chromium/src/out/Release/blink_perf_tests)
3338c1f base::ConditionVariable::TimedWait+0x11f (/home/sesse/chromium/src/out/Release/blink_perf_tests)
[…more stack trace…]After about 2 ms, it gets woken up, and after some stuff around WaitableEvent and such creates a visitor and then goes stuff like this:
ThreadPoolForeg 3863092 [002] 5978508.376889: 1 cycles:
464ad0f cppgc::internal::TraceTraitBase<blink::HeapVector<cppgc::internal::BasicMember<blink::RegisteredEventListener, cppgc::internal::StrongMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::i>
28f3b76 cppgc::internal::(anonymous namespace)::ConcurrentMarkingTask::Run+0x3f6 (/home/sesse/chromium/src/out/Release/blink_perf_tests)
[…more stack trace…]
But after just 151 µs, it's done, and has exited ConcurrentMarkingTask::Run:ThreadPoolForeg 3863092 [002] 5978508.377040: 4 cycles:
522c8db base::internal::Invoker<base::internal::BindState<gin::V8Platform::CreateJobImpl(v8::TaskPriority, std::__Cr::unique_ptr<v8::JobTask, std::__Cr::default_delete<v8::JobTask> >, v8::SourceLocation con>
3314496 base::internal::Invoker<base::internal::BindState<base::internal::JobTaskSource::JobTaskSource(base::Location const&, base::TaskTraits const&, base::RepeatingCallback<void (base::JobDelegate*)>, bas>
[…more stack trace…]
It then goes back to pthread_cond_timedwait, waits a bit, wakes up again, and so on. I'm not sure if I can defend the “hundreds” moniker (it's a bit hard to read the traces), but it's easily ten times during that single run. You can see this in the graphs in the document I posted. I'm trying to figure out if this is intentional or not. :-)It's a bit unclear to me how to figure out when the barriers are actually causing problems. We hit DijkstraMarkingBarrierRangeSlow() a _lot_ of times (more than 10k), but “slow” does perhaps not really mean “the slow case”?
I am currently OOO -- should we schedule a VC in when I am back to discuss how to proceed here?
Sure, let me know when you're back.
Anton Bikineev (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
Thanks Steinar for the analysis.
You can see this in the graphs in the document I posted. I'm trying to figure out if this is intentional or not. :-)
It is. The scheduler signals the concurrent marker that it should yield [1] (for whatever reason the scheduler decides, e.g. a higher piority task comes in), which is why you see this incremental behaviour.
It's a bit unclear to me how to figure out when the barriers are actually causing problems. We hit DijkstraMarkingBarrierRangeSlow() a lot of times (more than 10k), but “slow” does perhaps not really mean “the slow case”?
This is a slow path of the write barrier (when concurrent marking is in progress), but that doesn't necessarily mean this would be observable. Would it be possible to run it through `perf diff` and see how many more cycles in `DijkstraMarkingBarrierRangeSlow()` we get with the CL?
To view, visit change 4346534. To unsubscribe, or for help writing mail filters, visit settings.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
It is. The scheduler signals the concurrent marker that it should yield [1] (for whatever reason the scheduler decides, e.g. a higher piority task comes in), which is why you see this incremental behaviour.
This is very interesting! This is a userspace scheduler, right, not the Linux kernel's scheduler? I don't understand exactly why it would want it to yield, because there's nothing else going on in these cores. It's simply going back to the kernel's idle task, as far as I can see.
This is a slow path of the write barrier (when concurrent marking is in progress), but that doesn't necessarily mean this would be observable. Would it be possible to run it through perf diff and see how many more cycles in DijkstraMarkingBarrierRangeSlow() we get with the CL?
It's in the document; we spend about 2.5M cycles more in DijkstraMarkingBarrierRangeSlow() with the CL. (Well, it's not 100%, because I also think I could pthread_mutex_lock as part of that table, which can sometimes happen on the othre threads, but it's in the right ballpark, and it fits well with the ~10% slowdown we're seeing.)
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
> It is. […]
On the memory front, I think I have some good news. I spent a lot of time trying to hook Oilpan better into Heaptrack, and the conclusion is: We're now using more RAM in peak, but we're using slightly less after GC.
One of the things that confused me a lot is that ComputedStyle's Set* can now allocate memory in a very surprising way: Of course, it can do allocation as part of copy-on-write, but it can _also_ do so as part of the write barrier afterwards. If it needs to go into the slow path, seemingly it can allocate some sort of work list, and of course that shows up as “SetFoo() allocated more memory than before”. Even though the ComputedStyle sub-objects are slightly smaller than before, and we allocate the same number of them as far as I can see. I assume we also allocate more memory in peak just because GC is like that (delayed free instead of right away).
So, well, this means that I no longer have strong blockers against this CL. It would be nice to know what's going on with all the barriers and stuff (slowing down the News test), and it would be nice to have a way of doing a regular GC between the two calculations, but overall, I believe we're now no worse off than we used to.
To view, visit change 4346534. To unsubscribe, or for help writing mail filters, visit settings.
Anton Bikineev (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
This is very interesting! This is a userspace scheduler, right, not the Linux kernel's scheduler? I don't understand exactly why it would want it to yield, because there's nothing else going on in these cores. It's simply going back to the kernel's idle task, as far as I can see.
Exactly, that's the Chromium's scheduler. It's probably a question to the Catan folks why yielding is requested when the core starves.
It's in the document; we spend about 2.5M cycles more in DijkstraMarkingBarrierRangeSlow() with the CL.
Thanks for the detailed analysis! Would it be possible to see the hottest callers of `DijkstraMarkingBarrierRangeSlow`? We could try to optimize the barrier in those contexts, if possible.
I spent a lot of time trying to hook Oilpan better into Heaptrack
I imagine it must've been quite tricky, since IIRC heaptrack intercepts through `dlsym`, so I guess you made `MakeGarbageCollected` (or `ObjectAllocator::Allocate`) and the sweeping functions as `__attribute__((visibility("default")))`? In any case, I think it would be great to upstream the changes to Oilpan under a flag!
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
Thanks for the detailed analysis! Would it be possible to see the hottest callers of DijkstraMarkingBarrierRangeSlow? We could try to optimize the barrier in those contexts, if possible.
I'll see what I can do. :-) I assume they're all a bunch of callers to the Member setters in ComputedStyle, though.
I imagine it must've been quite tricky, since IIRC heaptrack intercepts through dlsym, so I guess you made MakeGarbageCollected (or ObjectAllocator::Allocate) and the sweeping functions as __attribute__((visibility("default")))? In any case, I think it would be great to upstream the changes to Oilpan under a flag!
Heaptrack has an API where you can tell is explicitly, but I struggled a lot with finding all the right places. I ended up adding code to these three:
- MakeGarbageCollectedTraitBase::Allocate
- cppgc::subtle::Resize
- cppgc::internal::SetMemoryInaccessible (with pointer adjustment)
However, using that API requires pulling in a .h file, which I suppose needs something in third_party, which I wasn't really ready for figuring out… :-)
I also have some hooks for PartitionAlloc; there are 7–8 places there, IIRC. (There is a hooks framework, but it doesn't actually give you all allocations, so it's not that useful for these purposes.)
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
> Thanks for the detailed analysis! Would it be possible to see the hottest callers of DijkstraMarki […]
I sampled some data; I took backtraces every 20 ns (50 MHz sampling, roughly), picked out the ones that contained DijkstraMarkingBarrierSlow, and grouped them on the three immediate caller function names. These are the top hitters; hopefully they're what you wanted. (These include parsing, which is why you're seeing things like ConsumeFontFamily in there. It was a bit simpler this way; I can remove it if they're troublesome.)
```
4450 samples (28.79%):
WriteBarrier::CombinedWriteBarrierSlow<(WriteBarrierSlotType)1>
blink::NodeData::SetComputedStyle
blink::Element::RecalcOwnStyle
-------
1102 samples (7.13%):
WriteBarrier::CombinedWriteBarrierSlow<(WriteBarrierSlotType)1>
blink::LayoutObject::SetStyle
blink::Element::RecalcOwnStyle
-------
1023 samples (6.62%):
WriteBarrier::CombinedWriteBarrierSlow<(WriteBarrierSlotType)0>
WTF::HashTable<BasicMember<blink::MemoryPressureListener>, BasicMember<blink::MemoryPressureListener>, WTF::IdentityExtractor, WTF::HashTraits<BasicMember<blink::MemoryPressureListener> >, WTF::HashTraits<BasicMember<blink::MemoryPressureListener> >, blink::HeapAllocator>::insert<WTF::IdentityHashTranslator<WTF::HashTraits<BasicMember<blink::MemoryPressureListener> > >, blink::MemoryPressureListener*&, blink::MemoryPressureListener*&>
blink::MemoryPressureListenerRegistry::RegisterClient
-------
729 samples (4.72%):
WriteBarrier::CombinedWriteBarrierSlow<(WriteBarrierSlotType)0>
blink::CachedMatchedProperties::Set
blink::MatchedPropertiesCache::Add
-------
701 samples (4.54%):
WriteBarrier::CombinedWriteBarrierSlow<(WriteBarrierSlotType)1>
blink::ComputedStyleBuilderBase::SetFont
blink::FontBuilder::CreateFont
-------
519 samples (3.36%):
blink::CSSValueList::Append
blink::css_parsing_utils::ConsumeFontFamily
blink::css_parsing_utils::ParseLonghand
-------
468 samples (3.03%):
WriteBarrier::CombinedWriteBarrierSlow<(WriteBarrierSlotType)1>
blink::ComputedStyleBuilderBase::SetColor
blink::css_longhand::Color::ApplyValue
-------
465 samples (3.01%):
WTF::HashTable<BasicMember<blink::MemoryPressureListener>, BasicMember<blink::MemoryPressureListener>, WTF::IdentityExtractor, WTF::HashTraits<BasicMember<blink::MemoryPressureListener> >, WTF::HashTraits<BasicMember<blink::MemoryPressureListener> >, blink::HeapAllocator>::insert<WTF::IdentityHashTranslator<WTF::HashTraits<BasicMember<blink::MemoryPressureListener> > >, blink::MemoryPressureListener*&, blink::MemoryPressureListener*&>
blink::MemoryPressureListenerRegistry::RegisterClient
blink::Resource::Resource
-------
398 samples (2.57%):
WriteBarrier::CombinedWriteBarrierSlow<(WriteBarrierSlotType)0>
blink::StyleResolver::ApplyAnimatedStyle
blink::StyleResolver::ResolveStyle
```
Anton Bikineev (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
1 comment:
Patchset:
MakeGarbageCollectedTraitBase::Allocate
cppgc::subtle::Resize
cppgc::internal::SetMemoryInaccessible (with pointer adjustment)
These seem to be right.
I also have some hooks for PartitionAlloc; there are 7–8 places there, IIRC. (There is a hooks framework, but it doesn't actually give you all allocations, so it's not that useful for these purposes.)
I remember there was an issue while working with heaptrack and Chromium: the overriden `operator new/delete` call the shim functions directly, i.e. bypassing `malloc`/`free`. As a result, I didn't get comprehensive results. I fixed it so: https://chromium-review.googlesource.com/c/chromium/src/+/4778619. Just to make sure, this is the same fix you are referring to?
I sampled some data; I took backtraces every 20 ns (50 MHz sampling, roughly), picked out the ones that contained DijkstraMarkingBarrierSlow, and grouped them on the three immediate caller function names. These are the top hitters; hopefully they're what you wanted. (These include parsing, which is why you're seeing things like ConsumeFontFamily in there. It was a bit simpler this way; I can remove it if they're troublesome.)
Thanks for the analysis! Would it be possible to add this data to the doc so that it's all in one place?
The problem is that the dominating `SetComputedStyle` is called upon `RecalcOwnStyle` on the existing element, so it's not an initializing store, which would allow us to drop the barrier..
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anders Hartvoll Ruud, Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
1 comment:
Patchset:
operator new/delete call the shim functions directly, i.e. bypassing malloc/free. As a result, I didn't get comprehensive results. I fixed it so: https://chromium-review.googlesource.com/c/chromium/src/+/4778619. Just to make sure, this is the same fix you are referring to?
No, I'm not referring to any fix, and I'm not using malloc/free; just as with Oilpan, I'm calling heaptrack_report_alloc() etc. at the right spots. The bug I'm talking about is crbug.com/1363147.
Thanks for the analysis! Would it be possible to add this data to the doc so that it's all in one place?
Done.
The problem is that the dominating SetComputedStyle is called upon RecalcOwnStyle on the existing element, so it's not an initializing store, which would allow us to drop the barrier..
Yes, no big surprise there; we can't just get rid of the stores. The question is why they hit the slow path.
To view, visit change 4346534. To unsubscribe, or for help writing mail filters, visit settings.
Anders Hartvoll Ruud (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
Anders Hartvoll Ruud would like Steinar H Gunderson to review this change authored by Ian Kilpatrick.
[gc] Make ComputedStyle & friends GarbageCollected.
This patch moves ComputedStyle (and sub-objects) onto the gc-heap for
security reasons.
The performance profile of this patch is "complex". :).
For the motionmark-benchmark (non-PGO as so many signatures change due
to this patch) we roughly have:
+-------------+----------------+----------------+---------------+
| MotionMark | M1[1][2] | Win10[3][4] | Pixel4[5][6] |
+-------------+----------------+----------------+---------------+
| Leaves | -1.2%, -2.2% | NA, -1.1% | +4.3%, +3.6% |
| Multiply | | +1.2%, +1.6% | +3.4%, +3.7% |
| Design | +6.3%, +6.0% | +4.4%, +5.8% | |
| Paths | | -1.0%, NA | |
| Suites | -1.1%, NA | | |
+-------------+----------------+----------------+---------------+
[1] https://pinpoint-dot-chromeperf.appspot.com/job/14ec7f21a60000
[2] https://pinpoint-dot-chromeperf.appspot.com/job/13457172a60000
[3] https://pinpoint-dot-chromeperf.appspot.com/job/1286eecfa60000
[4] https://pinpoint-dot-chromeperf.appspot.com/job/140ffd8fa60000
[5] https://pinpoint-dot-chromeperf.appspot.com/job/1434db4fa60000
[6] https://pinpoint-dot-chromeperf.appspot.com/job/10c9d653a60000
This is (broadly speaking) a net-win. (E.g. M1 ~+0.5%).
There is no significant difference on Speedometer.
https://pinpoint-dot-chromeperf.appspot.com/job/10c0e757a60000
There should be no functional behaviour change.
Change-Id: I604f6ec1e4c08dd4006bc1416e40cfd5594e5363
---
M third_party/blink/renderer/build/scripts/core/style/make_computed_style_base.py
M third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.cc.tmpl
M third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.h.tmpl
M third_party/blink/renderer/build/scripts/templates/fields/field.tmpl
M third_party/blink/renderer/build/scripts/templates/fields/group.tmpl
M third_party/blink/renderer/core/animation/animation_utils.cc
M third_party/blink/renderer/core/animation/compositor_animations_test.cc
M third_party/blink/renderer/core/animation/css/css_animation_update.cc
M third_party/blink/renderer/core/animation/css/css_animation_update.h
M third_party/blink/renderer/core/animation/css/css_animations.cc
M third_party/blink/renderer/core/animation/css/css_animations.h
M third_party/blink/renderer/core/animation/css_length_interpolation_type.cc
M third_party/blink/renderer/core/animation/keyframe_effect_model_test.cc
M third_party/blink/renderer/core/animation/scroll_timeline_util_test.cc
M third_party/blink/renderer/core/animation/transition_keyframe.cc
M third_party/blink/renderer/core/css/container_query_evaluator_test.cc
M third_party/blink/renderer/core/css/css_math_expression_node_test.cc
M third_party/blink/renderer/core/css/css_properties.json5
M third_party/blink/renderer/core/css/font_face_set_document.cc
M third_party/blink/renderer/core/css/post_style_update_scope.cc
M third_party/blink/renderer/core/css/post_style_update_scope.h
M third_party/blink/renderer/core/css/properties/css_property_test.cc
M third_party/blink/renderer/core/css/properties/longhands/custom_property_test.cc
M third_party/blink/renderer/core/css/resolver/element_resolve_context.cc
M third_party/blink/renderer/core/css/resolver/element_resolve_context.h
M third_party/blink/renderer/core/css/resolver/element_style_resources.cc
M third_party/blink/renderer/core/css/resolver/font_builder_test.cc
M third_party/blink/renderer/core/css/resolver/matched_properties_cache.cc
M third_party/blink/renderer/core/css/resolver/matched_properties_cache.h
M third_party/blink/renderer/core/css/resolver/matched_properties_cache_test.cc
M third_party/blink/renderer/core/css/resolver/style_adjuster.cc
M third_party/blink/renderer/core/css/resolver/style_builder_test.cc
M third_party/blink/renderer/core/css/resolver/style_cascade_test.cc
M third_party/blink/renderer/core/css/resolver/style_resolver.cc
M third_party/blink/renderer/core/css/resolver/style_resolver.h
M third_party/blink/renderer/core/css/resolver/style_resolver_state.cc
M third_party/blink/renderer/core/css/resolver/style_resolver_state.h
M third_party/blink/renderer/core/css/resolver/style_resolver_test.cc
M third_party/blink/renderer/core/css/style_engine.cc
M third_party/blink/renderer/core/css/style_engine_test.cc
M third_party/blink/renderer/core/display_lock/display_lock_context.cc
M third_party/blink/renderer/core/display_lock/display_lock_context.h
M third_party/blink/renderer/core/dom/document.cc
M third_party/blink/renderer/core/dom/document.h
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/dom/first_letter_pseudo_element.cc
M third_party/blink/renderer/core/dom/first_letter_pseudo_element.h
M third_party/blink/renderer/core/dom/layout_tree_builder.cc
M third_party/blink/renderer/core/dom/layout_tree_builder.h
M third_party/blink/renderer/core/dom/node.cc
M third_party/blink/renderer/core/dom/node.h
M third_party/blink/renderer/core/dom/node_rare_data.cc
M third_party/blink/renderer/core/dom/node_rare_data.h
M third_party/blink/renderer/core/dom/pseudo_element.cc
M third_party/blink/renderer/core/dom/pseudo_element.h
M third_party/blink/renderer/core/dom/text.cc
M third_party/blink/renderer/core/frame/local_frame_view.cc
M third_party/blink/renderer/core/highlight/highlight_style_utils.cc
M third_party/blink/renderer/core/highlight/highlight_style_utils.h
M third_party/blink/renderer/core/highlight/highlight_style_utils_test.cc
M third_party/blink/renderer/core/html/canvas/canvas_font_cache.cc
M third_party/blink/renderer/core/html/canvas/canvas_font_cache.h
M third_party/blink/renderer/core/html/forms/date_time_edit_element.cc
M third_party/blink/renderer/core/html/forms/date_time_edit_element.h
M third_party/blink/renderer/core/html/forms/html_select_list_element.cc
M third_party/blink/renderer/core/html/forms/internal_popup_menu.cc
M third_party/blink/renderer/core/html/forms/menu_list_inner_element.cc
M third_party/blink/renderer/core/html/forms/menu_list_inner_element.h
M third_party/blink/renderer/core/html/forms/select_type.cc
M third_party/blink/renderer/core/html/forms/text_control_inner_elements.cc
M third_party/blink/renderer/core/html/forms/text_control_inner_elements.h
M third_party/blink/renderer/core/html/html_embed_element_test.cc
M third_party/blink/renderer/core/html/html_html_element.cc
M third_party/blink/renderer/core/html/html_html_element.h
M third_party/blink/renderer/core/html/html_plugin_element.cc
M third_party/blink/renderer/core/html/html_plugin_element.h
M third_party/blink/renderer/core/html/shadow/meter_shadow_element_test.cc
M third_party/blink/renderer/core/html/shadow/progress_shadow_element_test.cc
M third_party/blink/renderer/core/inspector/inspector_dom_snapshot_agent.cc
M third_party/blink/renderer/core/inspector/inspector_dom_snapshot_agent.h
M third_party/blink/renderer/core/layout/build.gni
M third_party/blink/renderer/core/layout/custom_scrollbar.cc
M third_party/blink/renderer/core/layout/custom_scrollbar.h
M third_party/blink/renderer/core/layout/flexible_box_algorithm.cc
M third_party/blink/renderer/core/layout/flexible_box_algorithm.h
M third_party/blink/renderer/core/layout/layout_block.cc
M third_party/blink/renderer/core/layout/layout_block_flow.cc
M third_party/blink/renderer/core/layout/layout_block_flow.h
M third_party/blink/renderer/core/layout/layout_box.cc
M third_party/blink/renderer/core/layout/layout_box.h
M third_party/blink/renderer/core/layout/layout_object.cc
M third_party/blink/renderer/core/layout/layout_object.h
M third_party/blink/renderer/core/layout/layout_object_hot.cc
M third_party/blink/renderer/core/layout/layout_text.cc
M third_party/blink/renderer/core/layout/layout_text.h
M third_party/blink/renderer/core/layout/layout_view.cc
M third_party/blink/renderer/core/layout/list_marker.cc
M third_party/blink/renderer/core/layout/ng/flex/ng_flex_layout_algorithm.cc
M third_party/blink/renderer/core/layout/ng/grid/ng_grid_line_resolver.h
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_box_state.h
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_break_token.cc
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_break_token.h
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder.cc
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder.h
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder_test.cc
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.cc
M third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.h
M third_party/blink/renderer/core/layout/ng/inline/ng_line_box_fragment_builder.h
M third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.cc
M third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.h
M third_party/blink/renderer/core/layout/ng/inline/ng_line_info.h
M third_party/blink/renderer/core/layout/ng/inline/ng_line_truncator.h
M third_party/blink/renderer/core/layout/ng/layout_ng_ruby_run.cc
M third_party/blink/renderer/core/layout/ng/ng_box_fragment_builder.h
M third_party/blink/renderer/core/layout/ng/ng_fragment_builder.h
M third_party/blink/renderer/core/layout/ng/ng_ink_overflow.cc
M third_party/blink/renderer/core/layout/ng/ng_layout_algorithm.h
M third_party/blink/renderer/core/layout/ng/ng_length_utils_test.cc
M third_party/blink/renderer/core/layout/ng/ng_out_of_flow_layout_part.cc
M third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.h
M third_party/blink/renderer/core/layout/ng/ng_relative_utils_test.cc
M third_party/blink/renderer/core/layout/ng/physical_fragment_rare_data.cc
M third_party/blink/renderer/core/layout/ng/physical_fragment_rare_data.h
M third_party/blink/renderer/core/layout/ng/table/layout_ng_table.cc
M third_party/blink/renderer/core/layout/ng/table/layout_ng_table.h
M third_party/blink/renderer/core/layout/ng/table/layout_ng_table_cell.cc
M third_party/blink/renderer/core/layout/ng/table/layout_ng_table_row.cc
M third_party/blink/renderer/core/layout/ng/table/layout_ng_table_section.cc
M third_party/blink/renderer/core/layout/ng/table/ng_table_borders.cc
M third_party/blink/renderer/core/layout/ng/table/ng_table_borders.h
M third_party/blink/renderer/core/layout/ng/table/ng_table_layout_algorithm.cc
M third_party/blink/renderer/core/layout/ng/table/ng_table_node.cc
M third_party/blink/renderer/core/layout/ng/table/ng_table_node.h
M third_party/blink/renderer/core/layout/overflow_model.h
D third_party/blink/renderer/core/layout/style_retain_scope.cc
D third_party/blink/renderer/core/layout/style_retain_scope.h
D third_party/blink/renderer/core/layout/style_retain_scope_test.cc
M third_party/blink/renderer/core/layout/text_autosizer.cc
M third_party/blink/renderer/core/page/print_context.cc
M third_party/blink/renderer/core/paint/ng/ng_decorating_box.h
M third_party/blink/renderer/core/paint/ng/ng_highlight_painter.cc
M third_party/blink/renderer/core/paint/ng/ng_highlight_painter.h
M third_party/blink/renderer/core/paint/ng/ng_inline_paint_context.h
M third_party/blink/renderer/core/paint/outline_painter_test.cc
M third_party/blink/renderer/core/paint/paint_layer.cc
M third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc
M third_party/blink/renderer/core/style/build.gni
M third_party/blink/renderer/core/style/computed_style.cc
M third_party/blink/renderer/core/style/computed_style.h
M third_party/blink/renderer/core/style/computed_style_diff_functions.json5
M third_party/blink/renderer/core/style/computed_style_extra_fields.json5
M third_party/blink/renderer/core/style/computed_style_test.cc
M third_party/blink/renderer/core/style/data_ref.h
M third_party/blink/renderer/core/style/fill_layer.cc
M third_party/blink/renderer/core/style/fill_layer.h
M third_party/blink/renderer/core/style/list_style_type_data.h
M third_party/blink/renderer/core/style/member_copy.h
M third_party/blink/renderer/core/style/style_base_data.cc
M third_party/blink/renderer/core/style/style_base_data.h
M third_party/blink/renderer/core/style/style_cached_data.h
D third_party/blink/renderer/core/style/style_filter_data.cc
D third_party/blink/renderer/core/style/style_filter_data.h
M third_party/blink/renderer/core/style/style_highlight_data.cc
M third_party/blink/renderer/core/style/style_highlight_data.h
M third_party/blink/renderer/core/svg/svg_element.cc
M third_party/blink/renderer/core/svg/svg_element.h
M third_party/blink/renderer/core/svg/svg_element_rare_data.cc
M third_party/blink/renderer/core/svg/svg_element_rare_data.h
M third_party/blink/renderer/core/testing/internals.cc
M third_party/blink/renderer/core/view_transition/view_transition_pseudo_element_base.cc
M third_party/blink/renderer/core/view_transition/view_transition_pseudo_element_base.h
M third_party/blink/renderer/modules/animationworklet/worklet_animation_test.cc
M third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.cc
M third_party/blink/renderer/modules/csspaint/nativepaint/background_color_paint_definition_test.cc
M third_party/blink/renderer/modules/formatted_text/formatted_text.cc
M third_party/blink/renderer/modules/formatted_text/formatted_text_run.cc
177 files changed, 1,191 insertions(+), 1,550 deletions(-)
Anders Hartvoll Ruud (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen, Steinar H Gunderson.
Patch set 62:Code-Review +1
4 comments:
Patchset:
PS62 lgtm
At this point it makes sense to also get a +1 from sesse@ before landing to make it super-clear that he's OK with the CL from a perf/memory standpoint.
File third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.h.tmpl:
Patch Set #52, Line 190:
void TraceAfterDispatch(Visitor* visitor) const {The inheritance hierarchy is: […]
Wouldn't it be more "normal" to trace the stuff which belongs to ComputedStyleBase in ComputedStyleBase::Trace, and then leave ComputedStyleBase::TraceAfterDispatch empty (if it needs to exist at all)?
File third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder.cc:
Patch Set #62, Line 199:
&styleIsn't this taking the address of the Member itself now? `DCHECK(style)`?
File third_party/blink/renderer/core/style/computed_style_extra_fields.json5:
Patch Set #62, Line 485:
inherited->highlight-dataLifting highlight data out of raredata abyss seems like an unrelated/unnecessary change in this CL? Perhaps `*->highlight-data`?
To view, visit change 4346534. To unsubscribe, or for help writing mail filters, visit settings.
Steinar H Gunderson (Gerrit)
Attention is currently required from: Anton Bikineev, Ian Kilpatrick, Kentaro Hara, Michael Lippautz, Rune Lillesveen.
Patch set 62:Code-Review +1
1 comment:
Patchset:
+1 for perf/RAM (of course I wouldn't mind someone figuring out the final barrier and GC bits, but this is already a net win from what I can see)