New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 834837 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 13
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Regression

Blocking:
issue 850347



Sign in to add a comment

7.7%-1171.1% regression in system_health.memory_mobile at 551053:551217

Project Member Reported by kouhei@google.com, Apr 19 2018

Issue description

Looks like this is from probe change though
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Apr 19 2018

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

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=97d4e3001c2b588c3a2845fa5d0c096fe8eefcec4e605a7e33cb1935cf5e94a5


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

android-nexus5
chromium-rel-mac11-air
chromium-rel-mac12
chromium-rel-win10
chromium-rel-win7-gpu-ati
chromium-rel-win7-x64-dual
linux-release
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Apr 19 2018

Cc: egdaniel@google.com robertph...@google.com
Status: Assigned (was: Untriaged)
📍 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
Owner: egdaniel@chromium.org
egdaniel: any update on this bug? This is a pretty big memory regression.
Cc: bsalomon@chromium.org ericrk@chromium.org
+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.
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?
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.
This all sounds good to me. If you have bandwidth to add the Skia API I'm happy to wire it up in Chrome.
Yeah I'll get the API done on the Skia side.

Comment 10 by zmo@chromium.org, Jun 7 2018

Blocking: 850347
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Project Member

Comment 12 by bugdroid1@chromium.org, 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

Owner: ericrk@chromium.org
Now that the skia changes have all landed, passing this off to ericrk for the chrome side changes.
Issue 835978 has been merged into this issue.
Labels: -Pri-2 Pri-1
Status: Started (was: Assigned)
Project Member

Comment 16 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
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