[tracing] Fix PartitionAlloc dumper reporting

[tracing] Fix PartitionAlloc dumper reporting

- Fix virtual vs resident size reporting in the "size" column:
  Prior to this change the PartitionAlloc dumper was reporting the
  mmap-ed (virtual address spave) size in the "size" column.
  This is inconsistent with the rest of the dumpers, which strive
  to report an estimation of actually resident memory in the "size"
  column.
  This change moves the PA resident size into "size".   
- Remove the "thread_1234" sub-node: turns out that there can be at
  most one PartitionAlloc instance per-process, even in single-process
  mode.
- Improve reporting of allocated_objects:
  - Each bucket gets a column "allocated_objects_size", previously 
    called "active_size" (confusing), which reports the size of bytes
    requested by PA clients.
  - The per-bucket "allocated_objects" sub-rows are removed: they just
    added visual clutter and didn't add any extra information than an
    extra column.
  - A global allocated_objects node is introduced. This will be required
    by subsequent CLs to properly account memory reporter by other pieces
    of blink. This will be to avoid that WebCache memory that is 
    allocated via PA gets double-counted in the effective_size.
- Improve other column names for sake of clarity:
   - committed -> virtual_committed
   - num_{active,full,...} -> {active,full,...}_pages. The previous name
     didn't make it clear that they count pages and not objects.

BUG=488472
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201972

[Primiano Tucci (use gerrit), 2015-09-07 13:03:46]

primiano@chromium.org changed reviewers:
+ ssid@chromium.org

[Primiano Tucci (use gerrit), 2015-09-07 13:03:47]

ssid, can I ask you a first review round before I bring this up with PA owners?

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

Thanks, LGTM.

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
File Source/platform/PartitionAllocMemoryDumpProvider.cpp (right):

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
Source/platform/PartitionAllocMemoryDumpProvider.cpp:103:
String::format("%s/allocated_objects", kPartitionAllocDumpName));
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:19]

primiano@chromium.org changed reviewers:
+ haraken@chromium.org

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

Hey Kentaro,
as anticipated here are the changes to PAlloc to make it more legible and
consistent with the rest of the dumpers.
You can see an actual screenshot of the final effect on
www/~primiano/memory_infra/misc/palloc_refactor_cl_1312843010.png


+bashi and perezju as this CL might affect telemetry data (as size now counts
the resident pages, not mmaped).

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
File Source/platform/PartitionAllocMemoryDumpProvider.cpp (right):

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
Source/platform/PartitionAllocMemoryDumpProvider.cpp:103:
String::format("%s/allocated_objects", kPartitionAllocDumpName));
On 2015/09/07 14:48:21, ssid wrote:
> Just wondering if the format is right here (12 spaces instead of 8?), unless
you
> have already run git cl format.

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

[Primiano Tucci (use gerrit), 2015-09-07 15:26:21]

As far as telemetry is concerned, I double-checked with Juan and the only effect
is that memory_dump_event.py will start counting the resident size and not the
mmaped one (which is what we want to achieve).

[ssid, 2015-09-07 15:59:26]

one nit.

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
File Source/platform/PartitionAllocMemoryDumpProvider.cpp (right):

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
Source/platform/PartitionAllocMemoryDumpProvider.cpp:11: #include
"wtf/Threading.h"
this can be removed now.

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

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
File Source/platform/PartitionAllocMemoryDumpProvider.cpp (right):

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
Source/platform/PartitionAllocMemoryDumpProvider.cpp:54:
allocatorDump->AddScalar("size", "bytes", memoryStats->totalResidentBytes);

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).

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
Source/platform/PartitionAllocMemoryDumpProvider.cpp:59:
allocatorDump->AddScalar("freeable_size", "bytes",
memoryStats->totalDiscardableBytes);

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

https://codereview.chromium.org/1312843010/diff/20001/Source/wtf/Partitions.cpp
File Source/wtf/Partitions.cpp (right):

https://codereview.chromium.org/1312843010/diff/20001/Source/wtf/Partitions.c...
Source/wtf/Partitions.cpp:121: partitionDumpStatsGeneric(fastMallocPartition(),
"fast_malloc", isLightDump, partitionStatsDumper);

I'd keep "_partition" for clarity.

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

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
File Source/platform/PartitionAllocMemoryDumpProvider.cpp (right):

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
Source/platform/PartitionAllocMemoryDumpProvider.cpp:11: #include
"wtf/Threading.h"
On 2015/09/07 15:59:26, ssid wrote:
> this can be removed now.

Done.

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
Source/platform/PartitionAllocMemoryDumpProvider.cpp:54:
allocatorDump->AddScalar("size", "bytes", memoryStats->totalResidentBytes);
On 2015/09/07 23:24:26, haraken wrote:
> Sorry for repeating the discussion again: 
No worries, better to spend time to make sure we measure the right thing.

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.

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)


> 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).
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?

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
Source/platform/PartitionAllocMemoryDumpProvider.cpp:59:
allocatorDump->AddScalar("freeable_size", "bytes",
memoryStats->totalDiscardableBytes);
On 2015/09/07 23:24:26, haraken wrote:
> 
> Keep "discardable". We consistently use "discardable" to measure the amount of
> (segmented) memory regions where no object resides.

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

https://codereview.chromium.org/1312843010/diff/20001/Source/wtf/Partitions.cpp
File Source/wtf/Partitions.cpp (right):

https://codereview.chromium.org/1312843010/diff/20001/Source/wtf/Partitions.c...
Source/wtf/Partitions.cpp:121: partitionDumpStatsGeneric(fastMallocPartition(),
"fast_malloc", isLightDump, partitionStatsDumper);
On 2015/09/07 23:24:26, haraken wrote:
> 
> I'd keep "_partition" for clarity.

I did remove it because the dump path is now:

partition_alloc/partitions/fast_malloc_partition etc I felt it was a bit
redundant having "partition" twice in the name.
WDYT? Restored in the meanwhile

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

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
File Source/platform/PartitionAllocMemoryDumpProvider.cpp (right):

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
Source/platform/PartitionAllocMemoryDumpProvider.cpp:54:
allocatorDump->AddScalar("size", "bytes", memoryStats->totalResidentBytes);
On 2015/09/08 07:56:56, Primiano Tucci wrote:
> On 2015/09/07 23:24:26, haraken wrote:
> > Sorry for repeating the discussion again: 
> No worries, better to spend time to make sure we measure the right thing.
> 
> 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.
> 
> 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)
> 
> 
> > 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).
> 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?
> 

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.

https://codereview.chromium.org/1312843010/diff/20001/Source/wtf/Partitions.cpp
File Source/wtf/Partitions.cpp (right):

https://codereview.chromium.org/1312843010/diff/20001/Source/wtf/Partitions.c...
Source/wtf/Partitions.cpp:121: partitionDumpStatsGeneric(fastMallocPartition(),
"fast_malloc", isLightDump, partitionStatsDumper);
On 2015/09/08 07:56:56, Primiano Tucci wrote:
> On 2015/09/07 23:24:26, haraken wrote:
> > 
> > I'd keep "_partition" for clarity.
> 
> I did remove it because the dump path is now:
> 
> partition_alloc/partitions/fast_malloc_partition etc I felt it was a bit
> redundant having "partition" twice in the name.
> WDYT? Restored in the meanwhile

ah, makes sense. Let's drop "_partition".

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

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
File Source/platform/PartitionAllocMemoryDumpProvider.cpp (right):

https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
Source/platform/PartitionAllocMemoryDumpProvider.cpp:54:
allocatorDump->AddScalar("size", "bytes", memoryStats->totalResidentBytes);
On 2015/09/08 08:07:33, haraken wrote:
> On 2015/09/08 07:56:56, Primiano Tucci wrote:
> > On 2015/09/07 23:24:26, haraken wrote:
> > > Sorry for repeating the discussion again: 
> > No worries, better to spend time to make sure we measure the right thing.
> > 
> > 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.
> > 
> > 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)
> > 
> > 
> > > 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).
> > 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?
> > 
> 
> 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.

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?

> 
> 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".
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
> allocators report the committed size as "size", PartitionAlloc's memory usage
> will be underestimated.
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.

[haraken, 2015-09-08 11:38:59]

On 2015/09/08 10:22:28, Primiano Tucci wrote:
>
https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
> File Source/platform/PartitionAllocMemoryDumpProvider.cpp (right):
> 
>
https://codereview.chromium.org/1312843010/diff/20001/Source/platform/Partiti...
> Source/platform/PartitionAllocMemoryDumpProvider.cpp:54:
> allocatorDump->AddScalar("size", "bytes", memoryStats->totalResidentBytes);
> On 2015/09/08 08:07:33, haraken wrote:
> > On 2015/09/08 07:56:56, Primiano Tucci wrote:
> > > On 2015/09/07 23:24:26, haraken wrote:
> > > > Sorry for repeating the discussion again: 
> > > No worries, better to spend time to make sure we measure the right thing.
> > > 
> > > 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.
> > > 
> > > 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)
> > > 
> > > 
> > > > 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).
> > > 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?
> > > 
> > 
> > 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.
> 
> 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?

Right.

In that scenario, oilpan reports 1 MB as "size". But PartitionAlloc reports (for
example) 100 KB as "size". If this is the behavior you want, I'm fine with this
CL.


> > 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".
> 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
> > allocators report the committed size as "size", PartitionAlloc's memory
usage
> > will be underestimated.
> 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?

OK, I understand your point. Ideally I think we should support the resident size
in all allocators, but until then it would make sense to ask each allocator to
report the best value they have (which is sometimes the committed size and
sometimes the resident size).

[haraken, 2015-09-08 11:40:27]

LGTM

[Primiano Tucci (use gerrit), 2015-09-09 08:44:37]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-09-09 08:44:37]

The patchset sent to the CQ was uploaded after l-g-t-m from ssid@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1312843010/#ps60001
(title: "Re-shorten partitions")

[commit-bot: I haz the power, 2015-09-09 08:44:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312843010/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312843010/60001

[commit-bot: I haz the power, 2015-09-09 09:05:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-09 09:05:34]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Primiano Tucci (use gerrit), 2015-09-09 09:08:03]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-09-09 09:08:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, ssid@chromium.org
Link to the patchset: https://codereview.chromium.org/1312843010/#ps80001
(title: "const size_t ... meh")

[commit-bot: I haz the power, 2015-09-09 09:08:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312843010/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312843010/80001

[commit-bot: I haz the power, 2015-09-09 10:12:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-09 10:12:19]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Primiano Tucci (use gerrit), 2015-09-09 10:43:28]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2015-09-09 10:43:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312843010/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312843010/80001

[commit-bot: I haz the power, 2015-09-09 12:19:22]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201972