Ensure caching of shaders/filters for OOP raster. |
|||
Issue descriptionSkia internally maintains a cache for SkShaders and SkImageFilters which is keyed on unique IDs tied to the object. With OOP raster. Since we always create new skia objects on the service side, this means we never get a cache hit. There's a few complications here. We could use the transfer cache for these things and reuse the skia backing on the service. But skia backing of the paint object on the service could have references to images from the transfer cache. For instance, if you have a record shader which has an image. So these internal references will need to be kept alive for the SkShader to be reused. It might also be wasteful to keep these images in the cache if skia already has a rasterized record with the required scale. So its worth considering to do the rasterization for such records in cc and caching that result instead.
,
Feb 23 2018
Its just speculative, I haven't come across a case where this showed up as a perf hit. Namespace-ing the ids on the renderer and using the transfer cache should work for all these cases. Good point about paths too!
,
Feb 27 2018
Dug a bit more and turns out, with GPU rasterization skia doesn't do any filter caching (https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/SkGpuDevice.cpp?q=SkGpuDevice.cpp&dr&l=1746).
,
Apr 6 2018
,
Apr 21 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/06f34304a6c544250bcd3a14be06f08dfd14a4b3 commit 06f34304a6c544250bcd3a14be06f08dfd14a4b3 Author: Adrienne Walker <enne@chromium.org> Date: Sat Apr 21 01:27:35 2018 cc: Serialize SkPaths using the transfer cache Bug: 815022 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Change-Id: I0856659a87a07369049902f2a1adb7d82fc4bc4a Reviewed-on: https://chromium-review.googlesource.com/1012187 Commit-Queue: enne <enne@chromium.org> Reviewed-by: Khushal <khushalsagar@chromium.org> Cr-Commit-Position: refs/heads/master@{#552542} [modify] https://crrev.com/06f34304a6c544250bcd3a14be06f08dfd14a4b3/cc/paint/BUILD.gn [modify] https://crrev.com/06f34304a6c544250bcd3a14be06f08dfd14a4b3/cc/paint/paint_op_reader.cc [modify] https://crrev.com/06f34304a6c544250bcd3a14be06f08dfd14a4b3/cc/paint/paint_op_writer.cc [add] https://crrev.com/06f34304a6c544250bcd3a14be06f08dfd14a4b3/cc/paint/path_transfer_cache_entry.cc [add] https://crrev.com/06f34304a6c544250bcd3a14be06f08dfd14a4b3/cc/paint/path_transfer_cache_entry.h [modify] https://crrev.com/06f34304a6c544250bcd3a14be06f08dfd14a4b3/cc/paint/transfer_cache_entry.cc [modify] https://crrev.com/06f34304a6c544250bcd3a14be06f08dfd14a4b3/cc/paint/transfer_cache_entry.h
,
May 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/90b79a26fcccb656317b63fcb2da2f4503cbc596 commit 90b79a26fcccb656317b63fcb2da2f4503cbc596 Author: Adrienne Walker <enne@chromium.org> Date: Tue May 08 21:40:42 2018 cc: Cache picture shaders on the service side Skia caches the results of SkPictureShaders internally based on its fUniqueId. In order to correctly hit that cache, the gpu process needs to cache and reuse SkPictureShader objects where possible. One possibility is to use the transfer cache for this (as was done for SkPaths). However, paint record shaders can transitively include images, paint records, other paint record shaders, paths. Using the transfer cache would mean having to support transfer cache dependency graphs, which would make future parallelization more complicated. Maybe we can punt on this forever, but definitely punt for now. This patch deliberately chooses a hacky workaround to avoid this design snag. In particular, we always serialize the entire PaintRecord in the shader along with images (at the correct scale). On the gpu service side, if this shader matches the same scale and colorspace of the last time we used it, we use the cached SkShader, otherwise we'll build a new one. Matching the scale is for visual correctness (as images at different scales might look visually worse). Matching the colorspace is for performance (to avoid unnecessary colorspace conversions) but also for visual correctness (colorspace conversion causing quality degradation). Performance for this is tricky to test. Animometer has no picture shaders at all. One bug that regressed when shaders were not being cached was the test case attached in http://crbug.com/788075 . Switching images back and forth on that test page had the following results: * gpu raster: 32.7ms/frame * oop raster: 29.7ms/frame * oop raster + patch: 26.0ms/frame (18% improvement over gpu raster) Bug: 815022 , 788075 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Change-Id: I350b1ed6ad90f8c660b1a47444f984ef309ae555 Reviewed-on: https://chromium-review.googlesource.com/1037770 Commit-Queue: enne <enne@chromium.org> Reviewed-by: ccameron <ccameron@chromium.org> Reviewed-by: Eric Karl <ericrk@chromium.org> Reviewed-by: Khushal <khushalsagar@chromium.org> Cr-Commit-Position: refs/heads/master@{#556968} [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/BUILD.gn [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/paint_op_buffer.cc [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/paint_op_buffer.h [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/paint_op_buffer_serializer.cc [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/paint_op_buffer_unittest.cc [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/paint_op_reader.cc [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/paint_op_reader.h [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/paint_op_writer.cc [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/paint_shader.cc [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/paint_shader.h [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/scoped_raster_flags.cc [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/scoped_raster_flags.h [add] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/shader_transfer_cache_entry.cc [add] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/shader_transfer_cache_entry.h [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/skia_paint_canvas.cc [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/skia_paint_canvas.h [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/transfer_cache_deserialize_helper.h [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/transfer_cache_entry.cc [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/paint/transfer_cache_entry.h [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/test/transfer_cache_test_helper.cc [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/cc/test/transfer_cache_test_helper.h [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/gpu/command_buffer/service/gles2_cmd_decoder.cc [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/gpu/command_buffer/service/raster_decoder.cc [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/gpu/command_buffer/service/service_transfer_cache.cc [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/gpu/command_buffer/service/service_transfer_cache.h [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/ui/gfx/color_space.cc [modify] https://crrev.com/90b79a26fcccb656317b63fcb2da2f4503cbc596/ui/gfx/color_space.h
,
May 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/868717cedf7ffa79d2b248712ee45095ae9831e0 commit 868717cedf7ffa79d2b248712ee45095ae9831e0 Author: Lei Zhang <thestig@chromium.org> Date: Wed May 09 04:33:56 2018 Fix jumbo build in cc/paint after r556968. Also rename globals from "g_foo_" to "g_foo" since they are not member variables. Bug: 815022 , 788075 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Change-Id: I5ecee5b3559572dc2f700ad8ae8eb784fc298456 Reviewed-on: https://chromium-review.googlesource.com/1050872 Commit-Queue: Mostyn Bramley-Moore <mostynb@vewd.com> Reviewed-by: enne <enne@chromium.org> Cr-Commit-Position: refs/heads/master@{#557091} [modify] https://crrev.com/868717cedf7ffa79d2b248712ee45095ae9831e0/cc/paint/paint_image.cc [modify] https://crrev.com/868717cedf7ffa79d2b248712ee45095ae9831e0/cc/paint/paint_shader.cc
,
Jun 19 2018
With shader caching, this brings us on-par with GPU raster. Filters are currently not cached with GPU raster also. |
|||
►
Sign in to add a comment |
|||
Comment 1 by enne@chromium.org
, Feb 23 2018