Issue metadata
Sign in to add a comment
|
4.9%-46.2% regression in system_health.memory_mobile at 535734:535839 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Feb 12 2018
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/11d10d6d840000
,
Feb 13 2018
📍 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
,
Feb 13 2018
,
Feb 13 2018
,
Feb 13 2018
,
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()).
,
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
,
Feb 15 2018
Issue 812365 has been merged into this issue.
,
Feb 15 2018
Darn, that CL didn't fix the graphs. Hmmm
,
Feb 20 2018
The same number of resources are being dumped. But the resources are each larger now?
,
Feb 20 2018
One possibility, one-copy resources are being allocated for framebuffer attachment now, which they were not before, and was a mistake.
,
Feb 20 2018
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.
,
Feb 20 2018
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.
,
Feb 20 2018
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.
,
Feb 20 2018
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.
,
Feb 20 2018
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.
,
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
,
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
,
Feb 21 2018
https://chromeperf.appspot.com/report?sid=8825ae9dd9fefbf9deb75622118fa404e8afca7dca62f15dae2587fa644fa696&rev=535839 recovered, yay |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Feb 12 2018