New issue
Advanced search Search tips

Issue 818709 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: May 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

3%-11.4% regression in smoothness.sync_scroll.key_mobile_sites_smooth at 540732:540754

Project Member Reported by rmcilroy@chromium.org, Mar 5 2018

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=818709

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


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

android-nexus5
android-webview-nexus5X
android-webview-nexus6
Cc: chrishtr@chromium.org skobes@chromium.org
Owner: chrishtr@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/1288b712440000

[RLS] Re-land: skip compositing steps on frame scroll. by chrishtr@chromium.org
https://chromium.googlesource.com/chromium/src/+/9732521599b291a0703830c0d9d94c381916b7d8

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3341c5a4883f580df6e4ae0138ca4b514b8ff2bc

commit 3341c5a4883f580df6e4ae0138ca4b514b8ff2bc
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Tue Mar 13 00:23:04 2018

Devirtualize ScrollableArea::UsesCompositedScrolling.

This method is called in various hot code paths, in particular
in ObjectPaintInvalidator.

Bug:  818709 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I6eaf1acadc5ac5721c7e42c900964bcc31fee769
Reviewed-on: https://chromium-review.googlesource.com/959155
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: vmpstr <vmpstr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542665}
[modify] https://crrev.com/3341c5a4883f580df6e4ae0138ca4b514b8ff2bc/third_party/WebKit/Source/core/frame/LocalFrameView.cpp
[modify] https://crrev.com/3341c5a4883f580df6e4ae0138ca4b514b8ff2bc/third_party/WebKit/Source/core/frame/LocalFrameView.h
[modify] https://crrev.com/3341c5a4883f580df6e4ae0138ca4b514b8ff2bc/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
[modify] https://crrev.com/3341c5a4883f580df6e4ae0138ca4b514b8ff2bc/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h
[modify] https://crrev.com/3341c5a4883f580df6e4ae0138ca4b514b8ff2bc/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMapping.cpp
[modify] https://crrev.com/3341c5a4883f580df6e4ae0138ca4b514b8ff2bc/third_party/WebKit/Source/core/paint/compositing/PaintLayerCompositor.cpp
[modify] https://crrev.com/3341c5a4883f580df6e4ae0138ca4b514b8ff2bc/third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp
[modify] https://crrev.com/3341c5a4883f580df6e4ae0138ca4b514b8ff2bc/third_party/WebKit/Source/platform/scroll/ScrollableArea.h

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e54f71ef43dee5f77d71aa53a626d1292ee637b

commit 1e54f71ef43dee5f77d71aa53a626d1292ee637b
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Tue Mar 13 00:24:54 2018

[PE] Don't call CompositedScrollsWithRespectTo during raster invalidation

It is redundant with the call to UsesCompositedScrolling just below, and
is equivalent to UsesCompositedScrolling + object != paint_invalidation_container.
UsesCompositedScrolling calls two methods plus a virtual, so may be slow.

Bug:  818709 

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Iab617e3941563ac1994e5732359cf691442a2b7e
Reviewed-on: https://chromium-review.googlesource.com/959444
Commit-Queue: vmpstr <vmpstr@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542667}
[modify] https://crrev.com/1e54f71ef43dee5f77d71aa53a626d1292ee637b/third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp

On this page:

https://www.google.co.uk/search?hl=en&q=barack+obama&cad=h

It looks like there is a 33% slowdown in PrePaint, when testing on a mid-range
Android phone. This appears to account for the bulk of the regression.

Note that the profile for this page is quite different on mobile and on desktop,
I think due to URL bar hiding.
I have profiled locally on desktop. Two problems:

1. Increased cost of ::UpdateForChildren in PaintPropertyTreeBuilder.cpp,
mostly due to expensive calls to LayoutBox::ScrolledContentOffset, but also
a bit of other stuff.

2. Increased cost of PaintInvalidator::InvalidatePaint. Haven't dug into that
yet.

Comment 8 by skobes@chromium.org, Mar 16 2018

BTW it appears that, despite r542665, ScrollableArea::UsesCompositedScrolling is still virtual.
Oops. Will fix now.
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0499ebf8339aea9b4c86b4a374b18c836f7b7121

commit 0499ebf8339aea9b4c86b4a374b18c836f7b7121
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Fri Mar 16 19:24:10 2018

[RLS] Micro-optimize ScrolledContentOffset and IsFixedPositionObjectInPagedMedia.

LayoutBox::ScrolledContentOffset can be much simpler for LayoutViews, and
also for LayoutBoxes that are not LayoutFlexibleBox. To tease this apart,
this patch makes ScrolledContentOffset virtual to provide the more expensive
implementation only when needed.

Also, check for fixed-position in IsFixedPositionObjectInPagedMedia
before finding the View(), which may be expensive because it involves ping-
ponging through Document.

Bug:818709

Change-Id: I8470b7c1487b3fbf4f227aad21e8f9da81c1e9bb
Reviewed-on: https://chromium-review.googlesource.com/964933
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543781}
[modify] https://crrev.com/0499ebf8339aea9b4c86b4a374b18c836f7b7121/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/0499ebf8339aea9b4c86b4a374b18c836f7b7121/third_party/WebKit/Source/core/layout/LayoutBox.h
[modify] https://crrev.com/0499ebf8339aea9b4c86b4a374b18c836f7b7121/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp
[modify] https://crrev.com/0499ebf8339aea9b4c86b4a374b18c836f7b7121/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h
[modify] https://crrev.com/0499ebf8339aea9b4c86b4a374b18c836f7b7121/third_party/WebKit/Source/core/layout/LayoutObject.cpp
[modify] https://crrev.com/0499ebf8339aea9b4c86b4a374b18c836f7b7121/third_party/WebKit/Source/core/layout/LayoutView.cpp
[modify] https://crrev.com/0499ebf8339aea9b4c86b4a374b18c836f7b7121/third_party/WebKit/Source/core/layout/LayoutView.h

Project Member

Comment 11 by bugdroid1@chromium.org, Mar 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/def2da31e92ed904c370231eacce9710707b95f3

commit def2da31e92ed904c370231eacce9710707b95f3
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Fri Mar 16 19:58:48 2018

[RLS] Actually make SA::UsesCompositedScrolling non-virtual.

This was an oversight in a previous CL.

Bug:  818709 
Change-Id: I2697d136be7af56e00f342f77451b765a37c8eaa
Reviewed-on: https://chromium-review.googlesource.com/966796
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543795}
[modify] https://crrev.com/def2da31e92ed904c370231eacce9710707b95f3/third_party/WebKit/Source/platform/scroll/ScrollableArea.h

Project Member

Comment 12 by bugdroid1@chromium.org, May 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/82812cf386713a67d3d1360be9b48b104b20d93f

commit 82812cf386713a67d3d1360be9b48b104b20d93f
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Wed May 02 03:47:11 2018

[RLS] Use faster fast-path when updating composited root scroller offset.

Previously, we would update all of the composited layers plus reach into the
LayerTreeHost (via some virtuals) to update even more things. All of these
will be updated anyway if the composited layer configurations actually change,
and it's pure overhead here.

PaintLayerCompositor::FrameViewDidScroll does the same thing (and this is
where I cribbed from in my original patch), but it's overkill. I didn't
change that call site, however, since it is non-RLS specific.

Bug:818709

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I3643e42bc4b68bafcfd06dfe787a18b09ee2c7da
Reviewed-on: https://chromium-review.googlesource.com/1038439
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Stefan Zager <szager@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555302}
[modify] https://crrev.com/82812cf386713a67d3d1360be9b48b104b20d93f/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.cc
[modify] https://crrev.com/82812cf386713a67d3d1360be9b48b104b20d93f/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.h
[modify] https://crrev.com/82812cf386713a67d3d1360be9b48b104b20d93f/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc

Status: WontFix (was: Assigned)
I was finally able to reproduce the regression. On a Nexus 5, I was able to
observe a difference in frame_times of 20ms before patch and 24ms after patch.

When looking at the resulting traces, I think I understand what is happening.
Before the patch, there are 3 long updates at the beginning of the scroll, each
of which is 14ms. After, there is only 1 and it is 29ms.

Subsequent main frame computations are about the same cost in both cases.

Therefore this is not a real regression.

Sign in to add a comment