Issue metadata
Sign in to add a comment
|
12.2% regression in blink_perf.layout at 597646:597695 |
||||||||||||||||||||||
Issue descriptionSplitting off from bug 893885, maybe separate root cause.
,
Oct 10
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/101328eae40000
,
Oct 12
📍 Found significant differences after each of 4 commits. https://pinpoint-dot-chromeperf.appspot.com/job/101328eae40000 Revert "Fix bugs and improve perf in Touch Bar text suggestions." by avi@chromium.org https://chromium.googlesource.com/chromium/src/+/6068cf6b0b7348f09085a3256588f831653b4a52 fixed-grid-lots-of-data: 544.5 → 553.4 (+8.895) Reland "[CI] Group HasLayer conditions together in NeedsEffect" by wangxianzhu@chromium.org https://chromium.googlesource.com/chromium/src/+/173a5dada833bc96df1dcf54cd0bd547fa1e3d18 fixed-grid-lots-of-data: 552.8 → 566.3 (+13.55) [LayoutNG] Separate code for computing visual and layout overflows. by chrishtr@chromium.org https://chromium.googlesource.com/chromium/src/+/2231aea5dcda383dd2c924e7750a02a3f88c783c fixed-grid-lots-of-data: 573.6 → 509.9 (-63.68) [PE] Check and fix under-invalidation of GeometryMapper cache by wangxianzhu@chromium.org https://chromium.googlesource.com/chromium/src/+/9cdbb8acce4b90a37cf9f022bda2be2e6d9215a5 fixed-grid-lots-of-data: 499 → 482.4 (-16.64) Understanding performance regressions: http://g.co/ChromePerformanceRegressions Benchmark documentation link: https://bit.ly/blink-perf-benchmarks
,
Oct 12
The third CL seems to cause the most regression. The first two are actually progressions.
,
Oct 12
,
Oct 12
,
Oct 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3fc2105ef4ed16f850f9f20c9939ecba6a21a77b commit 3fc2105ef4ed16f850f9f20c9939ecba6a21a77b Author: Chris Harrelson <chrishtr@chromium.org> Date: Tue Oct 16 22:39:47 2018 Optimize visual overflow computation code by devirtualizing some methods. Before this patch, ComputeVisualOverflow() was virtual, and so was AddVisualOverflowFromChildren(), which was called by ComputeVisualOverflow(). This pattern existed to share a bit more code with LayoutBlock other than the code to collect rects from children. This CL instead inlines the duplicated code in a few places and de-virtualizes AddVisualOverflowFromChildren. Bug: 894244 Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng Change-Id: Ic9e36c0271ac6d88ee660e9113e1a2b7db8e0148 Reviewed-on: https://chromium-review.googlesource.com/c/1281234 Reviewed-by: Stephen Chenney <schenney@chromium.org> Commit-Queue: Chris Harrelson <chrishtr@chromium.org> Cr-Commit-Position: refs/heads/master@{#600156} [modify] https://crrev.com/3fc2105ef4ed16f850f9f20c9939ecba6a21a77b/third_party/blink/renderer/core/layout/layout_block.h [modify] https://crrev.com/3fc2105ef4ed16f850f9f20c9939ecba6a21a77b/third_party/blink/renderer/core/layout/layout_block_flow.cc [modify] https://crrev.com/3fc2105ef4ed16f850f9f20c9939ecba6a21a77b/third_party/blink/renderer/core/layout/layout_list_item.cc [modify] https://crrev.com/3fc2105ef4ed16f850f9f20c9939ecba6a21a77b/third_party/blink/renderer/core/layout/layout_list_item.h [modify] https://crrev.com/3fc2105ef4ed16f850f9f20c9939ecba6a21a77b/third_party/blink/renderer/core/layout/layout_multi_column_set.cc [modify] https://crrev.com/3fc2105ef4ed16f850f9f20c9939ecba6a21a77b/third_party/blink/renderer/core/layout/layout_multi_column_set.h [modify] https://crrev.com/3fc2105ef4ed16f850f9f20c9939ecba6a21a77b/third_party/blink/renderer/core/layout/layout_table.cc [modify] https://crrev.com/3fc2105ef4ed16f850f9f20c9939ecba6a21a77b/third_party/blink/renderer/core/layout/layout_table.h [modify] https://crrev.com/3fc2105ef4ed16f850f9f20c9939ecba6a21a77b/third_party/blink/renderer/core/layout/layout_text_control_single_line.cc [modify] https://crrev.com/3fc2105ef4ed16f850f9f20c9939ecba6a21a77b/third_party/blink/renderer/core/layout/layout_text_control_single_line.h [modify] https://crrev.com/3fc2105ef4ed16f850f9f20c9939ecba6a21a77b/third_party/blink/renderer/core/layout/ng/layout_ng_mixin.cc [modify] https://crrev.com/3fc2105ef4ed16f850f9f20c9939ecba6a21a77b/third_party/blink/renderer/core/layout/ng/layout_ng_mixin.h
,
Dec 1
I've investigated this. We are hitting an edge case that make this test perform exceptionally badly. # Test case: Simplified test case: <body display:grid> <div width:200, height:200> repeated 2000 times </body> Running test changes body's width, forces layout. This causes two layouts: - once with vertical scrollbar. - second time with horizontal scrollbar. Each time, body computes overflow by traversing all of its 2000 children. With this patch, instead of doing this loop once, it happens twice, once for VisualOverflow, once for LayoutOverflow. On my linux box, time taken is: LayoutBlock::AddLayoutOverflowFromBlockChildren 8.57% LayoutBlock::AddVisualOverflowFromBlockChildren 5.37% # Analysis I believe most of the performance hit comes from doing 2000 Elements traversal twice as often: Old code path: LayoutBlock::AddOverflowFromBlockChildren Relayout LayoutBlock::AddOverflowFromBlockChildren New code path: LayoutBlock::AddLayoutOverflowFromBlockChildren LayoutBlock::AddVisualOverflowFromBlockChildren Relayout LayoutBlock::AddLayoutOverflowFromBlockChildren LayoutBlock::AddVisualOverflowFromBlockChildren I have not run the tests on Android. Linux results do not support 12% regression, since the entire traversal takes ~14%. Linux results would support 7% regression. Additional Android regression might be caused by cache misses. Once computation of VisualOverflow is fully moved to PaintLayer, visual overflow will only have to be computed once before paint: After move to PaintLayer: LayoutBlock::AddLayoutOverflowFromBlockChildren Relayout LayoutBlock::AddLayoutOverflowFromBlockChildren LayoutBlock::AddVisualOverflowFromBlockChildren This should improve performance. It will not be as fast as it was before, because we'd still need one additional traversal before paint. This extra traversal cannot be avoided. # Recommendations There is no easy fix. Be aware that overflow computation is on the hot path.
,
Dec 3
I agree with this conclusion. WontFix. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Oct 10