Issue metadata
Sign in to add a comment
|
7.7%-1171.1% regression in system_health.memory_mobile at 551053:551217 |
||||||||||||||||||||
Issue descriptionLooks like this is from probe change though
,
Apr 19 2018
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/149a403ec40000
,
Apr 19 2018
📍 Found a significant difference after 1 commit. https://pinpoint-dot-chromeperf.appspot.com/job/149a403ec40000 Don't allow ganesh to allocate mip maps for wrapped textures. by egdaniel@google.com https://skia.googlesource.com/skia/+/e3204864899651a132d3387422d7fd599c21b3ac Understanding performance regressions: http://g.co/ChromePerformanceRegressions
,
Jun 1 2018
egdaniel: any update on this bug? This is a pretty big memory regression.
,
Jun 4 2018
+cc: ericrk, bsalomon So just to give some background to what the original change here was. We want to have Ganesh in a world where when a client (chrome) gives us a wrapped texture we do not alter that texture in anyway. This includes the allocation/generation of mip maps for that texture. Before the original change Ganesh would happily allocate mips for GL texture (on a side note, this was opaque to the client and no one was actually accounting for this extra memory being used). This CL, instead leaves the original texture as is, and instead allocates a new mipped texture and copies the original into it. This new mipped texture lives purely inside Ganesh and its caches. There are numerous benefits to moving this new system. First the client can be sure whatever texture they give Ganesh stays in the same state as what they can us. Second, if we decide we want to move to using texture storage for allocations, we need to allocate the mips at creation time and the old late mip allocation won't work. And third, when we move for Vulkan, there is no such thing as late mip allocation. We always have to create a new VkImage and copy into it. The ideal fix for this memory increase (and performance since we won't need an extra copy), is for Chrome to give us or tell us to make a mipped texture from the outset if they know it will need mip maps. Ericrk@ do you know (or do you know who would know) if chrome can know ahead of time during texture creation whether they will need mips? We can add to the SkImage::makeTextureImage call a parameter that says make the mips to help in the process as well.
,
Jun 4 2018
I see, this makes sense. The answer is that, in most cases, Chrome *does* know whether mips are needed and can pass this in. Additionally, Chrome becomes aware if mips are needed later. How about the following: 1) As you suggest, add a flag to makeTextureImage to indicate that mips should be generated. 2) If we need to generate mips later, Chrome can generate the mips directly with GL and re-export the texture to Skia with mips set to true. Alternately, we could have Skia generate the mips in #2 and somehow re-generate the texture? WDYT?
,
Jun 4 2018
I think that all sounds good. For the "re-export" part of 2) I don't think Skia really cares if chrome makes the new texture or we allow skia to make the new texture. For skia making the new texture, I think the best API is to just use the makeTextureImage call on the texture backend image. If the mip state is the same we will just return the original image, otherwise if the original was not mipped and you request mips we'll create a new mipped texture and chrome can do its "texture steal" thing to get ownership of the new mipped GrBackendTexture.
,
Jun 4 2018
This all sounds good to me. If you have bandwidth to add the Skia API I'm happy to wire it up in Chrome.
,
Jun 4 2018
Yeah I'll get the API done on the Skia side.
,
Jun 7 2018
,
Jun 13 2018
The following revision refers to this bug: https://skia.googlesource.com/skia/+/5f4b09d523a761a3a5c622bb01eeba47905da5f0 commit 5f4b09d523a761a3a5c622bb01eeba47905da5f0 Author: Greg Daniel <egdaniel@google.com> Date: Wed Jun 13 13:55:56 2018 Allow caller to specify if the want mip maps in makeTextureImage call. Since Ganesh no longer will allocate mips late, this gives the clients a way to tell skia that they want the texture they will be using to have mips. It also supports allowing a client to take a non mipped texture backed image and turn it into a new image which is mipped and texture backed. Bug: chromium:834837 Change-Id: I1781ce618c22023b6309f248e7ee49e69bd3c6df Reviewed-on: https://skia-review.googlesource.com/134323 Commit-Queue: Greg Daniel <egdaniel@google.com> Reviewed-by: Eric Karl <ericrk@chromium.org> Reviewed-by: Cary Clark <caryclark@google.com> Reviewed-by: Brian Salomon <bsalomon@google.com> [modify] https://crrev.com/5f4b09d523a761a3a5c622bb01eeba47905da5f0/src/gpu/GrTextureProducer.h [modify] https://crrev.com/5f4b09d523a761a3a5c622bb01eeba47905da5f0/src/image/SkImage_Gpu.cpp [modify] https://crrev.com/5f4b09d523a761a3a5c622bb01eeba47905da5f0/src/image/SkImage.cpp [modify] https://crrev.com/5f4b09d523a761a3a5c622bb01eeba47905da5f0/src/gpu/GrTextureProducer.cpp [modify] https://crrev.com/5f4b09d523a761a3a5c622bb01eeba47905da5f0/docs/SkImage_Reference.bmh [modify] https://crrev.com/5f4b09d523a761a3a5c622bb01eeba47905da5f0/tests/ImageTest.cpp [modify] https://crrev.com/5f4b09d523a761a3a5c622bb01eeba47905da5f0/include/core/SkImage.h
,
Jun 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6b91daa8df790da14d7ba894b7c769a227736653 commit 6b91daa8df790da14d7ba894b7c769a227736653 Author: skia-chromium-autoroll <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Date: Thu Jun 14 04:02:00 2018 Roll src/third_party/skia fdcfb8b7c23f..657edbede4e3 (27 commits) https://skia.googlesource.com/skia.git/+log/fdcfb8b7c23f..657edbede4e3 git log fdcfb8b7c23f..657edbede4e3 --date=short --no-merges --format='%ad %ae %s' 2018-06-13 bungeman@google.com Remove SkBool8 (again). 2018-06-13 angle-skia-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com Roll third_party/externals/angle2 3e313805e5a2..4cc753e01054 (8 commits) 2018-06-13 bungeman@google.com Adjust FreeType matrix based on what came back. 2018-06-13 csmartdalton@google.com ccpr: Initialize the atlas size more intelligently 2018-06-13 fmalita@chromium.org Handle missing json resource gracefully in 3dgm 2018-06-13 recipe-roller@chromium.org Roll recipe dependencies (trivial). 2018-06-13 benjaminwagner@google.com Update Win version in Skolo. 2018-06-13 bungeman@google.com Remove SkString fwd decl from SkTypes.h. 2018-06-13 robertphillips@google.com Revert "Add --gpuThreads support to skpbench.py" 2018-06-13 recipe-roller@chromium.org Roll recipe dependencies (trivial). 2018-06-13 mtklein@google.com Revert "simplify SkTFitsIn, try 2" 2018-06-13 mtklein@google.com Revert "use std::enable_if instead of assert()" 2018-06-13 robertphillips@google.com Add --gpuThreads support to skpbench.py 2018-06-13 robertphillips@google.com Pull non-substantive changes out of omnibus CL 2018-06-13 egdaniel@google.com Remove unused code that was used for late mip allocations. 2018-06-13 egdaniel@google.com Fix ImageTest to check if gpu mip maps are supported. 2018-06-13 caryclark@skia.org minor fixes to SkRRect 2018-06-13 khushalsagar@chromium.org fonts: Ignore re-initialization of fallback glyphs from the server. 2018-06-13 mtklein@chromium.org use std::enable_if instead of assert() 2018-06-13 mtklein@chromium.org Does everyone support __has_include() now? 2018-06-13 bungeman@google.com Remove SkMulDiv. 2018-06-13 bungeman@google.com Always FreeType autohint when requested. 2018-06-13 timliang@google.com consolidated writing fields logic and added more builtins for skslc msl backend 2018-06-13 egdaniel@google.com Allow caller to specify if the want mip maps in makeTextureImage call. 2018-06-13 angle-skia-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com Roll third_party/externals/angle2 7ce4a15115cb..3e313805e5a2 (2 commits) 2018-06-13 mtklein@chromium.org simplify SkTFitsIn, try 2 2018-06-13 robertphillips@google.com Fix IsFunctionallyExact Created with: gclient setdep -r src/third_party/skia@657edbede4e3 The AutoRoll server is located here: https://autoroll.skia.org Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;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 BUG= chromium:829622 , chromium:834837 , chromium:849034 TBR=csmartdalton@chromium.org Change-Id: If651c755d8ea6b2cf1bb3c8f84e8709e80f352a4 Reviewed-on: https://chromium-review.googlesource.com/1100316 Reviewed-by: skia-chromium-autoroll <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Commit-Queue: skia-chromium-autoroll <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Cr-Commit-Position: refs/heads/master@{#567125} [modify] https://crrev.com/6b91daa8df790da14d7ba894b7c769a227736653/DEPS
,
Jun 15 2018
Now that the skia changes have all landed, passing this off to ericrk for the chrome side changes.
,
Jun 25 2018
Issue 835978 has been merged into this issue.
,
Jun 28 2018
,
Jul 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc commit 4c5bf9d6c03db357cf4388790132e5d60bb9b3dc Author: Eric Karl <ericrk@chromium.org> Date: Fri Jul 13 08:26:44 2018 Explicitly manage mips in GPU Image Decode Cache. Changes in Skia require that Chrome explicitly specify whether an SkImage needs mips. With this change, we now request mips in two places - at image upload time, if needed immediately, or at image use time, if we need to add them to an existing upload. As mips use additional space and aren't needed for exactly-sized images, we try to avoid adding them unless necessary. To simplify handling, we abstract texture upload, colorspace conversion, and mip generation behind a new MakeTextureImage function. This function provides similar functionality to Skia's SkImage::makeTextureImage, but works around colorspace limitations (Skia's fn ignores colorspaces for now). Bug: 834837 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Change-Id: Ib616c79c800f1b2f32011c225d6e5a6eff00c36c Reviewed-on: https://chromium-review.googlesource.com/1125304 Commit-Queue: Eric Karl <ericrk@chromium.org> Reviewed-by: Khushal <khushalsagar@chromium.org> Cr-Commit-Position: refs/heads/master@{#574861} [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/BUILD.gn [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/paint/decoded_draw_image.cc [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/paint/decoded_draw_image.h [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/paint/image_transfer_cache_entry.cc [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/paint/image_transfer_cache_entry.h [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/paint/paint_op_reader.cc [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/paint/paint_op_writer.cc [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/paint/paint_op_writer.h [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/paint/paint_shader.cc [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/paint/paint_shader.h [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/paint/scoped_raster_flags.cc [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/paint/transfer_cache_unittest.cc [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/test/test_options_provider.cc [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/tiles/gpu_image_decode_cache.cc [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/tiles/gpu_image_decode_cache.h [add] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/tiles/gpu_image_decode_cache_perftest.cc [modify] https://crrev.com/4c5bf9d6c03db357cf4388790132e5d60bb9b3dc/cc/tiles/gpu_image_decode_cache_unittest.cc
,
Jul 13
It appears that many of the bots which originally filed the alert have no new data, but on those that do, we see a full recovery with the fix above. Closing. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Apr 19 2018