Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1369253002:
Add Web Resources usage to chrome://tracing. (Closed)
DescriptionAdd Web Resources usage to chrome://tracing.
The web resources downloaded are stored in MemoryCache. These resources
allocate memory from either PartitionAlloc or Discardable
(PurgeableVector). This CL adds the memory usage of the resources to
tracing.
BUG=520842
Committed: https://crrev.com/695d47eaa44d789ada5e81bfc1758929df52af93
Cr-Commit-Position: refs/heads/master@{#355283}
Patch Set 1 #Patch Set 2 : Nits/. #
Total comments: 19
Patch Set 3 : Addressing comments. #
Total comments: 12
Patch Set 4 : Fixes. #
Total comments: 2
Patch Set 5 : Fix purgeable_size. #Patch Set 6 : Rebase. #Patch Set 7 : Use singleton provider. #
Total comments: 17
Patch Set 8 : Fixes. #
Total comments: 2
Patch Set 9 : Fix names. #Messages
Total messages: 35 (6 generated)
ssid@chromium.org changed reviewers: + haraken@chromium.org
+haraken Just a rough idea of adding MemoryCache to tracing. WDYT?
Thanks for working on this! The lifetime management of Resources are pretty complicated... https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (hasClients()) { This condition is not quite right. Even if the Resource doesn't have a client, it doesn't always mean that it is dead. Actually, the live/dead condition of Resources is extremely complicated (See the implementation of Resource::canDelete()). You should make the condition more correct. You can refer to the implementation of MemoryCache::pruneLiveResources and MemoryCache::pruneDeadResources, which handle the liveness/deadness correctly. https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:736: const bool purgeable = m_data->isLocked() && !wasPurged(); Can't we just use isPurgeable()? https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ScriptResource.cpp (right): https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ScriptResource.cpp:76: void ScriptResource::onMemoryDump(WebProcessMemoryDump* memoryDump) const Why do you specialize for ScriptResource (while you don't specialize for other Resources)? https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/PurgeableVector.cpp (right): https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/PurgeableVector.cpp:87: dump->AddScalar("discardable_size", "bytes", m_discardableSize); This is confusing. Even if m_discardable is non-null, it is not discardable while m_discardable.isLocked() returns true. I think this should be: if (m_discardable) { if (m_discardable.isLocked()) ...; else ...; } else { ...; } https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/PurgeableVector.cpp:90: memoryDump->AddSuballocation(dump->guid(), String(WTF::Partitions::allocatorPoolNameForTracing())); What does allocatorPoolNameForTracing return? (I couldn't find it in a code search.) https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/SharedBuffer.cpp (right): https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/SharedBuffer.cpp:416: m_buffer.onMemoryDump(dumpPrefix + "/buffer", memoryDump); /buffer => /shared_buffer https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/SharedBuffer.cpp:418: if (m_size - m_buffer.size() > 0) { Wouldn't it be enough to check: if (!m_buffer.size()) { // This means that data is in segments. ...; } ? https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/SharedBuffer.cpp:421: const String dataDumpName = dumpPrefix + "/data"; /data => /segments https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/SharedBuffer.cpp:423: dump->AddScalar("size", "bytes", m_size - m_buffer.size()); At this point, I think m_buffer.size() is guaranteed to be 0. So this can be just m_size.
Patchset #3 (id:40001) has been deleted
Thanks for patient review. Actually this CL depends on 3 other CLs which are in review. The base in rietveld only shows one of them. I will add these CL once primiano is fine with the interface changes. I have made changes to the extent I understood the code. What do you think about it now? https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (hasClients()) { On 2015/09/28 14:54:02, haraken wrote: > > This condition is not quite right. Even if the Resource doesn't have a client, > it doesn't always mean that it is dead. Actually, the live/dead condition of > Resources is extremely complicated (See the implementation of > Resource::canDelete()). > > You should make the condition more correct. You can refer to the implementation > of MemoryCache::pruneLiveResources and MemoryCache::pruneDeadResources, which > handle the liveness/deadness correctly. Yes I had checked the MemoryCache::pruneDeadResources. But I also found the MemoryCache::getStatistics, which takes the resource as live if it has clients. This sounded more appropriate. It is more like I don't get the definition of "live" I am not convinced with the conditions like: 1. !m_loader (resource is loading but need not be live) 2. !m_preloadCount - resource is preloaded but no one is using it yet. 3. !m_proxyResource - If there is a proxy updating the resource, why should it be called live, when there are no clients using it. Now changed since the other cases seem imply it is live. https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:736: const bool purgeable = m_data->isLocked() && !wasPurged(); On 2015/09/28 14:54:02, haraken wrote: > > Can't we just use isPurgeable()? Sorry didn't know this existed. There are so many methods to read through. Thanks fixed. https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ScriptResource.cpp (right): https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ScriptResource.cpp:76: void ScriptResource::onMemoryDump(WebProcessMemoryDump* memoryDump) const On 2015/09/28 14:54:02, haraken wrote: > > Why do you specialize for ScriptResource (while you don't specialize for other > Resources)? Ah because the Scripts keep count of the decoded scripts as live resources, and this is not considered as decoded since the cache should not purge it. Also all the other resources store the raw data in "m_data", but the script resources use extra memory. That is why special case. If there are more special cases like this I will update in future, adding more. https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/PurgeableVector.cpp (right): https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/PurgeableVector.cpp:87: dump->AddScalar("discardable_size", "bytes", m_discardableSize); On 2015/09/28 14:54:02, haraken wrote: > > This is confusing. Even if m_discardable is non-null, it is not discardable > while m_discardable.isLocked() returns true. I think this should be: > > if (m_discardable) { > if (m_discardable.isLocked()) > ...; > else > ...; > } else { > ...; > } > I am sorry I screwed up the condition here. Now fixed it. Now it just dumps both, not thinking if locked or not. It means: if discardable it creates a discardable dump, and it creates a normal dump with sub-allocations from partition alloc. This createDiscardableMemoryAllocatorDump is added in the cl http://crrev.com/1372573003. Do you think this is fine? https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/PurgeableVector.cpp:90: memoryDump->AddSuballocation(dump->guid(), String(WTF::Partitions::allocatorPoolNameForTracing())); On 2015/09/28 14:54:02, haraken wrote: > > What does allocatorPoolNameForTracing return? (I couldn't find it in a code > search.) Sorry, this is added in this CL https://codereview.chromium.org/1366203002/. Just returns "partition_alloc/allocated_objects" meaning this is allocated from partitionAlloc. https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/SharedBuffer.cpp (right): https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/SharedBuffer.cpp:416: m_buffer.onMemoryDump(dumpPrefix + "/buffer", memoryDump); On 2015/09/28 14:54:02, haraken wrote: > > /buffer => /shared_buffer Done. https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/SharedBuffer.cpp:418: if (m_size - m_buffer.size() > 0) { On 2015/09/28 14:54:02, haraken wrote: > > Wouldn't it be enough to check: > > if (!m_buffer.size()) { // This means that data is in segments. > ...; > } > > ? I was wondering that both can be used at the same time and data is moved in parts. Changed it now. https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/SharedBuffer.cpp:421: const String dataDumpName = dumpPrefix + "/data"; On 2015/09/28 14:54:02, haraken wrote: > > /data => /segments Done. https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/SharedBuffer.cpp:423: dump->AddScalar("size", "bytes", m_size - m_buffer.size()); On 2015/09/28 14:54:02, haraken wrote: > > At this point, I think m_buffer.size() is guaranteed to be 0. So this can be > just m_size. Yeah I was just paranoid..
https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/SharedBuffer.cpp (right): https://codereview.chromium.org/1369253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/SharedBuffer.cpp:418: if (m_size - m_buffer.size() > 0) { On 2015/09/28 16:24:39, ssid wrote: > On 2015/09/28 14:54:02, haraken wrote: > > > > Wouldn't it be enough to check: > > > > if (!m_buffer.size()) { // This means that data is in segments. > > ...; > > } > > > > ? > > I was wondering that both can be used at the same time and data is moved in > parts. Changed it now. Shall we add an ASSERT about it? https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (!hasClients() && canDelete() && type() != MainResource) { - !hasClients() isn't needed since canDelete() has the check. - What is the type!=MainResource check for? - I guess we can just check canDelete() here. https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:736: dump->AddScalar("purgeable_size", "bytes", isPurgeable() ? 0 : totalSize); It won't make sense to dump both purged_size and purgeable_size. Maybe can we just dump isPurgeable()? BTW, when wasPurged() returns true, does it mean that encodedSize() returns 0? https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.h (right): https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.h:346: // Returns the memory dump name used for tracing. See Resource::onmemoryDump. onMemoryDump https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ScriptResource.cpp (right): https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ScriptResource.cpp:81: dump->AddScalar("size", "bytes", m_script.string().sizeInBytes()); Just to confirm: You're special-casing ScriptResource because m_script is one of the big memory consumers (i.e., this is not related to dumping memory of Resources), right? That makes sense to me. https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/PurgeableVector.cpp (right): https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/PurgeableVector.cpp:86: WebMemoryAllocatorDump* dump = memoryDump->createDiscardableMemoryAllocatorDump(dumpName, m_discardable.get()); Would you add a TODO? TODO(tasak): The discardable memory is locked (i.e., not discardable) while someone keeps a ResourcePtr to the Resource. We shouldn't count the memory as discardable. https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/PurgeableVector.cpp:89: if (m_vector.size() > 0) { It is not possible that both m_discardable and m_vector are used at the same time. See the comment in PurgeableVector.h. Can we change this to 'else'? Also it would be better to add an assert to check that there can't be data in both m_discardable and m_vector.
Patchset #4 (id:80001) has been deleted
Thanks Made changes, expect for the TODO. https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (!hasClients() && canDelete() && type() != MainResource) { On 2015/09/29 02:44:48, haraken wrote: > > - !hasClients() isn't needed since canDelete() has the check. > > - What is the type!=MainResource check for? > > - I guess we can just check canDelete() here. The pruneDeadResources prunes the resources with these checks. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... So, I added the check for Main Resource. Removed it. https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:736: dump->AddScalar("purgeable_size", "bytes", isPurgeable() ? 0 : totalSize); On 2015/09/29 02:44:48, haraken wrote: > > It won't make sense to dump both purged_size and purgeable_size. Maybe can we > just dump isPurgeable()? > > BTW, when wasPurged() returns true, does it mean that encodedSize() returns 0? If was_purged returns true, it meand that discardable memory was evicted from the system. This would mean that the discardable manager accounts 0 for it, and I am not taking the encoded)size as the main size for the dump, I am considering the size returned by discardable manager internally as the size of the buffer. So, yes I can remove the purged_size. https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.h (right): https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.h:346: // Returns the memory dump name used for tracing. See Resource::onmemoryDump. On 2015/09/29 02:44:48, haraken wrote: > > onMemoryDump Done. https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ScriptResource.cpp (right): https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ScriptResource.cpp:81: dump->AddScalar("size", "bytes", m_script.string().sizeInBytes()); On 2015/09/29 02:44:48, haraken wrote: > > Just to confirm: You're special-casing ScriptResource because m_script is one of > the big memory consumers (i.e., this is not related to dumping memory of > Resources), right? That makes sense to me. Yes, it was using extra memory(considerable) apart from the normal resource encoded_size. So, the special case. https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/PurgeableVector.cpp (right): https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/PurgeableVector.cpp:86: WebMemoryAllocatorDump* dump = memoryDump->createDiscardableMemoryAllocatorDump(dumpName, m_discardable.get()); On 2015/09/29 02:44:48, haraken wrote: > > Would you add a TODO? > > TODO(tasak): The discardable memory is locked (i.e., not discardable) while > someone keeps a ResourcePtr to the Resource. We shouldn't count the memory as > discardable. > The locked size of the discardable memory is added in the discardable memory manager soon. So, it will display if the memory is locked or not. (crbug.com/529943). Though the memory is locked, it is still considered as discardable memory as long as the memory is allocated using allocateWebDiscardable() api. I am currently working on showing the locked part of discardable. https://codereview.chromium.org/1369253002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/PurgeableVector.cpp:89: if (m_vector.size() > 0) { On 2015/09/29 02:44:48, haraken wrote: > > It is not possible that both m_discardable and m_vector are used at the same > time. See the comment in PurgeableVector.h. > > Can we change this to 'else'? Also it would be better to add an assert to check > that there can't be data in both m_discardable and m_vector. > I see. Thanks. Made it else.
Thanks, LGTM https://codereview.chromium.org/1369253002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/1369253002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/Resource.cpp:734: dump->AddScalar("purgeable_size", "bytes", isPurgeable() ? encodedSize() + overheadSize() : 0); Maybe we need a bit more tweak on this. Please look at the implementation of MemoryCache::getStatistics() and do the same calculation here.
Yes, Added !purged condition now. Thanks https://codereview.chromium.org/1369253002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/1369253002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/Resource.cpp:734: dump->AddScalar("purgeable_size", "bytes", isPurgeable() ? encodedSize() + overheadSize() : 0); On 2015/09/29 12:36:47, haraken wrote: > > Maybe we need a bit more tweak on this. Please look at the implementation of > MemoryCache::getStatistics() and do the same calculation here. Done.
I made some changes to code. The issue was: 1. The dump providers must be unregistered before platform is destroyed. But, MemoryCache object is never destroyed. 2. I cannot always register MemoryCache because in some tests the object is never created. So, I have introduced a new dump provider, which is singleton. It calls MemoryCache dump, if MemoryCache had been created. This has to be registered and unregistered when platform is initialized and destroyed. But, platform cannot include MemoryCache since it is in core/. So, the registration is done in Webkit.cpp in web/. The dump provider is placed in core/fetch since MemoryCache exists there and to use the cache and the dump provider to be able to use cache, the best place is core/fetch itself. Do you think this change is fine?
On 2015/10/12 16:16:00, ssid wrote: > I made some changes to code. The issue was: > 1. The dump providers must be unregistered before platform is destroyed. But, > MemoryCache object is never destroyed. > 2. I cannot always register MemoryCache because in some tests the object is > never created. > > So, I have introduced a new dump provider, which is singleton. It calls > MemoryCache dump, if MemoryCache had been created. This has to be registered and > unregistered when platform is initialized and destroyed. But, platform cannot > include MemoryCache since it is in core/. So, the registration is done in > Webkit.cpp in web/. > The dump provider is placed in core/fetch since MemoryCache exists there and to > use the cache and the dump provider to be able to use cache, the best place is > core/fetch itself. > > Do you think this change is fine? Maybe can we refactor code so that MemoryCache is always initialized in WebKit::initialize and destroyed in WebKit::shutdown (in preparation for landing this CL)? Then we can just install the dump provider in the WebKit::initialize/shutdown.
> Maybe can we refactor code so that MemoryCache is always initialized in > WebKit::initialize and destroyed in WebKit::shutdown (in preparation for landing > this CL)? Then we can just install the dump provider in the > WebKit::initialize/shutdown. Most tests do not initialize a MemoryCache at all. So, we would be creating an object unnecessarily if the cache is not required at all. Also, I can't find the right way to initialize the Persistent<> object. In tests when clearOutOldGarbage it crashes saying the object header is invalid since it is orphaned. I don't understand when an object is orphaned and why is the object orphaned when I hold a global reference to it. See CL (https://codereview.chromium.org/1399993004/) for what i am trying to do.
https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h (right): https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h:15: class BLINK_PLATFORM_EXPORT WebCacheMemoryDumpProvider final : public WebMemoryDumpProvider { It seems strange to put WebXXX in core/. WebXXX needs to be in platform/ or web/. Can we do something like this? - Put WebCacheMemoryDumpProvider in web/. - Make sure that the MemoryCache is initialized in WebKit::initialize(). - Set the pointer to the MemoryCache (core/) to the WebCacheMemoryDumpProvider (web/) in WebKit::initialize() (web/). https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/Platform.cpp (right): https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/Platform.cpp:58: if (s_platform && s_platform->m_mainThread) I'm just curious but why do we need this change in this CL? (I'm fine with the change itself.)
https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h (right): https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h:15: class BLINK_PLATFORM_EXPORT WebCacheMemoryDumpProvider final : public WebMemoryDumpProvider { On 2015/10/14 00:57:41, haraken wrote: > > It seems strange to put WebXXX in core/. WebXXX needs to be in platform/ or > web/. > > Can we do something like this? > > - Put WebCacheMemoryDumpProvider in web/. > - Make sure that the MemoryCache is initialized in WebKit::initialize(). > - Set the pointer to the MemoryCache (core/) to the WebCacheMemoryDumpProvider > (web/) in WebKit::initialize() (web/). I think it's the name only should be changed here. There is nothing really web about this. It's just a wrapper around MemoryCache. Should I make it MemoryCacheDumpProvider? I placed it here since MemoryCache has to include this file to set cache. It cannot include a file in web/. I could probably move it, but just wondering if it's really better. https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/Platform.cpp (right): https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/Platform.cpp:58: if (s_platform && s_platform->m_mainThread) On 2015/10/14 00:57:41, haraken wrote: > > I'm just curious but why do we need this change in this CL? (I'm fine with the > change itself.) Oh sorry. It came into this cl by mistake. I was making a different cl. Will remove it.
https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h (right): https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h:15: class BLINK_PLATFORM_EXPORT WebCacheMemoryDumpProvider final : public WebMemoryDumpProvider { On 2015/10/14 05:01:22, ssid wrote: > On 2015/10/14 00:57:41, haraken wrote: > > > > It seems strange to put WebXXX in core/. WebXXX needs to be in platform/ or > > web/. > > > > Can we do something like this? > > > > - Put WebCacheMemoryDumpProvider in web/. > > - Make sure that the MemoryCache is initialized in WebKit::initialize(). > > - Set the pointer to the MemoryCache (core/) to the WebCacheMemoryDumpProvider > > (web/) in WebKit::initialize() (web/). > > I think it's the name only should be changed here. There is nothing really web > about this. It's just a wrapper around MemoryCache. Should I make it > MemoryCacheDumpProvider? > > I placed it here since MemoryCache has to include this file to set cache. It > cannot include a file in web/. I could probably move it, but just wondering if > it's really better. The WebCacheMemoryDumpProvider inherits from WebMemoryDumpProvider, which is in public/platform/. Then it looks better to put the WebCacheMemoryDumpProvider in platform/ (not in web/, sorry). What's an issue of putting WebCacheMemoryDumpProvider in platform/?
https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h (right): https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h:15: class BLINK_PLATFORM_EXPORT WebCacheMemoryDumpProvider final : public WebMemoryDumpProvider { On 2015/10/14 05:09:44, haraken wrote: > On 2015/10/14 05:01:22, ssid wrote: > > On 2015/10/14 00:57:41, haraken wrote: > > > > > > It seems strange to put WebXXX in core/. WebXXX needs to be in platform/ or > > > web/. > > > > > > Can we do something like this? > > > > > > - Put WebCacheMemoryDumpProvider in web/. > > > - Make sure that the MemoryCache is initialized in WebKit::initialize(). > > > - Set the pointer to the MemoryCache (core/) to the > WebCacheMemoryDumpProvider > > > (web/) in WebKit::initialize() (web/). > > > > I think it's the name only should be changed here. There is nothing really web > > about this. It's just a wrapper around MemoryCache. Should I make it > > MemoryCacheDumpProvider? > > > > I placed it here since MemoryCache has to include this file to set cache. It > > cannot include a file in web/. I could probably move it, but just wondering if > > it's really better. > > The WebCacheMemoryDumpProvider inherits from WebMemoryDumpProvider, which is in > public/platform/. Then it looks better to put the WebCacheMemoryDumpProvider in > platform/ (not in web/, sorry). > > What's an issue of putting WebCacheMemoryDumpProvider in platform/? Platform cannot use core. So I cannot use memory cache. But the point of the dump provider interface is to have blink classes implement them where ever needed and take dumps. Like in previous case memory cache itself was implementing it.
https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h (right): https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h:15: class BLINK_PLATFORM_EXPORT WebCacheMemoryDumpProvider final : public WebMemoryDumpProvider { On 2015/10/14 06:13:17, ssid wrote: > On 2015/10/14 05:09:44, haraken wrote: > > On 2015/10/14 05:01:22, ssid wrote: > > > On 2015/10/14 00:57:41, haraken wrote: > > > > > > > > It seems strange to put WebXXX in core/. WebXXX needs to be in platform/ > or > > > > web/. > > > > > > > > Can we do something like this? > > > > > > > > - Put WebCacheMemoryDumpProvider in web/. > > > > - Make sure that the MemoryCache is initialized in WebKit::initialize(). > > > > - Set the pointer to the MemoryCache (core/) to the > > WebCacheMemoryDumpProvider > > > > (web/) in WebKit::initialize() (web/). > > > > > > I think it's the name only should be changed here. There is nothing really > web > > > about this. It's just a wrapper around MemoryCache. Should I make it > > > MemoryCacheDumpProvider? > > > > > > I placed it here since MemoryCache has to include this file to set cache. It > > > cannot include a file in web/. I could probably move it, but just wondering > if > > > it's really better. > > > > The WebCacheMemoryDumpProvider inherits from WebMemoryDumpProvider, which is > in > > public/platform/. Then it looks better to put the WebCacheMemoryDumpProvider > in > > platform/ (not in web/, sorry). > > > > What's an issue of putting WebCacheMemoryDumpProvider in platform/? > > Platform cannot use core. So I cannot use memory cache. > But the point of the dump provider interface is to have blink classes implement > them where ever needed and take dumps. Like in previous case memory cache itself > was implementing it. Inheriting public/platform/ from core/ sounds a bit nasty. However, the dependency is technically allowed and tasak@ is saying it will be ok. (To clean up the dependency, we need the blink componentization. Since we decided not to proceed with the componentization because of its opportunity cost, we think it's okay to allow the dependency at the moment.) I'll take a final look shortly.
Sorry about the review iterations. LGTM. https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MemoryCache.cpp (right): https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MemoryCache.cpp:68: *gMemoryCache = cache; You need to call setMemoryCache here as well. https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h (right): https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h:15: class BLINK_PLATFORM_EXPORT WebCacheMemoryDumpProvider final : public WebMemoryDumpProvider { BLINK_PLATFORM_EXPORT => CORE_EXPORT https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h:17: static WebCacheMemoryDumpProvider* instance(); Add a comment on why this needs to be a singleton. https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/SharedBuffer.cpp (right): https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/SharedBuffer.cpp:420: m_buffer.onMemoryDump(dumpPrefix + "/shared_buffer", memoryDump); if (m_buffer.size()) { m_buffer.onMemoryDump(dumpPrefix + "/shared_buffer", memoryDump); } else { ...; } ?
primiano@chromium.org changed reviewers: + primiano@chromium.org
Thanks for the hard work! Can I get a trace to see how this looks like? https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h (right): https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h:23: void setMemoryCache(MemoryCache* memoryCache) { m_memoryCache = memoryCache; } is there a way to check that the thread on which setMemoryCache is called is the same one of onMemoryDUmp (is there an equivalent of ThreadChecker in blink)? If that is not the case this would be racy
Thanks for the review and suggestions. I made changes. I have uploaded a sample trace of web_cache https://x20web.corp.google.com/~ssid/trace_web_cache.json.gz. Thanks. https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MemoryCache.cpp (right): https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MemoryCache.cpp:68: *gMemoryCache = cache; On 2015/10/14 09:04:07, haraken wrote: > > You need to call setMemoryCache here as well. Done. https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h (right): https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h:15: class BLINK_PLATFORM_EXPORT WebCacheMemoryDumpProvider final : public WebMemoryDumpProvider { On 2015/10/14 09:04:07, haraken wrote: > > BLINK_PLATFORM_EXPORT => CORE_EXPORT Done. https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h:17: static WebCacheMemoryDumpProvider* instance(); On 2015/10/14 09:04:07, haraken wrote: > > Add a comment on why this needs to be a singleton. Done. https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/WebCacheMemoryDumpProvider.h:23: void setMemoryCache(MemoryCache* memoryCache) { m_memoryCache = memoryCache; } On 2015/10/14 11:06:58, Primiano Tucci wrote: > is there a way to check that the thread on which setMemoryCache is called is the > same one of onMemoryDUmp (is there an equivalent of ThreadChecker in blink)? > If that is not the case this would be racy Done. https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/SharedBuffer.cpp (right): https://codereview.chromium.org/1369253002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/SharedBuffer.cpp:420: m_buffer.onMemoryDump(dumpPrefix + "/shared_buffer", memoryDump); On 2015/10/14 09:04:07, haraken wrote: > > if (m_buffer.size()) { > m_buffer.onMemoryDump(dumpPrefix + "/shared_buffer", memoryDump); > } else { > ...; > } > > ? Done.
LGTM on my side. Thanks for working on this! The loading team is starting analyzing Resource's memory usage using memory-infra.
Just looked at the trace, the subtraction math looks to partition_alloc looks correct to me (we need to write some page in the wiki though, it is not immediate to understand the effective_size logic in partitionalloc but I think the numbers are right) My only concerns here are: - looks like that "encoded_size" column never tells anything more interesting than the size+overhead itself. SHould we just remove this column? - I guess the decoded size will raise lot of questions by users. My understanding that this is memory not really retained by web cache, and the best explanation for this is "the size of the equivalent decompressed image the last time it got decoded, if did ever had a chance to get decoded" My question here is: is this information useful / actionable or is just going to give false alarm (OMG web cache is using 400 kb of decoded memory?) ? If we want to keep it, can we find a better, less alarming, name? Other than this LGTM, thanks for the hard work. https://codereview.chromium.org/1369253002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/1369253002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/Resource.cpp:738: const String overheadName = dumpName + "/overhead"; by looking at the trace I wonder if "metadata" would be a clearer name here?
On 2015/10/14 13:32:26, Primiano Tucci wrote: > Just looked at the trace, the subtraction math looks to partition_alloc looks > correct to me (we need to write some page in the wiki though, it is not > immediate to understand the effective_size logic in partitionalloc but I think > the numbers are right) > > My only concerns here are: > - looks like that "encoded_size" column never tells anything more interesting > than the size+overhead itself. SHould we just remove this column? encoded size is the total size that was downloaded from the net. That is the reason it exists as attribute. > - I guess the decoded size will raise lot of questions by users. My > understanding that this is memory not really retained by web cache, and the best > explanation for this is "the size of the equivalent decompressed image the last > time it got decoded, if did ever had a chance to get decoded" > My question here is: is this information useful / actionable or is just going to > give false alarm (OMG web cache is using 400 kb of decoded memory?) ? If we want > to keep it, can we find a better, less alarming, name? > That is the reason we have an attribute for it and not included in "size" column. But if you feel we should rename, please suggest a better name. I think decoded_size is a good name since it says the image is decoded and so much is in memory. But not added in "size" since it is not held by the web_cache. > Other than this LGTM, thanks for the hard work. > > https://codereview.chromium.org/1369253002/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/fetch/Resource.cpp (right): > > https://codereview.chromium.org/1369253002/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/fetch/Resource.cpp:738: const String overheadName > = dumpName + "/overhead"; > by looking at the trace I wonder if "metadata" would be a clearer name here? "metadata" sounds good to me.
On 2015/10/14 15:05:35, ssid wrote: > On 2015/10/14 13:32:26, Primiano Tucci wrote: > > Just looked at the trace, the subtraction math looks to partition_alloc looks > > correct to me (we need to write some page in the wiki though, it is not > > immediate to understand the effective_size logic in partitionalloc but I think > > the numbers are right) > > > > My only concerns here are: > > - looks like that "encoded_size" column never tells anything more interesting > > than the size+overhead itself. SHould we just remove this column? > > encoded size is the total size that was downloaded from the net. That is the > reason it exists as attribute. > > > - I guess the decoded size will raise lot of questions by users. My > > understanding that this is memory not really retained by web cache, and the > best > > explanation for this is "the size of the equivalent decompressed image the > last > > time it got decoded, if did ever had a chance to get decoded" > > My question here is: is this information useful / actionable or is just going > to > > give false alarm (OMG web cache is using 400 kb of decoded memory?) ? If we > want > > to keep it, can we find a better, less alarming, name? > > > > That is the reason we have an attribute for it and not included in "size" > column. But if you feel we should rename, please suggest a better name. I think > decoded_size is a good name since it says the image is decoded and so much is in > memory. But not added in "size" since it is not held by the web_cache. > > > Other than this LGTM, thanks for the hard work. > > > > > https://codereview.chromium.org/1369253002/diff/180001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/fetch/Resource.cpp (right): > > > > > https://codereview.chromium.org/1369253002/diff/180001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/fetch/Resource.cpp:738: const String > overheadName > > = dumpName + "/overhead"; > > by looking at the trace I wonder if "metadata" would be a clearer name here? > > "metadata" sounds good to me. haraken@ do you have any suggestion on these fields / naming?
On 2015/10/15 15:26:02, ssid wrote: > On 2015/10/14 15:05:35, ssid wrote: > > On 2015/10/14 13:32:26, Primiano Tucci wrote: > > > Just looked at the trace, the subtraction math looks to partition_alloc > looks > > > correct to me (we need to write some page in the wiki though, it is not > > > immediate to understand the effective_size logic in partitionalloc but I > think > > > the numbers are right) > > > > > > My only concerns here are: > > > - looks like that "encoded_size" column never tells anything more > interesting > > > than the size+overhead itself. SHould we just remove this column? > > > > encoded size is the total size that was downloaded from the net. That is the > > reason it exists as attribute. > > > > > - I guess the decoded size will raise lot of questions by users. My > > > understanding that this is memory not really retained by web cache, and the > > best > > > explanation for this is "the size of the equivalent decompressed image the > > last > > > time it got decoded, if did ever had a chance to get decoded" > > > My question here is: is this information useful / actionable or is just > going > > to > > > give false alarm (OMG web cache is using 400 kb of decoded memory?) ? If we > > want > > > to keep it, can we find a better, less alarming, name? > > > > > > > That is the reason we have an attribute for it and not included in "size" > > column. But if you feel we should rename, please suggest a better name. I > think > > decoded_size is a good name since it says the image is decoded and so much is > in > > memory. But not added in "size" since it is not held by the web_cache. > > > > > Other than this LGTM, thanks for the hard work. > > > > > > > > > https://codereview.chromium.org/1369253002/diff/180001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/fetch/Resource.cpp (right): > > > > > > > > > https://codereview.chromium.org/1369253002/diff/180001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/fetch/Resource.cpp:738: const String > > overheadName > > > = dumpName + "/overhead"; > > > by looking at the trace I wonder if "metadata" would be a clearer name here? > > > > "metadata" sounds good to me. > > haraken@ do you have any suggestion on these fields / naming? I don't have a strong opinion :) It would be better to use the same word as the other allocator.
OK, so considering that ssid is out for 4 days, I'd land this with your permission to avoid that it gets bitrotten and then we can fix names in a follow-up cl.
I removed the decoded size and changed the overhead to metadata. The encoded size is kept because it could also tell information about what is downloaded but discarded by discardable manager. https://codereview.chromium.org/1369253002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/1369253002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/Resource.cpp:738: const String overheadName = dumpName + "/overhead"; On 2015/10/14 13:32:25, Primiano Tucci wrote: > by looking at the trace I wonder if "metadata" would be a clearer name here? Done.
On 2015/10/21 12:42:57, ssid wrote: > I removed the decoded size and changed the overhead to metadata. The encoded > size is kept because it could also tell information about what is downloaded but > discarded by discardable manager. > > https://codereview.chromium.org/1369253002/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/fetch/Resource.cpp (right): > > https://codereview.chromium.org/1369253002/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/fetch/Resource.cpp:738: const String overheadName > = dumpName + "/overhead"; > On 2015/10/14 13:32:25, Primiano Tucci wrote: > > by looking at the trace I wonder if "metadata" would be a clearer name here? > > Done. I will landing this now. If there are further issues i will fix up in future patches, Thanks.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1369253002/#ps200001 (title: "Fix names.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1369253002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1369253002/200001
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/695d47eaa44d789ada5e81bfc1758929df52af93 Cr-Commit-Position: refs/heads/master@{#355283} |
