New issue
Advanced search Search tips

Issue 591246 link

Starred by 0 users

Issue metadata

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



Sign in to add a comment

5.7%-20.3% regression in thread_times.simple_mobile_sites at 378306:378381

Project Member Reported by qyears...@chromium.org, Mar 2 2016

Issue description

See the link to graphs below.
 
Cc: vmp...@chromium.org
Owner: vmp...@chromium.org

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

Hi vmpstr@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 : cc: ImageDecodes: handle low quality filters.
Author  : vmpstr
Commit description:
  
This patch pins low quality interpolation filters, which means that
it is now handled by the image decode controller.

R=enne
BUG= 558063 
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

Cr-Commit-Position: refs/heads/master@{#378355}
Commit  : 6372e1669779a7b82bd130c55dca71d39f23643f
Date    : Tue Mar 01 01:22:17 2016


===== TESTED REVISIONS =====
Revision                Mean Value  Std. Dev.   Num Values  Good?
chromium@378305         2.553192    0.06087     5           good
chromium@378343         2.552506    0.025254    5           good
chromium@378353         2.55959     0.043077    5           good
chromium@378354         2.568548    0.046056    5           good
chromium@378355         3.051709    0.068885    5           bad
chromium@378356         3.061208    0.079931    5           bad
chromium@378358         3.018591    0.103199    5           bad
chromium@378362         2.95915     0.088822    5           bad
chromium@378381         3.076367    0.074722    5           bad

Bisect job ran on: android_nexus7_perf_bisect
Bug ID: 591246

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --also-run-disabled-tests thread_times.simple_mobile_sites
Test Metric: thread_raster_cpu_time_per_frame/thread_raster_cpu_time_per_frame
Relative Change: 20.49%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus7_perf_bisect/builds/2819
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9019344217911190176


| 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: ericrk@chromium.org enne@chromium.org
It's weird, since this path should not be hit at all on android. I'll take a look.
So, I can confirm two things:
1. The patch in question does seem to regress the thread times numbers
2. The patch in question is not exercised at all on android. :\

I'm attaching two screenshots, one with and one without the patch (I've run this twice to confirm).

It seems that with the patch, we always decode image at a certain spot and it takes a long time. Without the patch, we don't. However, both of those happen at raster time and this patch is only working with image decode controller, which isn't run.

I noticed that there are also memory regressions associated with this bug. Is it possible to run a bisect to see when those happened? I'm not sure if those two are related, but if they are it could explain why we're doing extra decodes: we're running of memory and evicting discardable sooner. 

Honestly, I'm very hesitant about reverting this, since the next step is to enable image decodes on android, which should help cases like this (ie, if the image gets evicted, we wouldn't decode it twice since it would remain locked). 

That being said, clearly something is different if I can repro this fairly consistently. 
without.jpg
167 KB View Download
with.jpg
215 KB View Download
Actually, I think we might still be using the image decode controller in some cases. I will investigate further. 
Project Member

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

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

commit afb46c11bdde02fe8cd4a76ae5b08e30543d6255
Author: vmpstr <vmpstr@chromium.org>
Date: Thu Mar 03 01:48:27 2016

cc: Disable image hijack canvas when we don't have images in the map.

This patch ensures that we don't use the hijack canvas when there are
no images in the discardable map, since this is the way we "disable"
things on android.

Note we already do this in PlaybackToSharedCanvas, so this is just
ensuring that we also do that in PlaybackToCanvas.

BUG= 591246 
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

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

[modify] https://crrev.com/afb46c11bdde02fe8cd4a76ae5b08e30543d6255/cc/playback/display_list_raster_source.cc

That one should fix it. I'll check the bots before closing.
Status: Fixed (was: Assigned)
Everything seems to have recovered.
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 3 2016

Labels: merge-merged-2666
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/afb46c11bdde02fe8cd4a76ae5b08e30543d6255

commit afb46c11bdde02fe8cd4a76ae5b08e30543d6255
Author: vmpstr <vmpstr@chromium.org>
Date: Thu Mar 03 01:48:27 2016

cc: Disable image hijack canvas when we don't have images in the map.

This patch ensures that we don't use the hijack canvas when there are
no images in the discardable map, since this is the way we "disable"
things on android.

Note we already do this in PlaybackToSharedCanvas, so this is just
ensuring that we also do that in PlaybackToCanvas.

BUG= 591246 
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

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

[modify] https://crrev.com/afb46c11bdde02fe8cd4a76ae5b08e30543d6255/cc/playback/display_list_raster_source.cc

Sign in to add a comment