New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 798378 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

14.1%-14.3% regression in blink_perf.events at 526391:526403

Project Member Reported by alexclarke@chromium.org, Jan 2 2018

Issue description

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

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


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

chromium-rel-mac-retina
chromium-rel-mac12-mini-8gb
๐Ÿ“ Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/16bc935d040000
Cc: chrishtr@chromium.org
Owner: chrishtr@chromium.org
Status: Assigned (was: Untriaged)
๐Ÿ“ Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/16bc935d040000

Revert "Revert "[CI] Migrate hit-testing code in PaintLayer to use GeometryMapper.""
By chrishtr@chromium.org ยท Sat Dec 30 01:02:37 2017
chromium @ d45f384824617447c7e4e8808b4946230dab58e2

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
One of the largest costs is the time to run ConvertToLayerCoords, which
involves a DOM tree walk.

This cost is also present in the non-GeometryMapper code, but there is
an opportunity to get rid of it now that we have GeometryMapper::SourceToDestinationRect and fragment PaintOffsets. Trying
that.
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 5 2018

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

commit ed23c5622a815ec34ef4160db567cf83bccfcc41
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Fri Jan 05 21:08:24 2018

[PE] Replace PaintLayer::ConvertToLayerCords with GeometryMapper::SourceToDestinationRect.

This is a large performance boost when computing offsets of PaintLayers far from root_layer,
since ConvertToLayerCoords walks up the LayoutObject tree to do this (since finding the
containing-block PaintLayer involves calling ContainingLayer()).

In particular, it shows up in a big way in the performance test referenced in  issue 798378 .

Bug:  798378 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ic11ab49add8d9e9c53b0e0e521844b4405db144b
Reviewed-on: https://chromium-review.googlesource.com/849417
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Tien-Ren Chen <trchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527380}
[modify] https://crrev.com/ed23c5622a815ec34ef4160db567cf83bccfcc41/third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp

This CL did not impact anything in the metric. :( Back to looking at traces..
Cc: rdevlin....@chromium.org dpa...@chromium.org scottchen@chromium.org eroman@chromium.org wangxianzhu@chromium.org rsleevi@chromium.org dschuyler@chromium.org
 Issue 798375  has been merged into this issue.
 Issue 798380  has been merged into this issue.
https://chromium.googlesource.com/chromium/src/+/8b48d59ae898aa04bb10315475a39ce34e701e7a

caused a clear improvement on desktop platforms. Unfortunately, no movement
on Android though. Also, on desktop, the regression has not been fully
recovered.
It does seem to help on non-webview Android though.
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 26 2018

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

commit 484efec4dd6d485370023b7465b92b340128d2f2
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Fri Jan 26 18:48:00 2018

[PE] Reduce copies of LayoutRect in PaintLayerClipper.

Bug:  798378 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ic3c0b29430b06e76a3c81e654bc780e4a969592d
Reviewed-on: https://chromium-review.googlesource.com/880403
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532031}
[modify] https://crrev.com/484efec4dd6d485370023b7465b92b340128d2f2/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/484efec4dd6d485370023b7465b92b340128d2f2/third_party/WebKit/Source/core/layout/LayoutBox.h
[modify] https://crrev.com/484efec4dd6d485370023b7465b92b340128d2f2/third_party/WebKit/Source/core/layout/LayoutTable.cpp
[modify] https://crrev.com/484efec4dd6d485370023b7465b92b340128d2f2/third_party/WebKit/Source/core/layout/LayoutTable.h
[modify] https://crrev.com/484efec4dd6d485370023b7465b92b340128d2f2/third_party/WebKit/Source/core/layout/LayoutView.cpp
[modify] https://crrev.com/484efec4dd6d485370023b7465b92b340128d2f2/third_party/WebKit/Source/core/layout/LayoutView.h
[modify] https://crrev.com/484efec4dd6d485370023b7465b92b340128d2f2/third_party/WebKit/Source/core/paint/ClipRect.h
[modify] https://crrev.com/484efec4dd6d485370023b7465b92b340128d2f2/third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp

Project Member

Comment 12 by bugdroid1@chromium.org, Jan 29 2018

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

commit 3d21dce21361e269a791e626e14a0643a5ee5e18
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Mon Jan 29 19:42:16 2018

Revert "[PE] Reduce copies of LayoutRect in PaintLayerClipper."

This reverts commit 484efec4dd6d485370023b7465b92b340128d2f2.

Reason for revert: Makes performance worse on the bots, not better.
Unclear why.

Original change's description:
> [PE] Reduce copies of LayoutRect in PaintLayerClipper.
> 
> Bug:  798378 
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
> Change-Id: Ic3c0b29430b06e76a3c81e654bc780e4a969592d
> Reviewed-on: https://chromium-review.googlesource.com/880403
> Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
> Reviewed-by: Stephen Chenney <schenney@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#532031}

TBR=chrishtr@chromium.org,schenney@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  798378 
Change-Id: I8c6ec640efffe8cd7e2adaacd2ecc47d469794bb
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Reviewed-on: https://chromium-review.googlesource.com/891559
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532548}
[modify] https://crrev.com/3d21dce21361e269a791e626e14a0643a5ee5e18/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/3d21dce21361e269a791e626e14a0643a5ee5e18/third_party/WebKit/Source/core/layout/LayoutBox.h
[modify] https://crrev.com/3d21dce21361e269a791e626e14a0643a5ee5e18/third_party/WebKit/Source/core/layout/LayoutTable.cpp
[modify] https://crrev.com/3d21dce21361e269a791e626e14a0643a5ee5e18/third_party/WebKit/Source/core/layout/LayoutTable.h
[modify] https://crrev.com/3d21dce21361e269a791e626e14a0643a5ee5e18/third_party/WebKit/Source/core/layout/LayoutView.cpp
[modify] https://crrev.com/3d21dce21361e269a791e626e14a0643a5ee5e18/third_party/WebKit/Source/core/layout/LayoutView.h
[modify] https://crrev.com/3d21dce21361e269a791e626e14a0643a5ee5e18/third_party/WebKit/Source/core/paint/ClipRect.h
[modify] https://crrev.com/3d21dce21361e269a791e626e14a0643a5ee5e18/third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp

Project Member

Comment 13 by bugdroid1@chromium.org, Jan 30 2018

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

commit 65ce0f7c0d647161744517f25015e7308956b0b6
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Tue Jan 30 20:43:55 2018

[PE] Stop ref-counting PropertyTreeState in most cases.

Only PaintChunks need ref-counted PropertyTreeState objects, because they
are the only ones that store the objects in a persistent manner. Further
the performance overhead of RefPtr for other use-cases is too big to
justify.

Bug:798378

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Iac4d4e1bcf0e377033ab9c129f290e0745830343
Reviewed-on: https://chromium-review.googlesource.com/892561
Reviewed-by: Tien-Ren Chen <trchen@chromium.org>
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533012}
[modify] https://crrev.com/65ce0f7c0d647161744517f25015e7308956b0b6/third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp
[modify] https://crrev.com/65ce0f7c0d647161744517f25015e7308956b0b6/third_party/WebKit/Source/core/paint/SVGPaintContext.cpp
[modify] https://crrev.com/65ce0f7c0d647161744517f25015e7308956b0b6/third_party/WebKit/Source/platform/BUILD.gn
[modify] https://crrev.com/65ce0f7c0d647161744517f25015e7308956b0b6/third_party/WebKit/Source/platform/graphics/compositing/CompositedLayerRasterInvalidator.cpp
[modify] https://crrev.com/65ce0f7c0d647161744517f25015e7308956b0b6/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
[modify] https://crrev.com/65ce0f7c0d647161744517f25015e7308956b0b6/third_party/WebKit/Source/platform/graphics/compositing/PaintChunksToCcLayer.cpp
[modify] https://crrev.com/65ce0f7c0d647161744517f25015e7308956b0b6/third_party/WebKit/Source/platform/graphics/paint/GeometryMapperTest.cpp
[modify] https://crrev.com/65ce0f7c0d647161744517f25015e7308956b0b6/third_party/WebKit/Source/platform/graphics/paint/PaintChunkProperties.h
[modify] https://crrev.com/65ce0f7c0d647161744517f25015e7308956b0b6/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h
[add] https://crrev.com/65ce0f7c0d647161744517f25015e7308956b0b6/third_party/WebKit/Source/platform/graphics/paint/RefCountedPropertyTreeState.cpp
[add] https://crrev.com/65ce0f7c0d647161744517f25015e7308956b0b6/third_party/WebKit/Source/platform/graphics/paint/RefCountedPropertyTreeState.h

Status: Fixed (was: Assigned)
Hurray! The whole regression is now gone after my latest patch.

Sign in to add a comment