Add OOP memory logging allocator shim

Add OOP memory logging allocator shim

The allocator shim is temporary until the base on can be refactored. The rest of the machinery for out-of-process heap profiling is hooked up in this patch.

Review-Url: https://codereview.chromium.org/2949703002
Cr-Commit-Position: refs/heads/master@{#481009}
Committed: https://chromium.googlesource.com/chromium/src/+/7c54a077d28ab2547386556e4c23c6714327a49b

[brettw, 2017-06-20 19:48:17]

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

[brettw, 2017-06-20 19:48:21]

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

[commit-bot: I haz the power, 2017-06-20 19:49:06]

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-20 19:49:55]

Description was changed from

==========
Add allocator shim

BUG=
==========

to

==========
Add OOP memory logging allocator shim

The allocator shim is temporary until the base on can be refactored. The rest of
the machinery for out-of-process heap profiling is hooked up in this patch.
==========

[brettw, 2017-06-20 19:49:55]

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

[awong, 2017-06-20 21:01:47]

LGTM w/ nits

All comments are effectively nits or documentation changes.

https://codereview.chromium.org/2949703002/diff/20001/chrome/common/profiling...
File chrome/common/profiling/memlog_allocator_shim.cc (right):

https://codereview.chromium.org/2949703002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_allocator_shim.cc:21: const int kMaxStackEntries
= 48;
Can we add some comments explaining why we chose these other magic numbers?

https://codereview.chromium.org/2949703002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_allocator_shim.cc:56: // "address" is the address
in question, which is used to select which send
I thought chrome used |address| to designate parameter vairables in
documentation? Am I misremembering?

https://codereview.chromium.org/2949703002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_allocator_shim.cc:179: AllocPacket* alloc_packet
= reinterpret_cast<AllocPacket*>(message);
:-/ 

Can you add a TODO + file a bug assigned to me to check the C++ validity of
this?

I think technically you're expected to placement new the AllocPacket into the
buffer and that the reinterpret_cast<> as used is undefined behavior.

https://codereview.chromium.org/2949703002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_allocator_shim.cc:184: #if 0
???

https://codereview.chromium.org/2949703002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_allocator_shim.cc:226: free_packet.address =
(uint64_t)address;
reinterpret_cast<>?

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

https://codereview.chromium.org/2949703002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_sender.h:13: 
newline inconsistency between here and the above namespace block.

[awong, 2017-06-20 21:03:11]

actually...one major question...

https://codereview.chromium.org/2949703002/diff/20001/chrome/common/profiling...
File chrome/common/profiling/memlog_allocator_shim.cc (right):

https://codereview.chromium.org/2949703002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_allocator_shim.cc:61:
g_send_buffers[bin_to_use].Send(data, size);
How does this handle threading?

[brettw, 2017-06-20 21:15:04]

Review comments

[brettw, 2017-06-20 21:15:04]

The CQ bit was checked by brettw@chromium.org

[brettw, 2017-06-20 21:15:05]

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/2949703002/#ps40001
(title: "Review comments")

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

CQ is trying da patch.

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

[brettw, 2017-06-20 21:15:16]

The CQ bit was unchecked by brettw@chromium.org

[brettw, 2017-06-20 21:22:11]

More comments

[brettw, 2017-06-20 21:22:12]

The CQ bit was checked by brettw@chromium.org

[brettw, 2017-06-20 21:22:12]

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/2949703002/#ps60001
(title: "More comments")

[brettw, 2017-06-20 21:22:28]

https://codereview.chromium.org/2949703002/diff/20001/chrome/common/profiling...
File chrome/common/profiling/memlog_allocator_shim.cc (right):

https://codereview.chromium.org/2949703002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_allocator_shim.cc:56: // "address" is the address
in question, which is used to select which send
On 2017/06/20 21:01:47, awong wrote:
> I thought chrome used |address| to designate parameter vairables in
> documentation? Am I misremembering?

Lots of people do that, I do it sometimes, but there's no consistency or policy.

https://codereview.chromium.org/2949703002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_allocator_shim.cc:184: #if 0
On 2017/06/20 21:01:47, awong wrote:
> ???

Oh, that was me testing performance without stack captures. Removed.

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

https://codereview.chromium.org/2949703002/diff/20001/chrome/common/profiling...
chrome/common/profiling/memlog_sender.h:13: 
On 2017/06/20 21:01:47, awong wrote:
> newline inconsistency between here and the above namespace block.

I changed it, but I did it this way on purpose. It's pretty common to skip the
newlines around lists of simple forward declarations in namespaces.

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

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 23:10:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497993732116000,
"parent_rev": "cbcda7af9eb6627071532029b314185e34fa76e3", "commit_rev":
"7c54a077d28ab2547386556e4c23c6714327a49b"}

[commit-bot: I haz the power, 2017-06-20 23:11:04]

Description was changed from

==========
Add OOP memory logging allocator shim

The allocator shim is temporary until the base on can be refactored. The rest of
the machinery for out-of-process heap profiling is hooked up in this patch.
==========

to

==========
Add OOP memory logging allocator shim

The allocator shim is temporary until the base on can be refactored. The rest of
the machinery for out-of-process heap profiling is hooked up in this patch.

Review-Url: https://codereview.chromium.org/2949703002
Cr-Commit-Position: refs/heads/master@{#481009}
Committed:
https://chromium.googlesource.com/chromium/src/+/7c54a077d28ab2547386556e4c23...
==========

[commit-bot: I haz the power, 2017-06-20 23:11:06]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/7c54a077d28ab2547386556e4c23...

[Primiano Tucci (use gerrit), 2017-06-21 14:11:29]

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

[Primiano Tucci (use gerrit), 2017-06-21 14:11:29]

https://codereview.chromium.org/2949703002/diff/60001/chrome/common/profiling...
File chrome/common/profiling/memlog_allocator_shim.cc (right):

https://codereview.chromium.org/2949703002/diff/60001/chrome/common/profiling...
chrome/common/profiling/memlog_allocator_shim.cc:188: char
message[max_message_size];
Doesn't this have to be declared  ALIGNAS(sizeof(uint64_t)) ? 
If this is not aligned all the assignment operations below in lines 209-217 will
very silently be way slower on x86/64 and eventualy SIGBUS the day that you use
this on arm.