Add out-of-process memory logging stream parsing.

Add out-of-process memory logging stream parsing.

This is the receiver end, it is not yet hooked up and there is no sending end yet.

The MemlogConnectionManager get the notifications of new pipe connections and hooks up the parser to it. The events go to a per-process MemlogAllocationTracker object to track all live memory allocations. This is currently just a simple map.

Stacks are de-duplicated across processes. This saves a little memory. This requires that individual stacks are refcounted and atomized. The StackStorage object tracks all globals stacks for this purpose.

Review-Url: https://codereview.chromium.org/2943733002
Cr-Commit-Position: refs/heads/master@{#480715}
Committed: https://chromium.googlesource.com/chromium/src/+/bd8214bfccde827f7a0e4c3683af8e4937b3d8d7

[brettw, 2017-06-16 17:50:56]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 17:51:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[brettw, 2017-06-16 17:56:07]

Description was changed from

==========
Add out-of-process memory logging stream parsing.
==========

to

==========
Add out-of-process memory logging stream parsing.

This is the receiver end, it is not yet hooked up and there is no sending end
yet.

The MemlogConnectionManager get the notifications of new pipe connections and
hooks up the parser to it. The events go to a per-process
MemlogAllocationTracker object to track all live memory allocations. This is
currently just a simple map.

Stacks are de-duplicated across processes. This saves a little memory. This
requires that individual stacks are refcounted and atomized. The StackStorage
object tracks all globals stacks for this purpose.
==========

[brettw, 2017-06-16 17:56:07]

brettw@chromium.org changed reviewers:
+ borisv@chromium.org

[brettw, 2017-06-16 17:56:52]

+CC some people.

[commit-bot: I haz the power, 2017-06-16 19:18:58]

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

[commit-bot: I haz the power, 2017-06-16 19:18:59]

Dry run: This issue passed the CQ dry run.

[Boris Vidolov, 2017-06-16 21:16:46]

https://codereview.chromium.org/2943733002/diff/20001/chrome/common/profiling...
File chrome/common/profiling/memlog_stream.h (right):

https://codereview.chromium.org/2943733002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_stream.h:25: char proctype[16];
What will this be?

https://codereview.chromium.org/2943733002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_stream.h:43: uint64_t address;
No stack trace here?

[brettw, 2017-06-16 23:57:53]

Remove version

[brettw, 2017-06-16 23:58:14]

https://codereview.chromium.org/2943733002/diff/20001/chrome/common/profiling...
File chrome/common/profiling/memlog_stream.h (right):

https://codereview.chromium.org/2943733002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_stream.h:25: char proctype[16];
On 2017/06/16 21:16:46, Boris Vidolov wrote:
> What will this be?

I meant to delete that but did it in the next path instead. I deleted it now.

https://codereview.chromium.org/2943733002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_stream.h:43: uint64_t address;
On 2017/06/16 21:16:45, Boris Vidolov wrote:
> No stack trace here?

No, I didn't hook up free stack traces. It's easy to add if we need it.

[brettw, 2017-06-16 23:58:21]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 23:58:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Boris Vidolov, 2017-06-17 00:53:08]

https://codereview.chromium.org/2943733002/diff/20001/chrome/profiling/alloca...
File chrome/profiling/allocation_tracker.cc (right):

https://codereview.chromium.org/2943733002/diff/20001/chrome/profiling/alloca...
chrome/profiling/allocation_tracker.cc:16:
AllocationTracker::AllocationTracker(CompleteCallback complete_cb)
Should this be CompleteCallback&&

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
File chrome/profiling/memlog_stream_parser.cc (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.cc:32: void
MemlogStreamParser::OnStreamData(std::unique_ptr<char[]> data, size_t sz) {
Should this be std::unique_ptr<char[]>&& data ?

[commit-bot: I haz the power, 2017-06-17 01:44:44]

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

[commit-bot: I haz the power, 2017-06-17 01:44:45]

Dry run: This issue passed the CQ dry run.

[brettw, 2017-06-17 02:41:51]

https://codereview.chromium.org/2943733002/diff/20001/chrome/profiling/alloca...
File chrome/profiling/allocation_tracker.cc (right):

https://codereview.chromium.org/2943733002/diff/20001/chrome/profiling/alloca...
chrome/profiling/allocation_tracker.cc:16:
AllocationTracker::AllocationTracker(CompleteCallback complete_cb)
On 2017/06/17 00:53:07, Boris Vidolov wrote:
> Should this be CompleteCallback&&

We usually don't pass these by reference or move. Internally base::Closure is
refcounted so it should be fine either way.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
File chrome/profiling/memlog_stream_parser.cc (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.cc:32: void
MemlogStreamParser::OnStreamData(std::unique_ptr<char[]> data, size_t sz) {
On 2017/06/17 00:53:07, Boris Vidolov wrote:
> Should this be std::unique_ptr<char[]>&& data ?

I think these are usually passed by value also (git grep "(std::unique_ptr").
I've had people correct me in the opposite direction for this kind of thing
also, since the theory is that then the caller can control whether to give you a
copy or a move.

In this case it doesn't matter since a unique_ptr is just a wrapper around a
pointer the same thing should happen in both cases.

[Boris Vidolov, 2017-06-17 03:13:15]

Thanks, Brett. I am done with the review. A few comments here and there about
the performance and simplicity of the code.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
File chrome/profiling/memlog_stream_parser.cc (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.cc:106: if (!PeekBytes(count, dest))
This doesn't sound to be very efficient:
ReadBytes calls PeekBytes, which calls AreBytesAvailable. Then ReadBytes calls
ConsumeBytes. All of PeekBytes, AreBytesAvailable and ConsumeBytes have similar
implementation walking the same blocks, tripling the work.
Maybe create one method that walks up to  'count' bytes and passes back an
iterator to the  last block? Then all three methods can call this one and decide
what to do based on the result.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.cc:123: blocks_.erase(blocks_.begin());
Can we erase a range instead of single block? Erasing one block at a time will
have quadratic complexity, as it always moves all of the following elements in
the array.

Overall, how many blocks do we expect to have? I am concerned that this code
would typically consume one block or less at a time, causing the vector to move
elements heavily if many blocks are available.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.cc:160: Stack
stack(std::move(stack_addrs));
Do we need to go through an intermediary 'Stack' variable? Can't we just pass
stack_addrs below?

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack.cc
File chrome/profiling/stack.cc (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack....
chrome/profiling/stack.cc:33: if (addrs_.size() != other.addrs_.size())
Why not compare hash_ first and do the expensive operations below only if hash_
is the same? Also, what do you think about optimizing for the same pointer if
(this == &other) return true;

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack....
chrome/profiling/stack.cc:36: if (addrs_[i] != other.addrs_[i])
I know that operator != is overridden, but I am still wondering if we can do
memcmp here. At least in theory, memcmp can use SSE instructions speeding up the
comparison several times.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack_...
File chrome/profiling/stack_storage.cc (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack_...
chrome/profiling/stack_storage.cc:26: stacks_.erase(key);
I am wondering if we really need to release stacks - what is the use case? I
understand that we may need to delete them all in the constructor, but probably
not in a public API. In short why bother with this method and the one below, if
it is not needed.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack_...
File chrome/profiling/stack_storage.h (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack_...
chrome/profiling/stack_storage.h:27: // Adds the give stack to the storage and
returns a key to it. If a matching
nit: "given"

[awong, 2017-06-19 20:00:12]

ajwong@chromium.org changed reviewers:
+ ajwong@chromium.org

[awong, 2017-06-19 20:00:13]

still reviewing, but sending while I run to meeting.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/address.h
File chrome/profiling/address.h (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/addres...
chrome/profiling/address.h:57: return static_cast<size_t>(a.value);
?? Shouldn't this return base::hash(a.value)?

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/alloca...
File chrome/profiling/allocation_tracker.cc (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/alloca...
chrome/profiling/allocation_tracker.cc:31: Stack&& stack) {
r-value reference? Why not by copy?

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/alloca...
File chrome/profiling/allocation_tracker.h (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/alloca...
chrome/profiling/allocation_tracker.h:17: // Tracks live allocations in one
process.
Can you add a TODO() comment staying this is an analogue to the memory-infra
allocation register and the two need to be merged/deduped?

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
File chrome/profiling/memlog_receiver.h (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver.h:21: MemlogReceiver() {}
Why do we need to declare the default constructor?

Shoudl this have a disallow copy and assign?

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
File chrome/profiling/memlog_stream_parser.h (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.h:21: ~MemlogStreamParser();
virtual?

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.h:48: MemlogReceiver* receiver_;  //
Unowned.
nit: Unowned -> Not owned.

[awong, 2017-06-19 21:51:28]

more comments.

Main consistent concern is we're incorreclty using &&

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
File chrome/profiling/memlog_stream_parser.cc (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.cc:18: using AddressVector =
base::StackVector<Address, 128>;
Random C++11 question... does this need to be in an anonymous namespace?  What
happens if another translation unit declares a profiling::AddressVector?

(I don't know the answer to this.)

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.cc:32: void
MemlogStreamParser::OnStreamData(std::unique_ptr<char[]> data, size_t sz) {
On 2017/06/17 02:41:51, brettw wrote:
> On 2017/06/17 00:53:07, Boris Vidolov wrote:
> > Should this be std::unique_ptr<char[]>&& data ?
> 
> I think these are usually passed by value also (git grep "(std::unique_ptr").
> I've had people correct me in the opposite direction for this kind of thing
> also, since the theory is that then the caller can control whether to give you
a
> copy or a move.
> 
> In this case it doesn't matter since a unique_ptr is just a wrapper around a
> pointer the same thing should happen in both cases.

Definitely should not be &&.  If the API is denoting ownership transfer, pass by
value is the right semantic for the RAII move-only types.

Any use of && is suspect to me if it isn't specifically an overload against
another function.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.cc:94: memcpy(&dest_char[used],
&blocks_[current_block].data[current_block_offset],
std::copy?

https://stackoverflow.com/questions/4707012/is-it-better-to-use-stdmemcpy-or-...

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.cc:102: return used == count;
Can this ever be false if we didn't return in line 82?

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
File chrome/profiling/memlog_stream_parser.h (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.h:40: bool PeekBytes(size_t count, void*
dest) const;
Why are these taking void* instead of char*?

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack.h
File chrome/profiling/stack.h (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack....
chrome/profiling/stack.h:25: class Stack {
nit: rename Backtrace.

Stack looks like a data structure with push/pop to me.

Also explain why this is different from base::debug::StackTrace

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack....
chrome/profiling/stack.h:27: explicit Stack(std::vector<Address>&& a);
If this is move-only, don't you need a DisallowCopyAndASsign?

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack....
chrome/profiling/stack.h:35: size_t hash() const { return hash_; }
Is this actually a "hash()" as in an unstable value for use in a hashtable that
you are memoizing?  Or is it really a fingerprint() that we're going to
(conceptually) expect to be identifying of this particular back trace?

If it's the latter, this should be called fingerprint(), preferable
fingerprint_algorithm_name().

FWIW, I think fingerprint_sha1 or finterprint_murmur would be more useful than
generic hash as an API.

[brettw, 2017-06-19 23:29:46]

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/address.h
File chrome/profiling/address.h (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/addres...
chrome/profiling/address.h:57: return static_cast<size_t>(a.value);
On 2017/06/19 20:00:13, awong wrote:
> ?? Shouldn't this return base::hash(a.value)?

Thanks, wrote this long ago as a placeholder and forgot about it.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/alloca...
File chrome/profiling/allocation_tracker.cc (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/alloca...
chrome/profiling/allocation_tracker.cc:31: Stack&& stack) {
On 2017/06/19 20:00:13, awong wrote:
> r-value reference? Why not by copy?

I assume you mean by value. I didn't want to have the caller accidentally copy
stacks. Stacks in this component are not something that should be copied,
they're passed from the parser to the tracker, and then inserted into the
StackStorage object. Normally one would do by-value to allow the caller to
decide whether it wants to move or give you a copy (I usually do this as per the
comment where Boris asked me to do the opposite thing).

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
File chrome/profiling/memlog_receiver.h (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver.h:21: MemlogReceiver() {}
On 2017/06/19 20:00:13, awong wrote:
> Why do we need to declare the default constructor?

Because of mistake.

> Shoudl this have a disallow copy and assign?

This isn't necessary since you can't copy classes with pure virtual methods.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
File chrome/profiling/memlog_stream_parser.cc (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.cc:18: using AddressVector =
base::StackVector<Address, 128>;
On 2017/06/19 21:51:27, awong wrote:
> Random C++11 question... does this need to be in an anonymous namespace?  What
> happens if another translation unit declares a profiling::AddressVector?
> 
> (I don't know the answer to this.)

I think it will be OK, but the anon namespace seems like a good idea anyway.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.cc:94: memcpy(&dest_char[used],
&blocks_[current_block].data[current_block_offset],
I didn't want to do an element-by-element copy here. std::copy may do this. I
looked at the GCC and MS implementations. I couldn't understand the GCC one and
I couldn't find the MS one. I prefer this simpler call which is definitely what
I want.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.cc:102: return used == count;
It can be now since I removed the AreBytesAvailable call.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.cc:106: if (!PeekBytes(count, dest))
On 2017/06/17 03:13:14, Boris Vidolov wrote:
> This doesn't sound to be very efficient:
> ReadBytes calls PeekBytes, which calls AreBytesAvailable. Then ReadBytes calls
> ConsumeBytes. All of PeekBytes, AreBytesAvailable and ConsumeBytes have
similar
> implementation walking the same blocks, tripling the work.
> Maybe create one method that walks up to  'count' bytes and passes back an
> iterator to the  last block? Then all three methods can call this one and
decide
> what to do based on the result.

I don't think returning an iterator would be very helpful since the log is in
going through the blocks, not detecting the end.

I removed the Peek call here and just said in the header that it may touch the
buffer on failure. In general, messages will almost never cross block
boundaries. Read could be optimized more by duplicating some code but that
didn't seem worth it to me.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.cc:123: blocks_.erase(blocks_.begin());
On 2017/06/17 03:13:15, Boris Vidolov wrote:
> Can we erase a range instead of single block? Erasing one block at a time will
> have quadratic complexity, as it always moves all of the following elements in
> the array.
> 
> Overall, how many blocks do we expect to have? I am concerned that this code
> would typically consume one block or less at a time, causing the vector to
move
> elements heavily if many blocks are available.

I would hope the common case is 0-2 blocks (they're 64K each so have a lot of
messages in each one). I believe this code should generally be able to keep up
with the stream sent by the instrumented process. I did consider a list or
something here but it seemed moving pointers was much more efficient than
mallocing for the expected workload.

A queue would probably be better overall so I used that.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.cc:160: Stack
stack(std::move(stack_addrs));
On 2017/06/17 03:13:14, Boris Vidolov wrote:
> Do we need to go through an intermediary 'Stack' variable? Can't we just pass
> stack_addrs below?

The stack object exists to store the hash so we don't have to re-hash for
different operations. Since this exists, it seems nice to use it consistently.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
File chrome/profiling/memlog_stream_parser.h (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.h:21: ~MemlogStreamParser();
On 2017/06/19 20:00:13, awong wrote:
> virtual?

I added override.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.h:40: bool PeekBytes(size_t count, void*
dest) const;
On 2017/06/19 21:51:27, awong wrote:
> Why are these taking void* instead of char*?

It's normal for file read functions to take void* so the caller can control
exactly what type they want. One might use uint8_t, char, unsigned char. Unix
read, fread, windows ReadFile all do this.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_stream_parser.h:48: MemlogReceiver* receiver_;  //
Unowned.
On 2017/06/19 20:00:13, awong wrote:
> nit: Unowned -> Not owned.

Done.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack.cc
File chrome/profiling/stack.cc (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack....
chrome/profiling/stack.cc:33: if (addrs_.size() != other.addrs_.size())
On 2017/06/17 03:13:15, Boris Vidolov wrote:
> Why not compare hash_ first and do the expensive operations below only if
hash_
> is the same? Also, what do you think about optimizing for the same pointer if
> (this == &other) return true;

The answer to both of these is that the only time this is called is for hash
collision checking. So the hash table will already have verified that the hashes
match. If we start using it in more places, this might be more useful, but I
couldn't see a common need to compare stacks for equality outside of the
containers.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack....
chrome/profiling/stack.cc:36: if (addrs_[i] != other.addrs_[i])
On 2017/06/17 03:13:15, Boris Vidolov wrote:
> I know that operator != is overridden, but I am still wondering if we can do
> memcmp here. At least in theory, memcmp can use SSE instructions speeding up
the
> comparison several times.

I switched to memcmp.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack.h
File chrome/profiling/stack.h (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack....
chrome/profiling/stack.h:25: class Stack {
On 2017/06/19 21:51:27, awong wrote:
> nit: rename Backtrace.
> 
> Stack looks like a data structure with push/pop to me.
> 
> Also explain why this is different from base::debug::StackTrace

Done.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack....
chrome/profiling/stack.h:27: explicit Stack(std::vector<Address>&& a);
On 2017/06/19 21:51:27, awong wrote:
> If this is move-only, don't you need a DisallowCopyAndASsign?

It's not move-only, but I guess I can make it like that.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack....
chrome/profiling/stack.h:35: size_t hash() const { return hash_; }
Changed to fingerprint.

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack_...
File chrome/profiling/stack_storage.cc (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/stack_...
chrome/profiling/stack_storage.cc:26: stacks_.erase(key);
I think freeing the stacks makes a big difference in memory usage, but since
this re-land isn't runnable right now I can't test. Otherwise we will keep
stacks around for allocations that are released and freed.

Also, renderer stacks won't match because of oilpan stuff (is my understanding),
so we definitely need to free all stacks associated with a renderer when it
exits.

[brettw, 2017-06-19 23:32:57]

Review comments

[brettw, 2017-06-19 23:32:58]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 23:33:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[brettw, 2017-06-19 23:33:46]

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
File chrome/profiling/memlog_receiver.h (right):

https://codereview.chromium.org/2943733002/diff/40001/chrome/profiling/memlog...
chrome/profiling/memlog_receiver.h:21: MemlogReceiver() {}
On 2017/06/19 23:29:45, brettw wrote:
> On 2017/06/19 20:00:13, awong wrote:
> > Why do we need to declare the default constructor?
> 
> Because of mistake.
> 
> > Shoudl this have a disallow copy and assign?
> 
> This isn't necessary since you can't copy classes with pure virtual methods.

Sorry, forgot to update after our conversation. I talked to Peter. We agreed
doing this is rare in our codebase. Disallowing copy and assign here prevents
derived classes from implementing those functions if they want (the general
discouragement against copying and assigning is now "avoid" (it used to be
"never do")). Ideally one might do protected ones but that's not what the macro
does.

[awong, 2017-06-19 23:42:05]

LGTM

https://codereview.chromium.org/2943733002/diff/60001/chrome/profiling/backtr...
File chrome/profiling/backtrace.cc (right):

https://codereview.chromium.org/2943733002/diff/60001/chrome/profiling/backtr...
chrome/profiling/backtrace.cc:19: size_t ComputeHash(const std::vector<Address>&
addrs) {
Can you add a TODO here assigned to me to fix this to use a fingerprint capable
hash?

[commit-bot: I haz the power, 2017-06-19 23:47:41]

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

[commit-bot: I haz the power, 2017-06-19 23:47:42]

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

[brettw, 2017-06-20 00:53:55]

Add TODO

[brettw, 2017-06-20 00:53:56]

The CQ bit was checked by brettw@chromium.org

[brettw, 2017-06-20 00:53:56]

The patchset sent to the CQ was uploaded after l-g-t-m from ajwong@chromium.org
Link to the patchset: https://codereview.chromium.org/2943733002/#ps80001
(title: "Add TODO")

[commit-bot: I haz the power, 2017-06-20 00:54:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 03:47:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497920036487120,
"parent_rev": "795ade2be7e5f0cd1daaa54b3247ae77ef314895", "commit_rev":
"bd8214bfccde827f7a0e4c3683af8e4937b3d8d7"}

[commit-bot: I haz the power, 2017-06-20 03:47:18]

Description was changed from

==========
Add out-of-process memory logging stream parsing.

This is the receiver end, it is not yet hooked up and there is no sending end
yet.

The MemlogConnectionManager get the notifications of new pipe connections and
hooks up the parser to it. The events go to a per-process
MemlogAllocationTracker object to track all live memory allocations. This is
currently just a simple map.

Stacks are de-duplicated across processes. This saves a little memory. This
requires that individual stacks are refcounted and atomized. The StackStorage
object tracks all globals stacks for this purpose.
==========

to

==========
Add out-of-process memory logging stream parsing.

This is the receiver end, it is not yet hooked up and there is no sending end
yet.

The MemlogConnectionManager get the notifications of new pipe connections and
hooks up the parser to it. The events go to a per-process
MemlogAllocationTracker object to track all live memory allocations. This is
currently just a simple map.

Stacks are de-duplicated across processes. This saves a little memory. This
requires that individual stacks are refcounted and atomized. The StackStorage
object tracks all globals stacks for this purpose.

Review-Url: https://codereview.chromium.org/2943733002
Cr-Commit-Position: refs/heads/master@{#480715}
Committed:
https://chromium.googlesource.com/chromium/src/+/bd8214bfccde827f7a0e4c3683af...
==========

[commit-bot: I haz the power, 2017-06-20 03:47:20]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/bd8214bfccde827f7a0e4c3683af...