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

Issue 821773 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Mar 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

11.6% regression in thread_times.key_silk_cases at 542622:542688

Project Member Reported by hjd@google.com, Mar 14 2018

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Mar 14 2018

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=821773

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


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

android-nexus5X
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Mar 14 2018

Cc: pdr@chromium.org trchen@chromium.org
Owner: trchen@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/14a4b81e440000

[Blink] Make PaintLayer::AncestorDependentCompositingInputs in-place by trchen@chromium.org
https://chromium.googlesource.com/chromium/src/+/0d2f4351ca12d64119d1aed52faf538a852a12c0

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

Comment 4 by trchen@chromium.org, Mar 15 2018

 Issue 821775  has been merged into this issue.

Comment 5 by trchen@chromium.org, Mar 19 2018

Cc: vmi...@chromium.org
The graph has been discontinued for a while: https://chromeperf.appspot.com/report?sid=61010c6d4b0b938b0e5568bc5a05f0f347043f98b6b517f011d63afdcc08be5c

Do you know what happened, Victor?

I have a speculative fix, and I would like to verify it actually fix the regression with graphs to support it. Thanks!

Comment 6 by hjd@chromium.org, Mar 20 2018

Could it have been this bug?: https://bugs.chromium.org/p/chromium/issues/detail?id=823315 (also seems to have come back now)

Comment 7 by vmi...@chromium.org, Mar 20 2018

Yes, I think hjd@ is right.  Some thread renaming broke this measurement, but it is back now.
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 26 2018

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

commit 28f75e04b5a1091303e3bddd9621ee04df91ac4d
Author: Tien-Ren Chen <trchen@chromium.org>
Date: Mon Mar 26 23:15:31 2018

[Blink] Make PaintLayer::AncestorDependentCompositingInput external again

A previous CL
https://chromium.googlesource.com/chromium/src/+/0d2f4351ca12d64119d1aed52faf538a852a12c0
embedded the structure to PaintLayer itself, in order to avoid the cost
of heap allocation. However that regressed transform animation with lots
of layers on a number of Android devices. It is speculated that the
increase in PaintLayer size put more pressure on L2 cache.

This CL changes the structure back to external again. Note that this is not
equivalent to reverting the previous CL. Before the regressed CL, the
structure is reallocated everytime we updated it. With this CL applied,
it will become a lazy rare data that will be allocated on first use,
but then reused throughout PaintLayer's lifetime.

BUG= 821773 

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ia65b8dba8c4d07eed22aed9903a57472ad06960f
Reviewed-on: https://chromium-review.googlesource.com/974282
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Tien-Ren Chen <trchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545882}
[modify] https://crrev.com/28f75e04b5a1091303e3bddd9621ee04df91ac4d/third_party/WebKit/Source/core/paint/PaintLayer.cpp
[modify] https://crrev.com/28f75e04b5a1091303e3bddd9621ee04df91ac4d/third_party/WebKit/Source/core/paint/PaintLayer.h

Comment 9 by trchen@chromium.org, Mar 27 2018

Status: Fixed (was: Assigned)
The nexus5x graph showed that we recovered much of the regression.

Sign in to add a comment