[tracing] Add support to pre-load and merge ProcessMemoryDump(s).

[tracing] Add support to pre-load and merge ProcessMemoryDump(s).

Allows MemoryDumpProvider(s) with dump-time constraints (mostly GC
allocators) to create out-of-band MemoryAllocatorDump(s) and merge
them during the onMemoryDump callback.

So far, the architecture of MemoryDumpProvider expected to never
instantiate ProcessMemoryDump(s) directly, but just fill up the
one passed as argument to the onMemoryDump(ProcessMemoryDump*)
callback.
This, however, doesn't cover the cases of dumpers that cannot dump
at arbitrary times (i.e. during onMemoryDump callback) and
need to pre-fill the dump stats before the actual callback
comes.
A concrete case of this are dump providers for garbage-collected
allocators (specifically BlinkGC) where some of the stats can be
collected only during the GC.

BUG=466141
Committed: https://crrev.com/c7336081e09ad57bc7e5c5c5d4076db2b6b9d4ee
Cr-Commit-Position: refs/heads/master@{#330924}

[Primiano Tucci (use gerrit), 2015-05-20 22:43:19]

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

[Primiano Tucci (use gerrit), 2015-05-20 22:43:19]

Dan, can you PTAL to this? It's a pretty small patch.

[Primiano Tucci (use gerrit), 2015-05-21 07:56:41]

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

[Primiano Tucci (use gerrit), 2015-05-21 07:56:41]

+petrcermak, can you PTAL

+ssid FYI

[petrcermak, 2015-05-21 08:17:38]

LGTM with a couple of comment-style-only comments :-)

Thanks,
Petr

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:40: // and mmaps case are not relevant,
let's just prevent clients to accidentally
s/case/cases/, s/to/from/

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:41: // try doing that.
"try" doesn't belong here (it would have to be "trying"). I'd modify the end of
the whole sentence to: "[...] prevent clients from doing that accidentally."

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:44: // Moves the ownership of all the
MemoryAllocatorDump(s) contained in |other|
nit: I don't think you need "the" in front of "MemoryAllocatorDump(s)".

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:44: // Merges all the
MemoryAllocatorDump(s) contained in |other| inside this
I don't think you need "the" in front of "MemoryAllocatorDump(s)".

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:45: // ProcessMemoryDump, moving their
ownership to this instance. |other| will
"transfer ownership" seems to be used much more often than "move ownership".

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:47: // This is to allow dump providers to
create and populate out-of-band
nit: I know what you mean, but "out-of-band" is not very clear to me. Maybe
"out-of-sync"?

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:48: // instances of ProcessMemoryDump and
later move them into the
nit: The *contents* of the PMD instances are moved (not the instances
themselves).

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
File base/trace_event/process_memory_dump_unittest.cc (right):

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump_unittest.cc:36: // Free the |pmd2| to check
that the memory ownership of the two MAD(s) is
nit: s/is/has been/

P.S. I don't think you need the word "effectively" in any of the comments.

[Primiano Tucci (use gerrit), 2015-05-21 08:33:46]

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:40: // and mmaps case are not relevant,
let's just prevent clients to accidentally
On 2015/05/21 08:17:37, petrcermak wrote:
> s/case/cases/, s/to/from/

Done.

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:41: // try doing that.
On 2015/05/21 08:17:38, petrcermak wrote:
> "try" doesn't belong here (it would have to be "trying"). I'd modify the end
of
> the whole sentence to: "[...] prevent clients from doing that accidentally."

Reworded as "trying from doing it" (and I saved one LOC, yay)

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:44: // Moves the ownership of all the
MemoryAllocatorDump(s) contained in |other|
On 2015/05/21 08:17:37, petrcermak wrote:
> nit: I don't think you need "the" in front of "MemoryAllocatorDump(s)".

Done.

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:44: // Merges all the
MemoryAllocatorDump(s) contained in |other| inside this
On 2015/05/21 08:17:38, petrcermak wrote:
> I don't think you need "the" in front of "MemoryAllocatorDump(s)".

Done.

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:45: // ProcessMemoryDump, moving their
ownership to this instance. |other| will
On 2015/05/21 08:17:38, petrcermak wrote:
> "transfer ownership" seems to be used much more often than "move ownership".

Done.

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:47: // This is to allow dump providers to
create and populate out-of-band
On 2015/05/21 08:17:38, petrcermak wrote:
> nit: I know what you mean, but "out-of-band" is not very clear to me. Maybe
> "out-of-sync"?

reworded

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:48: // instances of ProcessMemoryDump and
later move them into the
On 2015/05/21 08:17:38, petrcermak wrote:
> nit: The *contents* of the PMD instances are moved (not the instances
> themselves).

reworded

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
File base/trace_event/process_memory_dump_unittest.cc (right):

https://codereview.chromium.org/1151603004/diff/20001/base/trace_event/proces...
base/trace_event/process_memory_dump_unittest.cc:36: // Free the |pmd2| to check
that the memory ownership of the two MAD(s) is
On 2015/05/21 08:17:38, petrcermak wrote:
> nit: s/is/has been/
Done

> P.S. I don't think you need the word "effectively" in any of the comments.
Was more a way to say "you can check that memory ownership has been really
transferred only if you free the objects and the contained objects are still
accessible"

[Primiano Tucci (use gerrit), 2015-05-21 08:33:50]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-05-21 08:33:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
petrcermak@chromium.org
Link to the patchset: https://codereview.chromium.org/1151603004/#ps40001
(title: "petrcermak nits")

[commit-bot: I haz the power, 2015-05-21 08:34:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1151603004/40001

[commit-bot: I haz the power, 2015-05-21 10:07:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-21 10:08:32]

Patchset 3 (id:??) landed as
https://crrev.com/c7336081e09ad57bc7e5c5c5d4076db2b6b9d4ee
Cr-Commit-Position: refs/heads/master@{#330924}