All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=737981

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg7omGrwkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgrpfyvAoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg7oLxsgoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg7pCgrwkM


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

android-webview-nexus5X

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8975446519873618560

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

Hi ccameron@chromium.org, the bisect results pointed to your CL, please take a look at the
results.


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : Christopher Cameron
  Commit : 1894d423068be735560e0171922c833c4091b5d1
  Date   : Wed Jun 28 06:31:03 2017
  Subject: color: Enable color correct rendering by default

Bisect Details
  Configuration: android_webview_arm64_aosp_perf_bisect
  Benchmark    : system_health.memory_mobile
  Metric       : memory:webview:all_processes:reported_by_chrome:malloc:effective_size_avg/load_tools/load_tools_dropbox
  Change       : 18.80% | 36491420.0 -> 43351782.0

Revision             Result                   N
chromium@482896      36491420 +- 1299474      6      good
chromium@482902      36383233 +- 1070982      6      good
chromium@482903      36564267 +- 777347       6      good
chromium@482904      43009278 +- 1063461      6      bad       <--
chromium@482905      43248261 +- 1033619      6      bad
chromium@482907      43176763 +- 847872       6      bad
chromium@482917      43351782 +- 494167       6      bad

Please refer to the following doc on diagnosing memory regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.tools.dropbox system_health.memory_mobile

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8975446519873618560


For feedback, file a bug with component Speed>Bisection

 Issue 737980  has been merged into this issue.

 Issue 737985  has been merged into this issue.

Taking a look at these regressions.

 Issue 738134  has been merged into this issue.

 Issue 737979  has been merged into this issue.

 Issue 737597  has been merged into this issue.

 Issue 737705  has been merged into this issue.

 Issue 738166  has been merged into this issue.

It looks like we have a few categories of bugs. Each one still needs some individual attention. FWICT, they are:

Power regressions on Mac (battor)
- this affects WebGL, animation, and "trivial scrolling"
- for WebGL, some slight increase in power consumption is expected
  - this is because WebGL is drawn in sRGB and will need conversion to the display
- the power difference should be trivial, not this substantial
- this needs to be fixed to ship, hopefully is simple

memory.top_10_mobile
- this is the bulk of the regression instances by number
- this is limited to Android devices, but hits almost every android device
- for pages that don't have any custom-color-profile-images (so, most pages),
  this feature should have no impact on memory
- for all non-WCG Android devices (also known as all Android devices currently
  in existence), there should be zero memory impact.
- we may need to add a "sRGB-only-device" hack to make this happen

frame times/Filter Terrain SVG
- this is likely because we are putting an image into a Skia filter, which
  is then converted every single frame (because we didn't put caches there)
- we'll need to verify this theory, and see what can be done to mitigate

CPU time in "load_game_bubbles"
- needs investigation

I definitely see an image filter caching issue.  Not because we store images but because SkColorSpaceXformCanvas transforms the image filters on-the-fly: creates an ephemeral filter, draws, then throws it away:

https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkColorSpaceXformCanvas.cpp?rcl=9f183505acc94a587e71b485761f47781d6f5030&l=57

https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkColorSpaceXformer.cpp?rcl=9f183505acc94a587e71b485761f47781d6f5030&l=93

So SkImageFilter::filterImage() always sees new UIDs, and all cache look-ups are misses.

Issue 737976 has been merged into this issue.

 Issue 738717  has been merged into this issue.

 Issue 738715  has been merged into this issue.

 Issue 738715  has been merged into this issue.

 Issue 739187  has been merged into this issue.

Since #13, there are some more reports that have come in -- I'm working on some preliminary patches to consolidate color spaces so that we can detect when we can avoid work.

I don't know if you have a separate bug for the filter effects regression, but I can help with a solution if need be. The best thing would be to insert the colour correction filters at build time, probably in SkiaImageFilterBuilder::Build().

Note that filter effects themselves implement a simple form of colour correction, supporting linearRGB or sRGB interpolation (color-interpolation, color-interpolation-filters) as described in the filter effects spec. If required, the builder inserts a SkColorFilterImageFilter node to convert spaces which is cached along with the filter chain. (See SkiaImageFilterBuilder::TransformInterpolationSpace()).

Seems like having blink convert imagefilters up-front would be a definite win -- as today we perform that transformation on every draw. We would (I think) just have to coordinate doing the work in blink with disabling the same work in SkColorSpaceXformCanvas (it would just ignore imagefilters).

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/412cda7379626ee3acfd1dbb1441adde81efddc3

commit 412cda7379626ee3acfd1dbb1441adde81efddc3
Author: Mike Reed <reed@google.com>
Date: Wed Jul 05 20:16:27 2017

add srgb gamma colorfilters
... faster and more accurate than using SkTableColorFilter

todo: update blink after this lands

Bug:737981
Change-Id: I55b5c60dd23b9d2cbe9d60f83c74be1a8f3dcfcf
Reviewed-on: https://skia-review.googlesource.com/21368
Commit-Queue: Mike Reed <reed@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>

[modify] https://crrev.com/412cda7379626ee3acfd1dbb1441adde81efddc3/include/core/SkColorFilter.h
[modify] https://crrev.com/412cda7379626ee3acfd1dbb1441adde81efddc3/tests/ApplyGammaTest.cpp
[modify] https://crrev.com/412cda7379626ee3acfd1dbb1441adde81efddc3/tools/sk_tool_utils.h
[modify] https://crrev.com/412cda7379626ee3acfd1dbb1441adde81efddc3/src/effects/SkTableColorFilter.cpp
[modify] https://crrev.com/412cda7379626ee3acfd1dbb1441adde81efddc3/samplecode/SampleApp.cpp
[modify] https://crrev.com/412cda7379626ee3acfd1dbb1441adde81efddc3/src/core/SkColorFilter.cpp
[modify] https://crrev.com/412cda7379626ee3acfd1dbb1441adde81efddc3/tools/sk_tool_utils.cpp

I've separated out one Android memory regression investigation into  issue 739559 . Of note is that when we have a no-op ColorSpaceXformCanvas (all input images have been forced to be sRGB and the output color space is set to sRGB), there still exists a memory regression, but when we eliminate the ColorSpaceXformCanvas entirely, then the regression goes away.

Most of these are resolving now. After we have a few more data points I'll try to find which ones are still there.

Attaching a "can start breathing again" graph.

Deleted: perfbot.png 152 KB

	perfbot.png 152 KB View Download

Closed a bunch of the "obviously fixed" regressions. That included almost everything in
- smoothness.tough_filters_cases
- smoothness.gpu_rasterization.tough_filters_cases
- browse_shopping_amazon, load_tools_drive, load_tools_dropbox

I'm still waiting for a clear signal for some of the other tests (it's only been a few hours) -- they interact with  issue 738162  and  issue 736262 , which both had a bunch of (WontFix) regressions, which means that things aren't back to where they originally were.

 Issue 741232  has been merged into this issue.

Issue 741681 has been merged into this issue.

Issue 742340 has been merged into this issue.

 Issue 741684  has been merged into this issue.

Many more of these have now recovered with the fixes for SkImage_Lazy, including all of load_bubbles.

 Issue 746233  has been merged into this issue.

 Issue 746231  has been merged into this issue.

How is it that we're still having new reports filed on this bug ~4 weeks after the original change went in? Many of these have already recovered.

Sorry about that! There was a problem with our linux bots that caused them to go down for a month. They came back up recently and immediately reported that you'd caused a regression last month. Example: https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4bDetQoM

Issue 746241 has been merged into this issue.

Issue 750338 has been merged into this issue.

ccameron: any update on #28?

There are still a couple that haven't resolved 100%. 

Made a final pass through these:

There are indeed a few places where more power consumption or more memory usage is expected. In particular
- mac: displaying non-display-color-space content (WebGL, canvas, video)
  - we are now doing a conversion we used not to do
  - this affects Mac more than other platforms
  - color conversion in the WindowServer is more expensive than in GLRenderer (not sure why)
  - the baseline power usage for the WindowServer is much lower than GLRenderer, though
- mac: software decoded video
  - we used to lie about the color space for this (more than the above) on Mac
  - we now do conversion
  - this also sometimes causes our buffer queue to be 1 larger than it was, which can use more memory when we were on the border
- all: rasterizing images to color calibrated displays
  - this causes an extra conversion step that we used to (incorrectly) skip
  - this was the cause of some increase in the "tumblr" tests on a handful of platforms
  - I did some investigation into this in crbug.com/719735, to see if we can (correctly) skip this
  - at least on Mac, it depends on how often we re-use the tiles, can can often be a loss if we display them often
  - this is because of the WindowServer conversion cost

There is still "Analog Clock SVG" which I haven't dug in on yet. It clearly went bad (either to bi-modal or uni-modal) with color calibration on some Android devices. Some other platforms did similar things at other times (Linux at  issue 742473 , Windows alert from earlier was ignored).

ccameron: should we close this out, or are you planning on looking into "Analog Clock SVG"?

Yes, let's close this out.

There is going to be a new Skia implementation of color conversion (which will not use SkColorSpaceXformCanvas) -- I will be focusing on transitioning to using that.