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

Issue 811432 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

4.9%-46.2% regression in system_health.memory_mobile at 535734:535839

Project Member Reported by sullivan@chromium.org, Feb 12 2018

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Feb 12 2018

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

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


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

android-one
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Feb 13 2018

Cc: danakj@chromium.org sunn...@chromium.org ericrk@chromium.org
Owner: danakj@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/11d10d6d840000

Move ownership of gpu texture resources to ResourcePool by danakj@chromium.org
https://chromium.googlesource.com/chromium/src/+/af3170e5f40955165ffea7a5dd0d8032a9e012f5

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Comment 5 by danakj@chromium.org, Feb 13 2018

Cc: kylec...@chromium.org xing.xu@chromium.org piman@chromium.org

Comment 6 by danakj@chromium.org, Feb 13 2018

Cc: wolenetz@chromium.org
 Issue 811538  has been merged into this issue.

Comment 7 by danakj@chromium.org, Feb 14 2018

Previously the CreateMailbox call was done on the main thread in AcquireBufferForRaster, after which ResourceProvider would dump the resource. Now the texture_id is allocated in AcquireBufferForRaster, and we give a GUID based on that texture id, so it should dump at the same times before/after. (Dump can't happen between these 2 events cuz TileManager on the compositor thread does them in the same call stack in TileManager::CreateRasterTask().)

However CreateRasterTask can abort allocating a resource. In that case, we have no texture id so we could be dumping for 0 GUIDs. We do that if (has_at_raster_images && tile->is_prepaint()).
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 14 2018

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

commit 50fc16efa60869e2f49d5458810694479ed277f6
Author: danakj <danakj@chromium.org>
Date: Wed Feb 14 21:49:48 2018

Don't memory dump PoolResources until they have storage allocated.

It's possible for TileManager to grab a resource from the pool, but
then not use it. Also, until the raster task actually runs, there is
no memory allocated. So don't return a guid until a texture id is
created and storage is allocated for it.

R=ericrk@chromium.org, vmpstr@chromium.org

Bug:  811432 , 730660 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I9a33a66746c5c95a6ae9bdc363c784adbeb949bc
Reviewed-on: https://chromium-review.googlesource.com/919313
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536831}
[modify] https://crrev.com/50fc16efa60869e2f49d5458810694479ed277f6/cc/raster/gpu_raster_buffer_provider.cc
[modify] https://crrev.com/50fc16efa60869e2f49d5458810694479ed277f6/cc/raster/one_copy_raster_buffer_provider.cc

Comment 9 by danakj@chromium.org, Feb 15 2018

 Issue 812365  has been merged into this issue.
Darn, that CL didn't fix the graphs. Hmmm
The same number of resources are being dumped. But the resources are each larger now?
before cc.png
67.3 KB View Download
after cc.png
66.8 KB View Download
One possibility, one-copy resources are being allocated for framebuffer attachment now, which they were not before, and was a mistake.
I don't think that would cause this though. The number reported to tracing is:

  uint64_t total_bytes =
      ResourceUtil::UncheckedSizeInBytesAligned<size_t>(size_, format_);
  dump->AddScalar(MemoryAllocatorDump::kNameSize,
                  MemoryAllocatorDump::kUnitsBytes, total_bytes);

Which depends only on the resource's size_ and format_. So maybe one of those (format_?) changed.
Although it uses a shared guid with a texture reported from the gpu side, and maybe it has a different memory size and reports that and it picks that.
Looking at before/after traces, the numbers logged from the browser seem a bit weird:
It appears that resources logged from the Browser's resource provider are 2x the size they used to be. This is independent of any issues with aliasing across processes (that appears to be working fine). Will keep looking.
Ok, looks like this isn't a reporting issue, but an issue with format tracking (I think):

On low-end Android-one, where we're seeing this issue, we should be rasterizing to 16bpp images (not 32bpp). The renderer seems to be doing this correctly, but the browser resource provider seems to be treating these resources as 32bpp and sizing based on that for the dumps.

Note that the GPU process (which should be most accurate) dumps these resources as 16bpp, so I think this is just a tracking issue.
Thanks so much for the help, I had only been looking at the browser traces. We are indeed omitting the format when exporting the resource from ResourcePool.
Project Member

Comment 19 by bugdroid1@chromium.org, Feb 20 2018

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

commit abc59ef67aad37a5e929cb5e98bcfca4890ffb77
Author: danakj <danakj@chromium.org>
Date: Tue Feb 20 20:54:13 2018

Make ResourcePool export the resource format on TransferableResource

This format isn't part of the TransferableResource::MakeFoo() helpers
but is important nonetheless for memory accounting, and would be
important for software resources. For GL the format is known in the
service so it ends up not being used for correctness.

R=ericrk@chromium.org

Bug:  811432 ,  730660 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Ibaacf987cbbdb1cc17ad624325c843aca7ba7230
Reviewed-on: https://chromium-review.googlesource.com/927021
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537874}
[modify] https://crrev.com/abc59ef67aad37a5e929cb5e98bcfca4890ffb77/cc/resources/resource_pool.cc
[modify] https://crrev.com/abc59ef67aad37a5e929cb5e98bcfca4890ffb77/components/viz/common/resources/transferable_resource.h

Project Member

Comment 20 by bugdroid1@chromium.org, Feb 20 2018

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

commit 729d39efe011c30ab04a68905c0d3b1e173224f1
Author: danakj <danakj@chromium.org>
Date: Tue Feb 20 21:18:48 2018

OneCopy resources should not be allocated for framebuffer attachment

The ResourcePool used to be created for OneCopy mode with a kDefault
ResourceTextureHint, rather than kFramebuffer. This was a copy-paste
from GpuRasterBufferProvider, which does want to use textures for
framebuffer attachment.

R=vmpstr@chromium.org

Bug:  811432 ,  730660 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Ibfba07d0f8834ecdd7160d5e80d8e4d5bb544706
Reviewed-on: https://chromium-review.googlesource.com/926723
Reviewed-by: Eric Karl <ericrk@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537883}
[modify] https://crrev.com/729d39efe011c30ab04a68905c0d3b1e173224f1/cc/raster/one_copy_raster_buffer_provider.cc

Sign in to add a comment