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

Issue 648904 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Last visit > 30 days ago
Closed: Sep 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

1.9% regression in thread_times.key_idle_power_cases at 419080:419159

Project Member Reported by toyoshim@chromium.org, Sep 21 2016

Issue description

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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg-bKBugoM


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

android-galaxy-s5
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Sep 21 2016

Cc: jkrcal@chromium.org
Owner: jkrcal@chromium.org

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

Hi jkrcal@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 : Change the default value of the fetching_requires_signin parameter
Author  : jkrcal
Commit description:
  
With the current default value fetching_requires_signin=true, users get
suboptimal UX after fresh install / after clearing app data. Because of
bug 632199, the server-side variation parameters are not applied in the
first run and sign-in is required. This is inconsistent with the second
run of Chrome, where the server sets fetching_requires_signin:=false
and no sign-in is required any more.

BUG=646759

Review-Url: https://codereview.chromium.org/2343583002
Cr-Commit-Position: refs/heads/master@{#419147}
Commit  : c9be4526b76c9325ad29996f1eff185e3ff87535
Date    : Fri Sep 16 12:34:59 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev     N  Good?
chromium@419079  2.88354  0.00935989  5  good
chromium@419119  2.90067  0.00574241  5  good
chromium@419139  2.88216  0.00949536  5  good
chromium@419144  2.89322  0.00828958  8  good
chromium@419146  2.88737  0.00893155  5  good
chromium@419147  2.92787  0.0113552   8  bad    <--
chromium@419149  2.9354   0.00611362  5  bad
chromium@419159  2.9323   0.0169981   5  bad

Bisect job ran on: android_s5_perf_bisect
Bug ID: 648904

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests thread_times.key_idle_power_cases
Test Metric: tasks_per_second_total_all/set-timeout.html (Long Idle)
Relative Change: 1.69%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_s5_perf_bisect/builds/1016
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9000920825393606912


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

| 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!

Comment 4 by jkrcal@chromium.org, Sep 22 2016

Cc: treib@chromium.org tschumann@chromium.org
A change in performance metric is to be expected for a CL that switches on a considerably large feature.

I do not have enough experience to understand the severity of such a change: Tim, Marc, WDYT?
Cc: ntp-dev+bugs@chromium.org
Owner: tschumann@chromium.org
The referenced CL default-enabled the Zine content suggestions for non-signed-in users. I guess the bots are running as non-signed-in and that's why it's the first time our feature shows up.

We'll need to figure out a) how severe this increase is and b) understand if we can optimize our code. 
Cc: -tschumann@chromium.org skyos...@chromium.org
+skyostil Sami, I have a hard time understanding the alert. Can you share some documentation what this metric is precisely measuring? Also, who would be best to talk about the impact and if it's acceptable? 
Status: WontFix (was: Assigned)
This test is making sure different types of pages don't kill the battery while the screen is locked. The one that regressed is checking that a DOM timer becomes throttled in this case.

I checked the traces and the throttling appears to be working correctly after your patch landed, so I think we can ignore this alert.

(In the future consider adding a benchmark that specifically exercises large features like this or turn it on for all of the perf waterfall.)
Thanks for investigating. It would indeed be great to keep monitor performance aspects for our feature. Do you have some pointers for setting up a benchmark or adding them to the perf waterfall?

Thanks!

--Tim
Re #8: since Zine is default-enabled, it is now running on the perfbots, so you will at least get performance regression alerts for all your CLs in that codepath that regress any of Chrome's performance tests.

Sign in to add a comment