Issue metadata
Sign in to add a comment
|
6.4%-14.9% regression in power.top_25 at 381082:381154 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Mar 15 2016
=== 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!
,
Mar 22 2016
based on comment#2, erikchen@ could you please check the issue.
,
Mar 29 2016
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?
,
Mar 29 2016
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?
,
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!
,
Mar 29 2016
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.
,
Mar 29 2016
,
Mar 29 2016
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.
,
Mar 29 2016
CL up. Let's confirm that the regression goes away after it lands. https://codereview.chromium.org/1838173003/
,
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
,
Apr 4 2016
The regression looks recovered, right around 384113. I attached a screenshot.
,
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?
,
Apr 7 2016
That sounds reasonable to me.
,
Apr 7 2016
|
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by pras...@chromium.org
, Mar 15 2016