Updated MallocHooks to collect stack traces when memory is allocated.

Updated MallocHooks to collect stack traces when memory is allocated.

BUG=
R=johnmccutchan@google.com, zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/47f0da9cd79d41c3f23b03a0f6cc771566e62c78

[bkonyi, 2017-02-07 23:10:26]

bkonyi@google.com changed reviewers:
+ johnmccutchan@google.com, zra@google.com

[bkonyi, 2017-02-07 23:10:26]

Just looking for some initial comments/suggestions based on what I have so far
for the MallocHooks stack trace collection.

[Cutch, 2017-02-08 00:28:54]

First round of comments

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.cc
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:127: sample_ = Profiler::NativeSampleAllocation();
Naming nit:

This should be SampleNativeAllocation

Sample is the verb.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:171: inline void Insert(const Key& key, const Value&
value) {
These are declared inline in this source file. No need to use the inline keyword
(here or below)

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks_tes...
File runtime/vm/malloc_hooks_test.cc (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks_tes...
runtime/vm/malloc_hooks_test.cc:9: #include <execinfo.h>
This is a cross platform test file. backtrace/execinfo.h is not portable.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks_tes...
runtime/vm/malloc_hooks_test.cc:109: while (sample != NULL) {
This test might not be necessary. We have tests which verify the the stack
walkers work.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:427: : ProfilerStackWalker(thread->isolate() != NULL
Wrap comparison in parens:

(thread->isolate() != NULL) ? ...

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:868: static bool
GetAndValidateThreadStackBounds(OSThread* os_thread,
please find a way to merge this function with the one above.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:929: sample->Init(isolate,
OS::GetCurrentMonotonicMicros(), tid);
As discussed: Sample needs to store the Dart_Port not the Isolate.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:949: sample->Init(NULL, OS::GetCurrentMonotonicMicros(),
tid);
You should be passing in ILLEGAL_PORT_ID and not NULL for the Dart_Port in the
sample.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/profiler.h#newco...
runtime/vm/profiler.h:61: static Sample* NativeSampleAllocation();
SampleNativeAllocation

[bkonyi, 2017-02-08 01:37:05]

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.cc
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:127: sample_ = Profiler::NativeSampleAllocation();
On 2017/02/08 00:28:53, Cutch wrote:
> Naming nit:
> 
> This should be SampleNativeAllocation
> 
> Sample is the verb.

Done.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:171: inline void Insert(const Key& key, const Value&
value) {
On 2017/02/08 00:28:53, Cutch wrote:
> These are declared inline in this source file. No need to use the inline
keyword
> (here or below)

Acknowledged. Will be fixed in patch 3.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks_tes...
File runtime/vm/malloc_hooks_test.cc (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks_tes...
runtime/vm/malloc_hooks_test.cc:9: #include <execinfo.h>
On 2017/02/08 00:28:54, Cutch wrote:
> This is a cross platform test file. backtrace/execinfo.h is not portable.

This was more here for me to check that my implementation was correct. I'll
remove it and the below test before actually landing.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks_tes...
runtime/vm/malloc_hooks_test.cc:109: while (sample != NULL) {
On 2017/02/08 00:28:53, Cutch wrote:
> This test might not be necessary. We have tests which verify the the stack
> walkers work.

Makes sense. I'll remove it before actually landing.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:427: : ProfilerStackWalker(thread->isolate() != NULL
On 2017/02/08 00:28:54, Cutch wrote:
> Wrap comparison in parens:
> 
> (thread->isolate() != NULL) ? ...

Done.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:868: static bool
GetAndValidateThreadStackBounds(OSThread* os_thread,
On 2017/02/08 00:28:54, Cutch wrote:
> please find a way to merge this function with the one above.

Acknowledged. Will do this for patch 3.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:949: sample->Init(NULL, OS::GetCurrentMonotonicMicros(),
tid);
On 2017/02/08 00:28:54, Cutch wrote:
> You should be passing in ILLEGAL_PORT_ID and not NULL for the Dart_Port in the
> sample.

Done.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/profiler.h#newco...
runtime/vm/profiler.h:61: static Sample* NativeSampleAllocation();
On 2017/02/08 00:28:54, Cutch wrote:
> SampleNativeAllocation

Done.

[zra, 2017-02-08 17:42:55]

Initial comments from first PS. Might be stale. Starting to review most recent
PS, now.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/dart.cc#newcode235
runtime/vm/dart.cc:235: MallocHooks::InitOnce();
What's the reason for moving this around? If it's temporary because of a bug,
you probably want to add a comment with a TODO. If it's permanent, then a
comment explaining why it can't go earlier is probably appropriate.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.cc
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:251: MallocHookScope mhs;
If this is a fix for an existing bug, we should pull it out into its own CL and
land it first.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.h
File runtime/vm/malloc_hooks.h (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.h#n...
runtime/vm/malloc_hooks.h:8: #include "vm/globals.h"
#include "vm/allocation.h" for AllStatic

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.h#n...
runtime/vm/malloc_hooks.h:19: static bool stack_trace_collection_enabled();
Maybe ProfilingEnabled()

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks_tes...
File runtime/vm/malloc_hooks_test.cc (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks_tes...
runtime/vm/malloc_hooks_test.cc:9: #include <execinfo.h>
Not sure how portable this is. Should this be gated on TARGET_OS_LINUX?

[zra, 2017-02-08 18:14:14]

This looks headed in the right direction.

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks.cc
File runtime/vm/malloc_hooks.cc (left):

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:30: ASSERT(in_malloc_hook_flag_ !=
kUnsetThreadLocalKey);
Maybe change ASSERT() -> if() instead of removing.

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks.cc
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:71: static Mutex* malloc_hook_mutex_;
It can be nice have the Mutex first with a comment about what things below it
that it protects.

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:127: sample_ = Profiler::SampleNativeAllocation();
Let's add a TODO here for the following issue. This sample is going to include
frames between the actual call to malloc and this one. Those frames aren't going
to be useful, so we need to figure out a way to truncate the sample.

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:169: ~AddressMap() { Clear(); }
I think we need to mark the destructor of BaseDirectChainedHashMap as virtual.

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:171: inline void Insert(const Key& key, const Value&
value) {
Is inline needed here and below?

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks.h
File runtime/vm/malloc_hooks.h (right):

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.h:1: // Copyright (c) 2017, the Dart project authors. 
Please see the AUTHORS file
Adding a reminder here to update the _unsupported.cc file.

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks...
File runtime/vm/malloc_hooks_test.cc (right):

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks_test.cc:9: // #include <execinfo.h> TODO(bkonyi) remove
before final patch.
If this is always going to be on Linux, I'd be fine with having the test only
there.

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/os_thread.h
File runtime/vm/os_thread.h (right):

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/os_thread.h#...
runtime/vm/os_thread.h:117: static bool DoesCurrentThreadExist() { return
(GetCurrentTLS() != NULL); }
Maybe TryCurrent() (or CurrentIfExists()), which is like Current except that it
returns NULL if the OSThread doesn't exist, and then Current() calls
TryCurrent().

[bkonyi, 2017-02-09 21:22:47]

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/dart.cc#newcode235
runtime/vm/dart.cc:235: MallocHooks::InitOnce();
On 2017/02/08 17:42:55, zra wrote:
> What's the reason for moving this around? If it's temporary because of a bug,
> you probably want to add a comment with a TODO. If it's permanent, then a
> comment explaining why it can't go earlier is probably appropriate.

The stack trace collection relies on StubCode being initialized, so
MallocHooks::InitOnce() needed to be moved until after that was done.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.cc
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:171: inline void Insert(const Key& key, const Value&
value) {
On 2017/02/08 00:28:53, Cutch wrote:
> These are declared inline in this source file. No need to use the inline
keyword
> (here or below)

Done.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:251: MallocHookScope mhs;
On 2017/02/08 17:42:55, zra wrote:
> If this is a fix for an existing bug, we should pull it out into its own CL
and
> land it first.

This isn't a fix for an existing bug since ResetStats() didn't free memory
before this change.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.h
File runtime/vm/malloc_hooks.h (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.h#n...
runtime/vm/malloc_hooks.h:8: #include "vm/globals.h"
On 2017/02/08 17:42:55, zra wrote:
> #include "vm/allocation.h" for AllStatic

Done.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks.h#n...
runtime/vm/malloc_hooks.h:19: static bool stack_trace_collection_enabled();
On 2017/02/08 17:42:55, zra wrote:
> Maybe ProfilingEnabled()

Done.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks_tes...
File runtime/vm/malloc_hooks_test.cc (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/malloc_hooks_tes...
runtime/vm/malloc_hooks_test.cc:9: #include <execinfo.h>
On 2017/02/08 17:42:55, zra wrote:
> Not sure how portable this is. Should this be gated on TARGET_OS_LINUX?

Yes, it probably should. However, DART_USE_TCMALLOC is only defined when
building for Linux since nothing else supports tcmalloc at this point, so this
should be fine for now at least.

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/2680213002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:868: static bool
GetAndValidateThreadStackBounds(OSThread* os_thread,
On 2017/02/08 00:28:54, Cutch wrote:
> please find a way to merge this function with the one above.

Done.

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks.cc
File runtime/vm/malloc_hooks.cc (left):

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:30: ASSERT(in_malloc_hook_flag_ !=
kUnsetThreadLocalKey);
On 2017/02/08 18:14:14, zra wrote:
> Maybe change ASSERT() -> if() instead of removing.

I can't remember exactly why I removed this... I'm going to add it back for now.

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks.cc
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:71: static Mutex* malloc_hook_mutex_;
On 2017/02/08 18:14:14, zra wrote:
> It can be nice have the Mutex first with a comment about what things below it
> that it protects.

Done.

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:127: sample_ = Profiler::SampleNativeAllocation();
On 2017/02/08 18:14:14, zra wrote:
> Let's add a TODO here for the following issue. This sample is going to include
> frames between the actual call to malloc and this one. Those frames aren't
going
> to be useful, so we need to figure out a way to truncate the sample.

Done.

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:169: ~AddressMap() { Clear(); }
On 2017/02/08 18:14:14, zra wrote:
> I think we need to mark the destructor of BaseDirectChainedHashMap as virtual.

Done.

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:171: inline void Insert(const Key& key, const Value&
value) {
On 2017/02/08 18:14:14, zra wrote:
> Is inline needed here and below?

No, I've removed them. Forgot to upload them in this patch.

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks...
File runtime/vm/malloc_hooks_test.cc (right):

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks_test.cc:9: // #include <execinfo.h> TODO(bkonyi) remove
before final patch.
On 2017/02/08 18:14:14, zra wrote:
> If this is always going to be on Linux, I'd be fine with having the test only
> there.

This should basically always be on Linux since the only supported configurations
for tcmalloc at the moment require Linux in the first place.

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/os_thread.h
File runtime/vm/os_thread.h (right):

https://codereview.chromium.org/2680213002/diff/20001/runtime/vm/os_thread.h#...
runtime/vm/os_thread.h:117: static bool DoesCurrentThreadExist() { return
(GetCurrentTLS() != NULL); }
On 2017/02/08 18:14:14, zra wrote:
> Maybe TryCurrent() (or CurrentIfExists()), which is like Current except that
it
> returns NULL if the OSThread doesn't exist, and then Current() calls
> TryCurrent().

Done.

[bkonyi, 2017-02-14 23:26:33]

I've gone ahead and added tests for what I have so far. Everything is functional
in its current state and just needs to be hooked into the Observatory in another
CL.

[zra, 2017-02-15 05:50:46]

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/dart.cc#new...
runtime/vm/dart.cc:276: MallocHooks::InitOnce();
Maybe add a TODO to separate initialization of the hooks from the beginning of
collecting stack traces. That way we can at least have a (more) accurate count
of the malloc'd memory.

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks.cc:143: static const intptr_t kSkipCount = 5;
Is this the right value for Debug and Release? ia32 and x64? gcc and clang?

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks.cc:198: delete result->value;
result->value = NULL;

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks.cc:316: delete address_map_;
address_map_ = NULL;

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hooks.h
File runtime/vm/malloc_hooks.h (right):

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks.h:14: class Sample;
is this from vm/profiler.h? I'd prefer doing the #include here rather than a
forward declaration.

[Cutch, 2017-02-15 08:21:01]

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/dart.cc#new...
runtime/vm/dart.cc:235: // MallocHooks can't be initialized until StubCode has
been.
maybe add why?

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
File runtime/vm/malloc_hooks_test.cc (right):

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks_test.cc:122: #if defined(TARGET_OS_LINUX)
Not sure if this OS_LINUX test is a good idea

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks_test.cc:144:
reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(stack_trace[0]) + 10);
this won't be the same across MIPS/ARM/ARM64/etc

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks_test.cc:150: for (uintptr_t i = 0; i < 8; ++i) {
Where did 8 come from?

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:611: intptr_t skip_count = 0)
Support for skip count should be generalized and put into ProfilerStackWalker.

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:645: if (frame_count >= skip_count_ &&
!Append(reinterpret_cast<uword>(pc))) {
wrap in parens: (frame_count >= skip_count_)

Also, pretty sure you mean || here and not &&.

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:675: ++frame_count;
You should bump frame_count immediately after the Append call succeeds (line
~648)

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:813: static bool GetAndValidateStackBounds(Thread*
thread,
Seems like a bad merge here.

Please see this CL: https://codereview.chromium.org/2680123004/  (which landed a
few days ago). It introduces a replacement called
GetAndValidateThreadStackBounds. Your code should call that.

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:1414: if (sample->port() != filter->port() &&
wrap in parens: (sample->port() != filter->port())

[bkonyi, 2017-02-16 00:28:29]

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/dart.cc#new...
runtime/vm/dart.cc:235: // MallocHooks can't be initialized until StubCode has
been.
On 2017/02/15 08:21:00, Cutch wrote:
> maybe add why?

Done.

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/dart.cc#new...
runtime/vm/dart.cc:276: MallocHooks::InitOnce();
On 2017/02/15 05:50:45, zra wrote:
> Maybe add a TODO to separate initialization of the hooks from the beginning of
> collecting stack traces. That way we can at least have a (more) accurate count
> of the malloc'd memory.

Done.

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks.cc:143: static const intptr_t kSkipCount = 5;
On 2017/02/15 05:50:46, zra wrote:
> Is this the right value for Debug and Release? ia32 and x64? gcc and clang?

I've just tested and this is the right value for debug and release, ia32 and
x64. I'm pretty sure we only define DART_USE_TCMALLOC in the .gn files, so I
don't think we install the malloc hooks when using gcc.

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks.cc:198: delete result->value;
On 2017/02/15 05:50:46, zra wrote:
> result->value = NULL;

Done.

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks.cc:316: delete address_map_;
On 2017/02/15 05:50:46, zra wrote:
> address_map_ = NULL;

Done.

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hooks.h
File runtime/vm/malloc_hooks.h (right):

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks.h:14: class Sample;
On 2017/02/15 05:50:46, zra wrote:
> is this from vm/profiler.h? I'd prefer doing the #include here rather than a
> forward declaration.

Seems to be that there's a cycle created when including profiler.h, so I think
we need to keep this forward declaration.

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
File runtime/vm/malloc_hooks_test.cc (right):

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks_test.cc:122: #if defined(TARGET_OS_LINUX)
On 2017/02/15 08:21:00, Cutch wrote:
> Not sure if this OS_LINUX test is a good idea

After reading the comments below, I agree with you. There's too much platform
specific stuff that could go wrong in this test. I've removed the test and added
one that's less brittle.

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:611: intptr_t skip_count = 0)
On 2017/02/15 08:21:00, Cutch wrote:
> Support for skip count should be generalized and put into ProfilerStackWalker.

Done.

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:645: if (frame_count >= skip_count_ &&
!Append(reinterpret_cast<uword>(pc))) {
On 2017/02/15 08:21:00, Cutch wrote:
> wrap in parens: (frame_count >= skip_count_)
> 
> Also, pretty sure you mean || here and not &&.

Acknowledged.

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:675: ++frame_count;
On 2017/02/15 08:21:00, Cutch wrote:
> You should bump frame_count immediately after the Append call succeeds (line
> ~648)

Acknowledged.

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:813: static bool GetAndValidateStackBounds(Thread*
thread,
On 2017/02/15 08:21:00, Cutch wrote:
> Seems like a bad merge here.
> 
> Please see this CL: https://codereview.chromium.org/2680123004/  (which landed
a
> few days ago). It introduces a replacement called
> GetAndValidateThreadStackBounds. Your code should call that.

Not a bad merge, just haven't rebased in awhile. Done.

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:1414: if (sample->port() != filter->port() &&
On 2017/02/15 08:21:00, Cutch wrote:
> wrap in parens: (sample->port() != filter->port())

Done.

[zra, 2017-02-16 21:45:28]

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2680213002/diff/100001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks.cc:143: static const intptr_t kSkipCount = 5;
On 2017/02/16 00:28:28, bkonyi wrote:
> On 2017/02/15 05:50:46, zra wrote:
> > Is this the right value for Debug and Release? ia32 and x64? gcc and clang?
> 
> I've just tested and this is the right value for debug and release, ia32 and
> x64. I'm pretty sure we only define DART_USE_TCMALLOC in the .gn files, so I
> don't think we install the malloc hooks when using gcc.

ia32 is built with gcc on Linux, so we do use the malloc hooks with gcc.

Is there any assert we can place in the code to give an indication if this value
is wrong on some configuration? Could you set it to 4, for example, and then
check for an expected value in the first slot?

https://codereview.chromium.org/2680213002/diff/140001/runtime/vm/malloc_hook...
File runtime/vm/malloc_hooks_test.cc (right):

https://codereview.chromium.org/2680213002/diff/140001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks_test.cc:94: __declspec(noinline) static uintptr_t
GetProgramCounter() {
Use DART_NOINLINE defined in platform/globals.h.

Also this looks copied from vm/profiler.cc. Is there any way to avoid that?
Maybe add this to class OS in vm/os.h.

https://codereview.chromium.org/2680213002/diff/140001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks_test.cc:105: static char* __attribute__((noinline))
DART_NOINLINE

https://codereview.chromium.org/2680213002/diff/140001/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/2680213002/diff/140001/runtime/vm/profiler.h#...
runtime/vm/profiler.h:489: if (sample->port() != visitor->port() &&
parens around != clause

https://codereview.chromium.org/2680213002/diff/140001/runtime/vm/profiler_te...
File runtime/vm/profiler_test.cc (right):

https://codereview.chromium.org/2680213002/diff/140001/runtime/vm/profiler_te...
runtime/vm/profiler_test.cc:2470: // The malloc hooks change the original
behaviour of this test, so disable
It might be worthwhile to add some more detail to this comment in case we should
update the test instead of disabling MallocHooks.

[bkonyi, 2017-02-16 22:55:06]

https://codereview.chromium.org/2680213002/diff/140001/runtime/vm/malloc_hook...
File runtime/vm/malloc_hooks_test.cc (right):

https://codereview.chromium.org/2680213002/diff/140001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks_test.cc:94: __declspec(noinline) static uintptr_t
GetProgramCounter() {
On 2017/02/16 21:45:27, zra wrote:
> Use DART_NOINLINE defined in platform/globals.h.
> 
> Also this looks copied from vm/profiler.cc. Is there any way to avoid that?
> Maybe add this to class OS in vm/os.h.

Yes, this was from vm/profiler.cc. I've moved the declaration to vm/os.h and
implemented it in os_*.cc.

https://codereview.chromium.org/2680213002/diff/140001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks_test.cc:105: static char* __attribute__((noinline))
On 2017/02/16 21:45:27, zra wrote:
> DART_NOINLINE

Done.

https://codereview.chromium.org/2680213002/diff/140001/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/2680213002/diff/140001/runtime/vm/profiler.h#...
runtime/vm/profiler.h:489: if (sample->port() != visitor->port() &&
On 2017/02/16 21:45:27, zra wrote:
> parens around != clause

I swear I did this before... Done now.

https://codereview.chromium.org/2680213002/diff/140001/runtime/vm/profiler_te...
File runtime/vm/profiler_test.cc (right):

https://codereview.chromium.org/2680213002/diff/140001/runtime/vm/profiler_te...
runtime/vm/profiler_test.cc:2470: // The malloc hooks change the original
behaviour of this test, so disable
On 2017/02/16 21:45:27, zra wrote:
> It might be worthwhile to add some more detail to this comment in case we
should
> update the test instead of disabling MallocHooks.

Apparently this test works now with MallocHooks enabled. I must have been doing
something weird in an earlier CL which was changing the exclusive/inclusive tick
counts. Removed.

[zra, 2017-02-16 23:15:49]

lgtm with nits

Also wait for John to approve.

In the meantime maybe get a run from golem to see if there is any perf impact.

https://codereview.chromium.org/2680213002/diff/160001/runtime/vm/os_thread.h
File runtime/vm/os_thread.h (right):

https://codereview.chromium.org/2680213002/diff/160001/runtime/vm/os_thread.h...
runtime/vm/os_thread.h:11: #include "vm/malloc_hooks.h"
Why is this included here?

https://codereview.chromium.org/2680213002/diff/160001/runtime/vm/profiler_te...
File runtime/vm/profiler_test.cc (right):

https://codereview.chromium.org/2680213002/diff/160001/runtime/vm/profiler_te...
runtime/vm/profiler_test.cc:10: #include "vm/malloc_hooks.h"
Is this include still needed?

[bkonyi, 2017-02-16 23:25:31]

I've removed the extra includes and started a Golem run.

https://codereview.chromium.org/2680213002/diff/160001/runtime/vm/os_thread.h
File runtime/vm/os_thread.h (right):

https://codereview.chromium.org/2680213002/diff/160001/runtime/vm/os_thread.h...
runtime/vm/os_thread.h:11: #include "vm/malloc_hooks.h"
On 2017/02/16 23:15:49, zra wrote:
> Why is this included here?

This is probably something I forgot to remove from an earlier change or when I
was trying something... Removed.

https://codereview.chromium.org/2680213002/diff/160001/runtime/vm/profiler_te...
File runtime/vm/profiler_test.cc (right):

https://codereview.chromium.org/2680213002/diff/160001/runtime/vm/profiler_te...
runtime/vm/profiler_test.cc:10: #include "vm/malloc_hooks.h"
On 2017/02/16 23:15:49, zra wrote:
> Is this include still needed?

I don't believe so. Removed.

[Cutch, 2017-02-21 18:53:35]

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/malloc_hook...
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks.cc:203: void Clear() {
this should be marked virtual in the base class?

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:1107: stack_lower = sp;
OSThread::GetCurrentStackBounds

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/profiler.h#...
runtime/vm/profiler.h:489: if ((sample->port() != visitor->port()) &&
It seems like native samples have a port of ILLEGAL_PORT_ID. Do we need the
!sample->is_native_allocation_sample condition?

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/service.cc#...
runtime/vm/service.cc:3232: RUNNABLE_ISOLATE_PARAMETER,
DO we want this to be a vm-level service request?

NO_ISOLATE_PARAMETER

[bkonyi, 2017-02-22 19:12:51]

I've addressed John's latest comments, but had to make some additional changes
to allow for benchmarks and some larger tests to disable stack trace collection
as it causes them to timeout.

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/malloc_hook...
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/malloc_hook...
runtime/vm/malloc_hooks.cc:203: void Clear() {
On 2017/02/21 18:53:35, Cutch wrote:
> this should be marked virtual in the base class?

Done.

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:1107: stack_lower = sp;
On 2017/02/21 18:53:35, Cutch wrote:
> OSThread::GetCurrentStackBounds

Done.

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/profiler.h#...
runtime/vm/profiler.h:489: if ((sample->port() != visitor->port()) &&
On 2017/02/21 18:53:35, Cutch wrote:
> It seems like native samples have a port of ILLEGAL_PORT_ID. Do we need the
> !sample->is_native_allocation_sample condition?

No, we don't. I've removed the code associated with is_native_allocation_sample.

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/2680213002/diff/180001/runtime/vm/service.cc#...
runtime/vm/service.cc:3232: RUNNABLE_ISOLATE_PARAMETER,
On 2017/02/21 18:53:35, Cutch wrote:
> DO we want this to be a vm-level service request?
> 
> NO_ISOLATE_PARAMETER
> 

Done.

[bkonyi, 2017-02-22 19:12:53]

I've addressed John's latest comments, but had to make some additional changes
to allow for benchmarks and some larger tests to disable stack trace collection
as it causes them to timeout.

[Cutch, 2017-02-22 23:15:56]

https://codereview.chromium.org/2680213002/diff/200001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/2680213002/diff/200001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:1109: return NULL;
You might want to add some code like:

AtomicOperations::IncrementInt64By(
        &counters_.single_frame_sample_register_check, 1);

but with your own counter here and in the next return NULL block.

This will let you diagnose why the stack walker isn't working (someday in the
future)

https://codereview.chromium.org/2680213002/diff/200001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:1417: !(sample->port() == ILLEGAL_PORT)) {
What I meant here was to write the code as follows:

if (sample->port() != filter->port()) {
  // Another isolate.
  continue;
}

This is sufficient because:

1) All native allocation samples have ILLEGAL_PORT.
2) The native allocation filter's port is ILLEGAL_PORT.
3) All other filters have a port that isn't ILLEGAL_PORT.

---

Also, Sample should still have an explicit bit in the state indicating it is a
native allocation sample.

https://codereview.chromium.org/2680213002/diff/200001/runtime/vm/profiler_se...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/2680213002/diff/200001/runtime/vm/profiler_se...
runtime/vm/profiler_service.cc:2683: return !sample->is_allocation_sample() &&
(sample->port() == ILLEGAL_PORT);
is_allocation_sample is a different kind of sample (an allocation from Dart
code).

you should keep is_native_allocation_sample(), etc.

https://codereview.chromium.org/2680213002/diff/200001/runtime/vm/profiler_se...
runtime/vm/profiler_service.cc:2738: bool FilterSample(Sample* sample) { return
(sample->port() == ILLEGAL_PORT); }
return sample->is_native_allocation_sample();

[bkonyi, 2017-02-23 00:25:20]

https://codereview.chromium.org/2680213002/diff/200001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/2680213002/diff/200001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:1109: return NULL;
On 2017/02/22 23:15:55, Cutch wrote:
> You might want to add some code like:
> 
> AtomicOperations::IncrementInt64By(
>         &counters_.single_frame_sample_register_check, 1);
> 
> but with your own counter here and in the next return NULL block.
> 
> This will let you diagnose why the stack walker isn't working (someday in the
> future)

Done (I think).

https://codereview.chromium.org/2680213002/diff/200001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:1417: !(sample->port() == ILLEGAL_PORT)) {
On 2017/02/22 23:15:55, Cutch wrote:
> What I meant here was to write the code as follows:
> 
> if (sample->port() != filter->port()) {
>   // Another isolate.
>   continue;
> }
> 
> This is sufficient because:
> 
> 1) All native allocation samples have ILLEGAL_PORT.
> 2) The native allocation filter's port is ILLEGAL_PORT.
> 3) All other filters have a port that isn't ILLEGAL_PORT.
> 
> ---
> 
> Also, Sample should still have an explicit bit in the state indicating it is a
> native allocation sample.

Done.

https://codereview.chromium.org/2680213002/diff/200001/runtime/vm/profiler_se...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/2680213002/diff/200001/runtime/vm/profiler_se...
runtime/vm/profiler_service.cc:2683: return !sample->is_allocation_sample() &&
(sample->port() == ILLEGAL_PORT);
On 2017/02/22 23:15:55, Cutch wrote:
> is_allocation_sample is a different kind of sample (an allocation from Dart
> code).
> 
> you should keep is_native_allocation_sample(), etc.

Done.

https://codereview.chromium.org/2680213002/diff/200001/runtime/vm/profiler_se...
runtime/vm/profiler_service.cc:2738: bool FilterSample(Sample* sample) { return
(sample->port() == ILLEGAL_PORT); }
On 2017/02/22 23:15:56, Cutch wrote:
> return sample->is_native_allocation_sample();

Done.

[Cutch, 2017-02-23 18:10:25]

https://codereview.chromium.org/2680213002/diff/240001/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/2680213002/diff/240001/runtime/vm/profiler.h#...
runtime/vm/profiler.h:492: sample->port() != ILLEGAL_PORT) {
there should be no need for the && sample->port != ILLEGAL_PORT.

https://codereview.chromium.org/2680213002/diff/240001/runtime/vm/profiler_se...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/2680213002/diff/240001/runtime/vm/profiler_se...
runtime/vm/profiler_service.cc:2721: !sample->is_native_allocation_sample();
A sample should never be tagged as both an 'allocation sample' and a 'native
allocation sample' so there is no need for &&
!sample->is_native_allocation_sample()

[bkonyi, 2017-02-23 19:34:17]

https://codereview.chromium.org/2680213002/diff/240001/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/2680213002/diff/240001/runtime/vm/profiler.h#...
runtime/vm/profiler.h:492: sample->port() != ILLEGAL_PORT) {
On 2017/02/23 18:10:25, Cutch wrote:
> there should be no need for the && sample->port != ILLEGAL_PORT.

You're right. Thought I removed these checks in this patch, guess I missed one.
Removed.

https://codereview.chromium.org/2680213002/diff/240001/runtime/vm/profiler_se...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/2680213002/diff/240001/runtime/vm/profiler_se...
runtime/vm/profiler_service.cc:2721: !sample->is_native_allocation_sample();
On 2017/02/23 18:10:25, Cutch wrote:
> A sample should never be tagged as both an 'allocation sample' and a 'native
> allocation sample' so there is no need for &&
> !sample->is_native_allocation_sample()

Makes sense. Removed.

[Cutch, 2017-02-23 19:37:28]

lgtm

LGTM

[bkonyi, 2017-02-23 20:42:25]

Description was changed from

==========
Updated MallocHooks to collect stack traces when memory is allocated.

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

to

==========
Updated MallocHooks to collect stack traces when memory is allocated.

BUG=
R=johnmccutchan@google.com, zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/47f0da9cd79d41c3f23b03a0f6cc771566e62c78
==========

[bkonyi, 2017-02-23 20:42:27]

Committed patchset #14 (id:260001) manually as
47f0da9cd79d41c3f23b03a0f6cc771566e62c78 (presubmit successful).