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

Issue 598025 link

Starred by 0 users

Issue metadata

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



Sign in to add a comment

5.6%-135.4% regression in thread_times.key_idle_power_cases at 383185:383235

Project Member Reported by sullivan@chromium.org, Mar 25 2016

Issue description

See the link to graphs below.
 
Project Member

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

Cc: tedc...@chromium.org
Owner: tedc...@chromium.org

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

Hi tedchoc@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 : Enable url path fading all on Android configurations.
Author  : tedchoc
Commit description:
  
Previously, this was restricted to document mode, but that is
no longer desired and it should be enabled everywhere.

BUG=597147

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

Cr-Commit-Position: refs/heads/master@{#383200}
Commit  : 328129ac349c860993a94ccb1a8944e2f500cc89
Date    : Fri Mar 25 00:30:31 2016


===== TESTED REVISIONS =====
Revision                Mean Value  Std. Dev.   Num Values  Good?
chromium@383186         0.233137    0.002531    5           good
chromium@383199         0.230297    0.003442    5           good
chromium@383200         0.538801    0.007638    5           bad         <-
chromium@383201         0.540028    0.00055     5           bad
chromium@383202         0.538988    0.003622    5           bad
chromium@383205         0.540964    0.003519    5           bad
chromium@383211         0.537979    0.002503    5           bad
chromium@383235         0.539608    0.003815    5           bad

Bisect job ran on: android_one_perf_bisect
Bug ID: 598025

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --also-run-disabled-tests thread_times.key_idle_power_cases
Test Metric: thread_total_all_cpu_time_per_second/blank.html
Relative Change: 131.45%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_one_perf_bisect/builds/1229
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9017190537850110112


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

| 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!
Cc: rolfe@chromium.org
What is this specifically testing?  This change did add a new animation for the URL on every page load, so if we are expecting things to not happen after a page has been loaded, then I definitely think this is expected (granted the 135% is frightening).

FWIW, this has already been the default behavior in document mode for all users on Android L+.

Although, we could also argue that maybe we shouldn't animate on Android one.  Adding rolfe@ for that one.  Should we just not fade out the path on android one?  Hide it instantly?  Or just leave it as is for now?

Comment 4 by rolfe@chromium.org, Mar 27 2016

Guessing no Svelte device runs L + document mode that can conveniently dictate how this is handled?

Barring that instant fade seems the way to go. If that looks horrible, leave up the whole path, but I rather deal with an instant fade if not too jarring to keep the omnibox clear for everything else in it.
Cc: skyos...@chromium.org
Adding skyostil, owner of thread_times.key_idle_power_cases, to answer the questions in #3. The documentation for the benchmark says "Measures timeline metrics for sites that should be idle in foreground and background scenarios. The metrics are per-second rather than per-frame". Docs for the benchmark link here, but I don't see thread_times explained: http://www.chromium.org/developers/design-documents/rendering-benchmarks

The largest increase is 135% in android-one/thread_times.key_idle_power_cases / thread_total_all_cpu_time_per_second / blank.html: https://chromeperf.appspot.com/report?sid=cbe882a438825237fef872a39a1b7bc4fd8d54e5f1c49a392ba683e6b407a78b&rev=383235

Re #4: I don't know anything about document mode, but the android one svelte devices on the waterfall are running L (LMY47W).
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 28 2016

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

commit d070c0ede9bf1faea3a66d91b8f68014c6ae1f67
Author: tedchoc <tedchoc@chromium.org>
Date: Mon Mar 28 22:41:23 2016

Optimize the URL path fading to avoid unnecessary draws when empty.

BUG= 598025 

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

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

[modify] https://crrev.com/d070c0ede9bf1faea3a66d91b8f68014c6ae1f67/chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlContainer.java

Just hiding the path on svelte does look horrible without the animation.

I did find that we were over-zealously animating when we didn't need to (i.e. no path to fade), so hopefully that handles most of this.  Will monitor and either it will resolve itself.

But if this is comparing a completely blank page with a path to be faded, then I do expect a regression of some amount.
Cc: nyerramilli@chromium.org
Labels: TE-Triaged
gentle ping..
 Issue 598318  has been merged into this issue.
Status: WontFix (was: Assigned)
In general, this is a won't fix.

Previously, this behavior was done on document mode and we are enabled it for tabbed mode as well.  This just happened to be hit by that move.

I did make some improvements by not animating unnecessarily -- as it had been for the last year for document mode :-/.  But as this was approved by launch review, there isn't much else to do.

Launch review bug to enable this all the places: https://bugs.chromium.org/p/chromium/issues/detail?id=597147

Sign in to add a comment