New issue
Advanced search Search tips

Issue 595043 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression

Blocking:
issue 601270



Sign in to add a comment

6.4%-14.9% regression in power.top_25 at 381082:381154

Project Member Reported by pras...@chromium.org, Mar 15 2016

Issue description

See the link to graphs below.
 
Project Member

Comment 2 by 42576172...@developer.gserviceaccount.com, Mar 15 2016

Cc: erikc...@chromium.org
Owner: erikc...@chromium.org

=== Auto-CCing suspected CL author erikchen@chromium.org ===

Hi erikchen@chromium.org, the bisect results pointed to your CL below as possibly
causing a regression. Please have a look at this info and see whether
your CL be related.


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : base: Implement GetCurrentThreadPriority.
Author  : erikchen
Commit description:
  
The Chrome thread priority is saved in the thread dictionary during
SetCurrentThreadPriority(). This seemed a better approach than using
thread_policy_get(), since there are 4 Chrome priorities which won't cleanly map
to thread flavors.

BUG=554651

Review URL: https://codereview.chromium.org/1620673003

Cr-Commit-Position: refs/heads/master@{#381102}
Commit  : 42655dd168d884f60c0194e8a74606c68a3e6378
Date    : Mon Mar 14 22:55:13 2016


===== TESTED REVISIONS =====
Revision                Mean Value  Std. Dev.   Num Values  Good?
chromium@381101         235.901316  1.403099    8           good
chromium@381102         274.136842  0.976596    5           bad
chromium@381103         272.705263  0.93085     5           bad
chromium@381104         273.557895  1.234657    5           bad
chromium@381106         272.855263  0.861501    8           bad

Bisect job ran on: mac_10_10_perf_bisect
Bug ID: 595043

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --also-run-disabled-tests startup.warm.blank_page
Test Metric: open_tabs_time/open_tabs_time
Relative Change: 15.65%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_10_10_perf_bisect/builds/2041
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9018101899020156640


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with label Cr-Tests-AutoBisect.  Thank you!
Cc: nyerramilli@chromium.org
Labels: TE-Triaged
based on comment#2, erikchen@ could you please check the issue.
Not sure why the startup regressions are lumped in with the power regressions? I kicked off a bisect on the power regressions, will split them if they come back with a different cause.

Erik, does it make sense that your CL could have caused a startup regression?
Possible tangent:
I ran a bisect for  Issue 598431  which seems to have automatically resolved by merging into this issue. I don't see any comment here about the merged bug, nor do I see a comment in  Issue 598431  as to how it was resolved as duplicate. Does the new bug tracker not post a comment when someone marks a duplicate? Seems odd... maybe a bug with Monorail?
Project Member

Comment 6 by 42576172...@developer.gserviceaccount.com, Mar 29 2016


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : base: Implement GetCurrentThreadPriority.
Author  : erikchen
Commit description:
  
The Chrome thread priority is saved in the thread dictionary during
SetCurrentThreadPriority(). This seemed a better approach than using
thread_policy_get(), since there are 4 Chrome priorities which won't cleanly map
to thread flavors.

BUG=554651

Review URL: https://codereview.chromium.org/1620673003

Cr-Commit-Position: refs/heads/master@{#381102}
Commit  : 42655dd168d884f60c0194e8a74606c68a3e6378
Date    : Mon Mar 14 22:55:13 2016


===== TESTED REVISIONS =====
Revision                Mean Value  Std. Dev.   Num Values  Good?
chromium@381081         1396.0      27.93743    5           good
chromium@381096         1395.6      22.378561   5           good
chromium@381101         1415.125    8.592979    8           good
chromium@381102         1539.2      25.489213   5           bad         <-
chromium@381103         1515.2      58.130887   5           bad
chromium@381104         1555.4      25.234896   5           bad
chromium@381106         1559.2      22.241852   5           bad

Bisect job ran on: mac_10_10_perf_bisect
Bug ID: 595043

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --also-run-disabled-tests power.gpu_rasterization.top_25
Test Metric: idle_wakeups_total/idle_wakeups_total
Relative Change: 11.69%
Score: 98.0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_10_10_perf_bisect/builds/2069
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9016894357641451632


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=595043

| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Tests>AutoBisect.  Thank you!
chcunningham: thanks for pointing that out! I filed https://bugs.chromium.org/p/monorail/issues/detail?id=1190 about the lack of commenting on the bug duplication.

erikchen: The bisects say this cl did regress both the startup time and the power metrics. Note that the power metrics which regressed are idle wakeups; those correlate to power usage often but they're not a perfect proxy. In this case, I don't see the corresponding energy_consumption_mwh metrics increasing, so it's not clear to me whether the power metrics are worth chasing.
Cc: robliao@chromium.org gab@chromium.org
My changes to paltform_thread_mac.mm should have no functional change, so it must have been my changes to components/startup_metric_utils/browser/startup_metric_utils.cc. I can revert those changes - robliao and gab will need to figure out why their startup metrics are introducing a performance regression.
CL up. Let's confirm that the regression goes away after it lands.

https://codereview.chromium.org/1838173003/
Project Member

Comment 11 by bugdroid1@chromium.org, Mar 30 2016

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

commit a5ce8d5bc18a08f44528b32ee8507dbb6982cf7d
Author: erikchen <erikchen@chromium.org>
Date: Wed Mar 30 22:10:35 2016

Revert changes to startup_metric_utils.cc.

I recently enabled some code on OS X that lets startup_metric_utils.cc set the
thread priority (https://codereview.chromium.org/1620673003/). This is most
likely responsible for a performance regression.

BUG= 595043 

Review URL: https://codereview.chromium.org/1838173003

Cr-Commit-Position: refs/heads/master@{#384113}

[modify] https://crrev.com/a5ce8d5bc18a08f44528b32ee8507dbb6982cf7d/components/startup_metric_utils/browser/startup_metric_utils.cc

Status: Fixed (was: Assigned)
The regression looks recovered, right around 384113. I attached a screenshot.


Screen Shot 2016-04-04 at 2.56.15 PM.png
135 KB View Download

Comment 13 by gab@chromium.org, Apr 7 2016

Re #9: The only difference pre-revert and post-revert is the lack of a call to Mac's base::PlatformThread::GetCurrentThreadPriority() and base::PlatformThread::SetCurrentThreadPriority(), everything else is the same. So it appears it's not our metrics code that introduces this regression but potentially the Mac impl of these calls (especially since this is a static init that should only happen once per process)? This might be catching a bigger problem with the Mac impl and I think the Mac team should dig to the bottom of that.

Shall we open another bug for the Mac team to dig into the performance of PlatformThread's priority calls?
That sounds reasonable to me.

Comment 15 by gab@chromium.org, Apr 7 2016

Blocking: 601270

Sign in to add a comment