Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1265183002:
Add memory tracking to TexturePool. (Closed)
DescriptionAdd memory tracking to TexturePool.
Basic memory tracking logic for TexturePool. Will expand this once
ResourcePool is set up to track vended out resources.
BUG=512534
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/cfb031dd373e8bdc765d1e8d9694d203a264d745
Cr-Commit-Position: refs/heads/master@{#342154}
Patch Set 1 : #
Total comments: 14
Patch Set 2 : review feedback #
Total comments: 3
Patch Set 3 : #Patch Set 4 : rebase #Patch Set 5 : rebase #
Messages
Total messages: 24 (13 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
ericrk@chromium.org changed reviewers: + reveman@chromium.org
Small update to track memory pool usage.
https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... File cc/resources/resource_pool.cc (right): https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... cc/resources/resource_pool.cc:179: std::string prefix = base::StringPrintf("cc/resource_memory/pool_%" PRIx64, hm, what's a good name that describes what this memory is used for? It's used for tiles and tiles only once https://codereview.chromium.org/1230203007 lands. cc/tile_memory or cc/tile_resource_memory maybe? and we can rename this class to TileResourcePool/Allocator/Manager or something like that in a follow up patch. Wdyt? https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... cc/resources/resource_pool.cc:180: reinterpret_cast<uint64_t>(this)); hm, I guess we're missing a general mechanism for distinguishing between different compositors. Doesn't cc/resource_memory/resource_%d have this same problem? Should we fix that first? https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... cc/resources/resource_pool.cc:182: DumpResourceList(pmd, prefix + "/busy", busy_resources_); Instead of /unused + /busy, can you add a free_size attribute that is the same as the buffer size for unused buffers. Take a look at what I did in this patch: https://codereview.chromium.org/1230203007 https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... cc/resources/resource_pool.cc:195: base::StringPrintf("CC/resource_memory/resource_%d", resource.id()); s/CC/cc/ https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... File cc/resources/resource_pool.h (right): https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... cc/resources/resource_pool.h:55: // base::trace_event::MemoryDumpProvider implementation. nit: "// Overridden from base::trace_event::MemoryDumpProvider:" is more common in cc/ https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... cc/resources/resource_pool.h:76: const ResourceList& resource_list); Maybe use the same pattern as discardable memory and one-copy here. That is to add a "OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd, ...) const" function to the PoolResource struct and then use std::for_each.
Patchset #2 (id:120001) has been deleted
https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... File cc/resources/resource_pool.cc (right): https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... cc/resources/resource_pool.cc:179: std::string prefix = base::StringPrintf("cc/resource_memory/pool_%" PRIx64, On 2015/08/04 22:42:37, reveman wrote: > hm, what's a good name that describes what this memory is used for? It's used > for tiles and tiles only once https://codereview.chromium.org/1230203007 lands. > cc/tile_memory or cc/tile_resource_memory maybe? and we can rename this class to > TileResourcePool/Allocator/Manager or something like that in a follow up patch. > Wdyt? tile_memory seems fine, I wasn't focusing on the fact that this was non-generic in the future - was assuming we'd need lots of arbitrary pools. https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... cc/resources/resource_pool.cc:180: reinterpret_cast<uint64_t>(this)); On 2015/08/04 22:42:37, reveman wrote: > hm, I guess we're missing a general mechanism for distinguishing between > different compositors. Doesn't cc/resource_memory/resource_%d have this same > problem? Should we fix that first? Yeah - wasn't thinking that there would be one pool per compositor, was just thinking that different pools could be in play so we'd want a generic way of tagging up to N pools. If we have exactly one pool, it makes sense to split this up per compositor... For now, putting these under tile_memory, will handle doing per-compositor stuff (if needed) in a separate CL. https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... cc/resources/resource_pool.cc:182: DumpResourceList(pmd, prefix + "/busy", busy_resources_); On 2015/08/04 22:42:37, reveman wrote: > Instead of /unused + /busy, can you add a free_size attribute that is the same > as the buffer size for unused buffers. Take a look at what I did in this patch: > https://codereview.chromium.org/1230203007 yup, much nicer, good call. https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... cc/resources/resource_pool.cc:195: base::StringPrintf("CC/resource_memory/resource_%d", resource.id()); On 2015/08/04 22:42:37, reveman wrote: > s/CC/cc/ Done. https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... File cc/resources/resource_pool.h (right): https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... cc/resources/resource_pool.h:55: // base::trace_event::MemoryDumpProvider implementation. On 2015/08/04 22:42:37, reveman wrote: > nit: "// Overridden from base::trace_event::MemoryDumpProvider:" is more common > in cc/ Done. https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... cc/resources/resource_pool.h:76: const ResourceList& resource_list); On 2015/08/04 22:42:37, reveman wrote: > Maybe use the same pattern as discardable memory and one-copy here. That is to > add a "OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd, ...) const" > function to the PoolResource struct and then use std::for_each. Done - not sure std::for_each is actually more readable than just using range-based for (similar amount of code, but less lambdas)
lgtm with nit https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... File cc/resources/resource_pool.cc (right): https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... cc/resources/resource_pool.cc:179: std::string prefix = base::StringPrintf("cc/resource_memory/pool_%" PRIx64, On 2015/08/05 at 00:19:27, ericrk wrote: > On 2015/08/04 22:42:37, reveman wrote: > > hm, what's a good name that describes what this memory is used for? It's used > > for tiles and tiles only once https://codereview.chromium.org/1230203007 lands. > > cc/tile_memory or cc/tile_resource_memory maybe? and we can rename this class to > > TileResourcePool/Allocator/Manager or something like that in a follow up patch. > > Wdyt? > > tile_memory seems fine, I wasn't focusing on the fact that this was non-generic in the future - was assuming we'd need lots of arbitrary pools. Yea, I think this is just pointing out an old design flaw. This always needed a better name and now that we're trying to trace the memory it just becomes more obvious. https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... File cc/resources/resource_pool.h (right): https://codereview.chromium.org/1265183002/diff/100001/cc/resources/resource_... cc/resources/resource_pool.h:76: const ResourceList& resource_list); On 2015/08/05 at 00:19:27, ericrk wrote: > On 2015/08/04 22:42:37, reveman wrote: > > Maybe use the same pattern as discardable memory and one-copy here. That is to > > add a "OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd, ...) const" > > function to the PoolResource struct and then use std::for_each. > > Done - not sure std::for_each is actually more readable than just using range-based for (similar amount of code, but less lambdas) Yes, range-based loop is perfectly fine. https://codereview.chromium.org/1265183002/diff/140001/cc/resources/resource_... File cc/resources/resource_pool.cc (right): https://codereview.chromium.org/1265183002/diff/140001/cc/resources/resource_... cc/resources/resource_pool.cc:193: void ResourcePool::PoolResource::OnMemoryDump( nit: definition of nested class usually go before definition of enclosing class https://codereview.chromium.org/1265183002/diff/140001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1265183002/diff/140001/cc/resources/resource_... cc/resources/resource_provider.cc:1976: // TODO(ericrk): Add per-compositor ID in name. A simple way to solve this is to replace ResourceProvider::next_id_ with "base::StaticAtomicSequenceNumber g_next_resource_id"
https://codereview.chromium.org/1265183002/diff/140001/cc/resources/resource_... File cc/resources/resource_pool.cc (right): https://codereview.chromium.org/1265183002/diff/140001/cc/resources/resource_... cc/resources/resource_pool.cc:193: void ResourcePool::PoolResource::OnMemoryDump( On 2015/08/05 01:26:59, reveman wrote: > nit: definition of nested class usually go before definition of enclosing class Done.
The CQ bit was checked by ericrk@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1265183002/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1265183002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1265183002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ericrk@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1265183002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1265183002/180001
The CQ bit was checked by ericrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1265183002/#ps180002 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1265183002/180002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1265183002/180002
Message was sent while issue was closed.
Committed patchset #5 (id:180002)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/cfb031dd373e8bdc765d1e8d9694d203a264d745 Cr-Commit-Position: refs/heads/master@{#342154} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
