[tracing] Fix PartitionAlloc dumper reporting

Source/platform/PartitionAllocMemoryDumpProvider.cpp

Index: Source/platform/PartitionAllocMemoryDumpProvider.cpp
diff --git a/Source/platform/PartitionAllocMemoryDumpProvider.cpp b/Source/platform/PartitionAllocMemoryDumpProvider.cpp
index 3102f5913e52a96595a12099b0260c0b962c77a8..f8a2caf9ed5b87037cce4545968d04766f876e51 100644
--- a/Source/platform/PartitionAllocMemoryDumpProvider.cpp
+++ b/Source/platform/PartitionAllocMemoryDumpProvider.cpp
@@ -16,9 +16,12 @@ namespace {
 
 using namespace WTF;
 
+const char kPartitionAllocDumpName[] = "partition_alloc";
+const char kPartitionsDumpName[] = "partitions";
+
 String getPartitionDumpName(const char* partitionName)
 {
-    return String::format("partition_alloc/thread_%lu/%s", static_cast<unsigned long>(WTF::currentThread()), partitionName);
+    return String::format("%s/%s/%s", kPartitionAllocDumpName, kPartitionsDumpName, partitionName);
 }
 
 // This class is used to invert the dependency of PartitionAlloc on the
@@ -28,38 +31,32 @@ class PartitionStatsDumperImpl final : public PartitionStatsDumper {
 public:
     PartitionStatsDumperImpl(WebProcessMemoryDump* memoryDump, WebMemoryDumpLevelOfDetail levelOfDetail)
         : m_memoryDump(memoryDump)
-        , m_levelOfDetail(levelOfDetail)
-        , m_uid(0) { }
+        , m_uid(0)
+        , m_totalActiveBytes(0) { }
 
     // PartitionStatsDumper implementation.
     void partitionDumpTotals(const char* partitionName, const PartitionMemoryStats*) override;
     void partitionsDumpBucketStats(const char* partitionName, const PartitionBucketMemoryStats*) override;
 
+    const size_t totalActiveBytes() const { return m_totalActiveBytes; }
+
 private:
     WebProcessMemoryDump* m_memoryDump;
-    WebMemoryDumpLevelOfDetail m_levelOfDetail;
-    size_t m_uid;
+    unsigned long m_uid;
+    size_t m_totalActiveBytes;
 };
 
 void PartitionStatsDumperImpl::partitionDumpTotals(const char* partitionName, const PartitionMemoryStats* memoryStats)
 {
+    m_totalActiveBytes += memoryStats->totalActiveBytes;
     String dumpName = getPartitionDumpName(partitionName);
     WebMemoryAllocatorDump* allocatorDump = m_memoryDump->createMemoryAllocatorDump(dumpName);
-    allocatorDump->AddScalar("size", "bytes", memoryStats->totalMmappedBytes);
-    allocatorDump->AddScalar("committed_size", "bytes", memoryStats->totalCommittedBytes);
-    allocatorDump->AddScalar("active_size", "bytes", memoryStats->totalActiveBytes);
-    allocatorDump->AddScalar("resident_size", "bytes", memoryStats->totalResidentBytes);
+    allocatorDump->AddScalar("size", "bytes", memoryStats->totalResidentBytes);

[haraken, 2015/09/07 23:24:26]

Sorry for repeating the discussion again: Do we really want to report a resident
size as "size"? I guess it would be better to report a committed size as "size".

The reason is that it is not easy for an allocator to measure the resident size.
To distinguish the resident size from the committed size, the allocator needs to
keep track of what part of the memory the allocator has touched. PartitionAlloc
distinguishes the two, but I don't think other allocators have an ability to
distinguish them.

For example, in case of Oilpan, Heap::allocatedSpace() is measuring the
committed size (i.e., the amount of memory the allocator has committed), not the
resident size (i.e., the amount of memory the allocator has touched and thus a
physical memory has assigned).

[Primiano Tucci (use gerrit), 2015/09/08 07:56:56]

No worries, better to spend time to make sure we measure the right thing.

Do we really want to report a resident
So, my main question from my side is: what is the difference between the values
reported by PA as "resident" and "committed"?

My understanding, but please correct me if I am wrong, is that at some points
(upon frees) PA discards some pages and hence those pages can still count as
committed but not as resident.
Is this the only difference between committed and resident? What are differences
are there form PA viewpoint between the two?
My main point is: is resident_pages an upper bound on the memory actually used
by PA, or can underestimate it?

I agree with you that an allocator will never know accurately how many pages are
really resident. My goal here is to report the closest upper bound that reflects
the real memory usage (in terms of physical pages stolen from the system).
And I thought that when PA says resident:1M, committed:2M it has enough
information to be sure that at most 1M can be resident, not more (but might be
less if the clients never use that memory, but we will never know in PA)
Yes, in that case sounds like allocatedSpace is already the best upper bound on
used memory we have.
The all point here is: do we have a better bound for PA? Or can "resident" risk
to underestimate memory usage?

[haraken, 2015/09/08 08:07:33]

When PA commits 1 MB of system pages but doesn't yet touch them, the committed
size is 1 MB whereas the resident size is 0 MB. It is common that the committed
size is significantly larger than the resident size.

If you want to report the best upper bound of the resident size, it makes sense
to report the resident size.

My concern is that it will lead to an inconsistent result if some allocators
report the resident size as "size" but other allocators report the committed
size as "size". If PartitionAlloc reports the resident size as "size" but other
allocators report the committed size as "size", PartitionAlloc's memory usage
will be underestimated.

[Primiano Tucci (use gerrit), 2015/09/08 10:22:28]

Let me check if I am getting this right. You are talking about the case of PA
deciding to commit 1 MB to prepare buckets/partition-pages/etc, even if the
memory requested to it by its clients is << 1 MB, right?
In other words, if a client asks fastMalloc(1MB), PA will report a resident_size
of (at lest) 1MB, no matter what the client does with those 1MB, right?
Yes but that feels to me more a "this is the best estimation we can get for
these other allocators".


 If PartitionAlloc reports the resident size as "size" but other
Well the point here is: what do we want to measure here (measure == keep track
on continuous integration waterfalls)? 
What is the troublesome scenario: More objects being requested -> more memory
becoming resident or PA comitting more memory than necessary.
It is defintely very tempting to say "both" (and for telemetry we can actually
track both if we want).
I think here is more a matter of: what number do we think is more important for
people who want to look into memory and should show as first column?

From a Linux/Android viewpoint a +100K resident worries me more than a +1M.

+    allocatorDump->AddScalar("allocated_objects_size", "bytes", memoryStats->totalActiveBytes);
+    allocatorDump->AddScalar("virtual_size", "bytes", memoryStats->totalMmappedBytes);
+    allocatorDump->AddScalar("virtual_committed_size", "bytes", memoryStats->totalCommittedBytes);
     allocatorDump->AddScalar("decommittable_size", "bytes", memoryStats->totalDecommittableBytes);
-    allocatorDump->AddScalar("discardable_size", "bytes", memoryStats->totalDiscardableBytes);
-
-    // If detailed dumps, allocated_objects size will be aggregated from the
-    // children.
-    if (m_levelOfDetail == WebMemoryDumpLevelOfDetail::High)
-        return;
-
-    dumpName = dumpName + "/allocated_objects";
-    WebMemoryAllocatorDump* objectsDump = m_memoryDump->createMemoryAllocatorDump(dumpName);
-    objectsDump->AddScalar("size", "bytes", memoryStats->totalActiveBytes);
+    allocatorDump->AddScalar("freeable_size", "bytes", memoryStats->totalDiscardableBytes);

[haraken, 2015/09/07 23:24:26]

Keep "discardable". We consistently use "discardable" to measure the amount of
(segmented) memory regions where no object resides.

[Primiano Tucci (use gerrit), 2015/09/08 07:56:56]

Hmm ok reverted. I just fear that it might be confused with discardable memory
(ashmem & co)

 }
 
 void PartitionStatsDumperImpl::partitionsDumpBucketStats(const char* partitionName, const PartitionBucketMemoryStats* memoryStats)
@@ -67,26 +64,21 @@ void PartitionStatsDumperImpl::partitionsDumpBucketStats(const char* partitionNa
     ASSERT(memoryStats->isValid);
     String dumpName = getPartitionDumpName(partitionName);
     if (memoryStats->isDirectMap)
-        dumpName.append(String::format("/directMap_%lu", static_cast<unsigned long>(++m_uid)));
+        dumpName.append(String::format("/directMap_%lu", ++m_uid));
     else
         dumpName.append(String::format("/bucket_%u", static_cast<unsigned>(memoryStats->bucketSlotSize)));
 
     WebMemoryAllocatorDump* allocatorDump = m_memoryDump->createMemoryAllocatorDump(dumpName);
     allocatorDump->AddScalar("size", "bytes", memoryStats->residentBytes);
+    allocatorDump->AddScalar("allocated_objects_size", "bytes", memoryStats->activeBytes);
     allocatorDump->AddScalar("slot_size", "bytes", memoryStats->bucketSlotSize);
-    allocatorDump->AddScalar("active_size", "bytes", memoryStats->activeBytes);
-    allocatorDump->AddScalar("resident_size", "bytes", memoryStats->residentBytes);
     allocatorDump->AddScalar("decommittable_size", "bytes", memoryStats->decommittableBytes);
-    allocatorDump->AddScalar("discardable_size", "bytes", memoryStats->discardableBytes);
-    allocatorDump->AddScalar("num_active", "objects", memoryStats->numActivePages);
-    allocatorDump->AddScalar("num_full", "objects", memoryStats->numFullPages);
-    allocatorDump->AddScalar("num_empty", "objects", memoryStats->numEmptyPages);
-    allocatorDump->AddScalar("num_decommitted", "objects", memoryStats->numDecommittedPages);
-    allocatorDump->AddScalar("page_size", "bytes", memoryStats->allocatedPageSize);
-
-    dumpName = dumpName + "/allocated_objects";
-    WebMemoryAllocatorDump* objectsDump = m_memoryDump->createMemoryAllocatorDump(dumpName);
-    objectsDump->AddScalar("size", "bytes", memoryStats->activeBytes);
+    allocatorDump->AddScalar("freeable_size", "bytes", memoryStats->discardableBytes);
+    allocatorDump->AddScalar("total_pages_size", "bytes", memoryStats->allocatedPageSize);
+    allocatorDump->AddScalar("active_pages", "objects", memoryStats->numActivePages);
+    allocatorDump->AddScalar("full_pages", "objects", memoryStats->numFullPages);
+    allocatorDump->AddScalar("empty_pages", "objects", memoryStats->numEmptyPages);
+    allocatorDump->AddScalar("decommitted_pages", "objects", memoryStats->numDecommittedPages);
 }
 
 } // namespace
@@ -97,12 +89,21 @@ PartitionAllocMemoryDumpProvider* PartitionAllocMemoryDumpProvider::instance()
     return &instance;
 }
 
-bool PartitionAllocMemoryDumpProvider::onMemoryDump(WebMemoryDumpLevelOfDetail levelOfDetail, blink::WebProcessMemoryDump* memoryDump)
+bool PartitionAllocMemoryDumpProvider::onMemoryDump(WebMemoryDumpLevelOfDetail levelOfDetail, WebProcessMemoryDump* memoryDump)
 {
     PartitionStatsDumperImpl partitionStatsDumper(memoryDump, levelOfDetail);
 
+    WebMemoryAllocatorDump* partitionsDump = memoryDump->createMemoryAllocatorDump(
+        String::format("%s/%s", kPartitionAllocDumpName, kPartitionsDumpName));
+
     // This method calls memoryStats.partitionsDumpBucketStats with memory statistics.
     WTF::Partitions::dumpMemoryStats(levelOfDetail == WebMemoryDumpLevelOfDetail::Low, &partitionStatsDumper);
+
+    WebMemoryAllocatorDump* allocatedObjectsDump = memoryDump->createMemoryAllocatorDump(
+        String::format("%s/allocated_objects", kPartitionAllocDumpName));

[ssid, 2015/09/07 14:48:21]

Just wondering if the format is right here (12 spaces instead of 8?), unless you
have already run git cl format.

[Primiano Tucci (use gerrit), 2015/09/07 15:23:20]

Looking at other files (and git cl format) this seems the right indentation.

+    allocatedObjectsDump->AddScalar("size", "bytes", partitionStatsDumper.totalActiveBytes());
+    memoryDump->AddOwnershipEdge(allocatedObjectsDump->guid(), partitionsDump->guid());
+
     return true;
 }