tracing: Report ResourceClients and the reason for not deletable for web-caches

third_party/WebKit/Source/core/fetch/Resource.cpp

Index: third_party/WebKit/Source/core/fetch/Resource.cpp
diff --git a/third_party/WebKit/Source/core/fetch/Resource.cpp b/third_party/WebKit/Source/core/fetch/Resource.cpp
index aade7d714ec9492a3d1384bb3df6da8705d75448..70aac9e5eda90f81a6f1adc97af559eaeea7cfa3 100644
--- a/third_party/WebKit/Source/core/fetch/Resource.cpp
+++ b/third_party/WebKit/Source/core/fetch/Resource.cpp
@@ -521,6 +521,56 @@ bool Resource::canDelete() const
         && !m_protectorCount;
 }
 
+String Resource::reasonNotDeletable() const
+{
+    StringBuilder builder;
+    if (hasClients()) {
+        builder.append("hasClients(");
+        builder.appendNumber(m_clients.size());
+        if (!m_clientsAwaitingCallback.isEmpty()) {
+            builder.append(", AwaitingCallback=");
+            builder.appendNumber(m_clientsAwaitingCallback.size());
+        }
+        if (!m_finishedClients.isEmpty()) {
+            builder.append(", Finished=");
+            builder.appendNumber(m_finishedClients.size());
+        }
+        builder.append(")");
+    }
+    if (m_loader) {
+        if (!builder.isEmpty())
+            builder.append(' ');
+        builder.append("m_loader");

[ssid, 2015/10/30 07:00:31]

Can we get rid of m_ and display more useful info for common user. Maybe
"loader_exists"?

[hiroshige, 2015/10/30 07:34:33]

I made the names to directly corresponding to the variable names and
canDelete(), because I think
- it is clearer for Chromium developers
- it is hard to make the description fully understandable for "common users" --
Resource lifetime management is quite complex, and anyway requires reading
source code to understand it.

One possible way is to use class names where applicable:
  hasClients -> ResourceClient
  m_handleCount -> ResourcePtr
  m_protectorCount -> InternalResourcePtr
which will make the names and the column name of "ResourceClient" (L840)
consistent.

Added +loading-reviews@.
How do you think? Any good names?

[kinuko, 2015/11/02 07:41:57]

Either looks fine with me given that it seems difficult anyway for 'common
users' to understand the meaning of these values. If we're going to show this as
something like TRACE_EVENT_OBJECT_SNAPSHOT with json UI support that sounds good
to me too.

[ssid, 2015/11/04 12:12:57]

Sorry for the late reply. It is fine as it is, since it is difficult for others
to understand anyway and more useful for developers.

+    }
+    if (m_preloadCount) {
+        if (!builder.isEmpty())
+            builder.append(' ');
+        builder.append("m_preloadCount(");

[ssid, 2015/10/30 07:00:31]

preloaded_with_count or just preloaded, and similar names below..

+        builder.appendNumber(m_preloadCount);
+        builder.append(")");
+    }
+    if (!hasRightHandleCountApartFromCache(0)) {
+        if (!builder.isEmpty())
+            builder.append(' ');
+        builder.append("m_handleCount(");
+        builder.appendNumber(m_handleCount);
+        builder.append(")");
+    }
+    if (m_protectorCount) {
+        if (!builder.isEmpty())
+            builder.append(' ');
+        builder.append("m_protectorCount(");
+        builder.appendNumber(m_protectorCount);
+        builder.append(")");
+    }
+    if (memoryCache()->contains(this)) {
+        if (!builder.isEmpty())
+            builder.append(' ');
+        builder.append("memoryCache");

[ssid, 2015/10/30 07:00:31]

reason_not_deletable : memoryCache -> in_memory_cache?

[hiroshige, 2015/10/30 07:34:33]

Acknowledged.

[hiroshige, 2015/11/09 20:53:58]

Done.

+    }
+    return builder.toString();
+}
+
 bool Resource::hasOneHandle() const
 {
     return hasRightHandleCountApartFromCache(1);
@@ -738,6 +788,7 @@ void Resource::prune()
 void Resource::onMemoryDump(WebMemoryDumpLevelOfDetail levelOfDetail, WebProcessMemoryDump* memoryDump) const
 {
     static const size_t kMaxURLReportLength = 128;
+    static const int kMaxResourceClientToShowInMemoryInfra = 10;
 
     const String dumpName = getMemoryDumpName();
     WebMemoryAllocatorDump* dump = memoryDump->createMemoryAllocatorDump(dumpName);
@@ -760,6 +811,33 @@ void Resource::onMemoryDump(WebMemoryDumpLevelOfDetail levelOfDetail, WebProcess
             urlToReport = urlToReport + "...";
         }
         dump->addString("url", "", urlToReport);
+
+        dump->addString("not_deletable_because", "", reasonNotDeletable());

[ssid, 2015/10/30 07:00:31]

I think it is better to show "reason_not_deletable" or something like that,
instead of open phrase "not_deletable_because". What if that column is empty?

[hiroshige, 2015/11/09 20:53:58]

Done.

+
+        Vector<String> clientNames;
+        ResourceClientWalker<ResourceClient> w(m_clients);
+        while (ResourceClient* c = w.next())
+            clientNames.append(c->debugName());
+        ResourceClientWalker<ResourceClient> w2(m_clientsAwaitingCallback);
+        while (ResourceClient* c = w2.next())
+            clientNames.append("(awaiting) " + c->debugName());
+        ResourceClientWalker<ResourceClient> w3(m_finishedClients);
+        while (ResourceClient* c = w3.next())
+            clientNames.append("(finished) " + c->debugName());
+        std::sort(clientNames.begin(), clientNames.end(), codePointCompareLessThan);
+
+        StringBuilder builder;
+        for (size_t i = 0; i < clientNames.size() && i < kMaxResourceClientToShowInMemoryInfra; ++i) {
+            if (i > 0)
+                builder.append(" / ");
+            builder.append(clientNames[i]);
+        }
+        if (clientNames.size() > kMaxResourceClientToShowInMemoryInfra) {
+            builder.append(" / and ");
+            builder.appendNumber(clientNames.size() - kMaxResourceClientToShowInMemoryInfra);
+            builder.append(" more");
+        }
+        dump->addString("ResourceClient", "", builder.toString());
     }
 
     const String overheadName = dumpName + "/metadata";