I'm able to reproduce this issue on my Mac with forced software rendering.  Attached a trace with Skia category.  It looks like a SkCanvas::drawPicture inside a SkCanvas::drawRect is in each tile, which sounds like maybe we're not caching SkPictureShaders' output.

khushal@: could you PTAL?

Deleted: trace_slow_svg_bg_raster.json.gz 1.0 MB

trace_slow_svg_bg_raster.json.gz 1.0 MB Download

Yeah, I can repro it too. Here is a trace with some more tracing in skia. Look for PictureShaderCacheMiss, we can see a different shader id in each trace before the picture is rasterized. This means a different PaintShader instance created at record time. I thought we were caching PaintShader objects in blink to avoid this.

Actually attach the file.

Deleted: trace_skia_picture_cache_miss.json.gz 4.7 MB

trace_skia_picture_cache_miss.json.gz 4.7 MB Download

> This means a different PaintShader instance created at record time.

I'm seeing a different picture id in each tile.  Doesn't this imply we're re-creating pictures at raster time somehow?

Yup, you're right. It is at raster time and in skia, inside every drawRect call. Here is another trace, this time with picture creation traces. fmalita@, could you take a look at what's happening in skia..?

Deleted: trace_skia_shader_creation.json.gz 4.8 MB

trace_skia_shader_creation.json.gz 4.8 MB Download

This may also be related to SkColorSpaceXformCanvas, making temporary SkPictureShaders.

I tried forcing sRGB color profile in about:flags, but it looks like that doesn't disable passing through SkColorSpaceXformCanvas.  Should it?

c#6 indeed, I'm seeing rolling shader IDs in Skia (with a stable picture ID):

...
** SkPictureShader cache miss shader id: 1308, picture id: 12
** SkPictureShader cache miss shader id: 1309, picture id: 12
** SkPictureShader cache miss shader id: 1310, picture id: 12
** SkPictureShader cache miss shader id: 1311, picture id: 12
** SkPictureShader cache miss shader id: 1312, picture id: 12
...


Tracked it down to SkColorSpaceXformCanvas:

1  (anonymous namespace)::next_id                                    SkPictureShader.cpp              123  0x7fa62fc7b1b1 
2  SkPictureShader::SkPictureShader                                  SkPictureShader.cpp              137  0x7fa62fc7b160 
3  SkPictureShader::onMakeColorSpace                                 SkPictureShader.cpp              327  0x7fa62fc7c9b5 
4  SkShaderBase::makeColorSpace                                      SkShaderBase.h                   188  0x7fa62fa9cd89 
5  SkColorSpaceXformer::apply                                        SkColorSpaceXformer.cpp          136  0x7fa62fa9c05b 
6  SkColorSpaceXformer::apply                                        SkColorSpaceXformer.cpp          162  0x7fa62fa9c265 
7  SkColorSpaceXformCanvas::onDrawRect                               SkColorSpaceXformCanvas.cpp      57   0x7fa62fa98696 
8  SkCanvas::drawRect                                                SkCanvas.cpp                     1710 0x7fa62fa643ea 
9  cc::DrawRectOp::RasterWithFlags                                   paint_op_buffer.cc               1167 0x7fa627f8e563 
10 cc::Rasterizer<cc::DrawRectOp, true>::RasterWithFlags             paint_op_buffer.cc               94   0x7fa627f9e4e2 
11 cc::$_40::operator()                                              paint_op_buffer.cc               129  0x7fa627f97dec 
12 cc::$_40::__invoke                                                paint_op_buffer.cc               129  0x7fa627f97da0 
13 cc::PaintOpWithFlags::RasterWithFlags                             paint_op_buffer.cc               1839 0x7fa627f93920 
14 cc::PaintOpBuffer::Playback                                       paint_op_buffer.cc               2172 0x7fa627f94a1e 
15 cc::PaintOpBuffer::Playback                                       paint_op_buffer.cc               2046 0x7fa627f8e512 
16 cc::DrawRecordOp::Raster                                          paint_op_buffer.cc               1159 0x7fa627f8e4d9 
17 cc::Rasterizer<cc::DrawRecordOp, false>::Raster                   paint_op_buffer.cc               81   0x7fa627f9c604 
18 cc::$_13::operator()                                              paint_op_buffer.cc               115  0x7fa627f9703c 
19 cc::$_13::__invoke                                                paint_op_buffer.cc               115  0x7fa627f97008 
20 cc::PaintOp::Raster                                               paint_op_buffer.cc               1660 0x7fa627f92a54 
21 cc::PaintOpBuffer::Playback                                       paint_op_buffer.cc               2181 0x7fa627f94b01 
22 cc::DisplayItemList::Raster                                       display_item_list.cc             57   0x7fa627f613d5 
23 cc::RasterSource::RasterCommon                                    raster_source.cc                 162  0x7fa62826b325 
24 cc::RasterSource::PlaybackToCanvas                                raster_source.cc                 82   0x7fa62826adbd 
25 cc::RasterSource::PlaybackToCanvas                                raster_source.cc                 62   0x7fa62826a98d 
26 cc::RasterBufferProvider::PlaybackToMemory                        raster_buffer_provider.cc        92   0x7fa628269500 
27 cc::(anonymous namespace)::BitmapRasterBufferImpl::Playback       bitmap_raster_buffer_provider.cc 54   0x7fa62825c462 
28 cc::(anonymous namespace)::RasterTaskImpl::RunOnWorkerThread      tile_manager.cc                  136  0x7fa6283970d2 
29 content::CategorizedWorkerPool::RunTaskInCategoryWithLockAcquired categorized_worker_pool.cc       361  0x7fa62be355c3 
30 content::CategorizedWorkerPool::RunTaskWithLockAcquired           categorized_worker_pool.cc       340  0x7fa62be346a1 
31 content::CategorizedWorkerPool::Run                               categorized_worker_pool.cc       232  0x7fa62be344c1 
32 content::(anonymous namespace)::CategorizedWorkerPoolThread::Run  categorized_worker_pool.cc       35   0x7fa62be3584d 
33 base::SimpleThread::ThreadMain                                    simple_thread.cc                 68   0x7fa6309eed35 
34 base::(anonymous namespace)::ThreadFunc                           platform_thread_posix.cc         75   0x7fa6309d8b01 
35 start_thread                                                                                            0x7fa630d2a609 
36 clone                                                                                                   0x7fa61728ce6f 

So this goes back to the lack of caching for xformed objects in CC, and whipping out fresh shaders on every playback.

+reed

Looking closer at picture shader caching in Skia, we can probably help some in this case: currently we use a key based on (shader_id, color_space, matrix, tile, etc).

This seems kinda odd - the key should either be (shader_id, dest CS) as all other params are fixed for a given shader ID, or (picture_id, CS, matrix, etc).

The second form would allow us avoid thrashing as long as the picture and color space stay constant.

So I gave it a shot, and it does make a big difference... but only with --disable-gpu-compositing:

...
*** CACHE HIT picture id: 12, dst cs: (nil), own cs: 0x7f37fbfbb620
*** CACHE HIT picture id: 12, dst cs: (nil), own cs: 0x7f37fbfbb620
*** CACHE HIT picture id: 12, dst cs: (nil), own cs: 0x7f37fbfbb620
*** CACHE HIT picture id: 12, dst cs: (nil), own cs: 0x7f37fbfbb620
*** CACHE HIT picture id: 12, dst cs: (nil), own cs: 0x7f37fbfbb620
*** CACHE HIT picture id: 12, dst cs: (nil), own cs: 0x7f37fbfbb620
...


With GPU compositing, it appears that the color space object is changing on every playback, and again defeats caching:

...
*** CACHE MISS picture id: 11, dst cs: (nil), own cs: 0x3fd2291e1380
*** CACHE MISS picture id: 11, dst cs: (nil), own cs: 0x3fd2291e1460
*** CACHE MISS picture id: 11, dst cs: (nil), own cs: 0x3fd2291e1540
*** CACHE MISS picture id: 11, dst cs: (nil), own cs: 0x3fd2291e1620
*** CACHE MISS picture id: 11, dst cs: (nil), own cs: 0x3fd2291e1700
*** CACHE MISS picture id: 11, dst cs: (nil), own cs: 0x3fd2291e17e0
*** CACHE MISS picture id: 11, dst cs: (nil), own cs: 0x3fd2291e18c0
...


Chris, do you know why the color space is not stable?

In general, can we generate a stable key from CS? So that even if the object is different you have the same key if its the same CS.

Hmm, Brian will be the sane mind to correct me if I go wrong here...

We don't have a good way of getting a stable SkColorSpace key right now, and in general that'd be a very hard problem, but I think we could potentially make one work for the specific subset of simple color spaces that we can draw to and from (as opposed to the broader set that also includes complex color spaces we can only bulk transform into the simple ones).

Basically, we'd need to cat together the 7 transfer function and 12 gamut parameters into a nice little 76 byte chunk.  If we're ok with minuscule, non-zero false positive odds, we can probably hash that down to 4 or 8 bytes.

We do already have an SkColorSpace::Equals(), so if we can get ourselves in a mode where we're calling that rather than comparing keys, everything's easy.

Re #6, we always use the SkColorSpaceXformCanvas -- there is no "non consistent color mode" anymore. By targeting sRGB, we tend not to re-hit things.

Re #8, the color space should be stable in the sense that SkColorSpace::Equals should return true. It will not have same pointer, though.

Re #11, for key-ing SkColorSpaces, we will only ever raster to SkColorSpaces that are generated by gfx::ColorSpace::ToSkColorSpace, which is a bit more limited (just parametric and named)
https://cs.chromium.org/chromium/src/ui/gfx/color_space.cc?rcl=a7f4ab11cf9b65467aab860edc38e613d604b37d&l=403

If we want a short-term fix I can throw together a global cache to ensure the same gfx::ColorSpace returns the same SkColorSpace.

> Re #11, for key-ing SkColorSpaces, we will only ever raster to SkColorSpaces that are generated by gfx::ColorSpace::ToSkColorSpace, which is a bit more limited (just parametric and named)

Ah good.  That would still require that 7+12 float key, but good to know we're not dealing with the really complicated impossible ones.

> If we want a short-term fix I can throw together a global cache to ensure the same gfx::ColorSpace returns the same SkColorSpace.

This sgtm.

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

commit c99eb385c6ba9d5b90f3344247dd19258b9f6e18
Author: Christopher Cameron <ccameron@chromium.org>
Date: Wed Dec 20 19:35:07 2017

Cache gfx::ColorSpace to SkColorSpace conversion

There exist internal pointer-based SkColorSpace caches in skia.

To ensure that these caches hit, cache the results of
gfx::ColorSpace::ToSkColorSpace.

Add some tests for this.

Bug: 793116
Change-Id: I872e8137e6e6d87d3d01032d8a7ad3fdfecc0fbf
Reviewed-on: https://chromium-review.googlesource.com/834996
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525409}
[modify] https://crrev.com/c99eb385c6ba9d5b90f3344247dd19258b9f6e18/ui/gfx/color_space.cc
[modify] https://crrev.com/c99eb385c6ba9d5b90f3344247dd19258b9f6e18/ui/gfx/color_space_unittest.cc

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

commit bc5a59b8a5460650fde8ca3f57dd5b7c50d17c12
Author: Brian Osman <brianosman@google.com>
Date: Thu Jan 17 21:11:32 2019

Remove the gfx::ColorSpace -> SkColorSpace cache

This was required when Skia keyed internal caches based on SkColorSpace
pointers. That cache is now value-based[1], so this cache is unnecessary.

This effectively reverts http://crrev.com/c/834996

[1] https://skia-review.googlesource.com/c/skia/+/163249

Bug: 793116
Change-Id: I340830bfb86fd6ff85304bc30a34a1b772f6a7da
Reviewed-on: https://chromium-review.googlesource.com/c/1418390
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: Brian Osman <brianosman@google.com>
Reviewed-by: Mike Klein <mtklein@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623842}
[modify] https://crrev.com/bc5a59b8a5460650fde8ca3f57dd5b7c50d17c12/cc/tiles/gpu_image_decode_cache_unittest.cc
[modify] https://crrev.com/bc5a59b8a5460650fde8ca3f57dd5b7c50d17c12/ui/gfx/color_space.cc
[modify] https://crrev.com/bc5a59b8a5460650fde8ca3f57dd5b7c50d17c12/ui/gfx/color_space_unittest.cc