New issue
Advanced search Search tips

Issue 894244 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Dec 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

12.2% regression in blink_perf.layout at 597646:597695

Project Member Reported by sullivan@chromium.org, Oct 10

Issue description

Splitting off from bug 893885, maybe separate root cause.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=894244

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=3078756be3392ae3d75863d435d35caf598512c9e63b6dee3ba45c8fcba61be1


Bot(s) for this bug's original alert(s):

Android Nexus5X WebView Perf

blink_perf.layout - Benchmark documentation link:
  https://bit.ly/blink-perf-benchmarks
Cc: a...@chromium.org wangxianzhu@chromium.org chrishtr@chromium.org
Owner: wangxianzhu@chromium.org
Status: Assigned (was: Untriaged)
📍 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
Components: Blink>Layout
Owner: chrishtr@chromium.org
The third CL seems to cause the most regression. The first two are actually progressions. 
Mergedinto: 893897
Status: Duplicate (was: Assigned)
Status: Assigned (was: Duplicate)
Project Member

Comment 7 by bugdroid1@chromium.org, 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

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.
Status: WontFix (was: Assigned)
I agree with this conclusion. WontFix.

Sign in to add a comment