Updated native memory allocation profiling to use its own sample buffer instead of sharing a sample…

Updated native memory allocation profiling to use its own sample buffer instead of sharing a sample buffer with the CPU profiler, which did lead to samples being quickly overwritten even if the memory hadn't yet been freed. This will ensure allocations are accurately displayed in Observatory as the CPU profiler will no longer overwrite samples associated with native memory allocations.

BUG=
R=zra@google.com

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

[bkonyi, 2017-06-29 20:50:52]

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

[zra, 2017-06-30 18:49:17]

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

https://codereview.chromium.org/2966593002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:232: intptr_t AllocationSampleBuffer::ReserveSampleSlot()
{
If the lock must be held by this function, please call it
ReserveSampleSlotLocked, and ASSERT that the lock is held by the current thread
at the top.

https://codereview.chromium.org/2966593002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:242: return
AtomicOperations::FetchAndIncrement(&cursor_);
You're holding a lock, so does this still need to use an atomic?

https://codereview.chromium.org/2966593002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:1239: "Native memory profile sample buffer is full. Not
recording allocation "
Maybe "Native memory profile sample buffer is full because there are more than N
outstanding allocations. Not recording additional allocations" if there's some
way to know what N is.

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

https://codereview.chromium.org/2966593002/diff/1/runtime/vm/profiler.h#newco...
runtime/vm/profiler.h:361: 
rm extra newline

https://codereview.chromium.org/2966593002/diff/1/runtime/vm/profiler.h#newco...
runtime/vm/profiler.h:371: protected:
I'm having trouble finding why this needs to be protected an not private. If it
needs to be protected for some reason, could you add a comment here about what
needs access.

https://codereview.chromium.org/2966593002/diff/1/runtime/vm/profiler.h#newco...
runtime/vm/profiler.h:434: // If the sample is an allocation sample, we need to
check that the
Maybe assert that the sample is an allocation sample.

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

https://codereview.chromium.org/2966593002/diff/1/runtime/vm/profiler_service...
runtime/vm/profiler_service.cc:1291: samples_ =
sample_buffer_->BuildProcessedSampleBuffer(filter_);
Retain the NULL check, or ASSERT non-null.

[bkonyi, 2017-07-05 18:20:53]

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

https://codereview.chromium.org/2966593002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:232: intptr_t AllocationSampleBuffer::ReserveSampleSlot()
{
On 2017/06/30 18:49:16, zra wrote:
> If the lock must be held by this function, please call it
> ReserveSampleSlotLocked, and ASSERT that the lock is held by the current
thread
> at the top.

Done.

https://codereview.chromium.org/2966593002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:242: return
AtomicOperations::FetchAndIncrement(&cursor_);
On 2017/06/30 18:49:16, zra wrote:
> You're holding a lock, so does this still need to use an atomic?

Nope, removed.

https://codereview.chromium.org/2966593002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:1239: "Native memory profile sample buffer is full. Not
recording allocation "
On 2017/06/30 18:49:16, zra wrote:
> Maybe "Native memory profile sample buffer is full because there are more than
N
> outstanding allocations. Not recording additional allocations" if there's some
> way to know what N is.

Done.

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

https://codereview.chromium.org/2966593002/diff/1/runtime/vm/profiler.h#newco...
runtime/vm/profiler.h:361: 
On 2017/06/30 18:49:17, zra wrote:
> rm extra newline

Done.

https://codereview.chromium.org/2966593002/diff/1/runtime/vm/profiler.h#newco...
runtime/vm/profiler.h:371: protected:
On 2017/06/30 18:49:17, zra wrote:
> I'm having trouble finding why this needs to be protected an not private. If
it
> needs to be protected for some reason, could you add a comment here about what
> needs access.

This is just something I forgot to change back since I was going to inherit from
Sample at one point. Changed to private.

https://codereview.chromium.org/2966593002/diff/1/runtime/vm/profiler.h#newco...
runtime/vm/profiler.h:434: // If the sample is an allocation sample, we need to
check that the
On 2017/06/30 18:49:17, zra wrote:
> Maybe assert that the sample is an allocation sample.

I'll assert that the sample has a valid allocation address since I've removed
is_native_allocation_sample().

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

https://codereview.chromium.org/2966593002/diff/1/runtime/vm/profiler_service...
runtime/vm/profiler_service.cc:1291: samples_ =
sample_buffer_->BuildProcessedSampleBuffer(filter_);
On 2017/06/30 18:49:17, zra wrote:
> Retain the NULL check, or ASSERT non-null.

Done.

[zra, 2017-07-10 16:14:36]

lgtm

[bkonyi, 2017-07-10 17:31:51]

Description was changed from

==========
Updated native memory allocation profiling to use its own sample buffer instead
of sharing a sample buffer with the CPU profiler, which did lead to samples
being quickly overwritten even if the memory hadn't yet been freed. This will
ensure allocations are accurately displayed in Observatory as the CPU profiler
will no longer overwrite samples associated with native memory allocations.

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

to

==========
Updated native memory allocation profiling to use its own sample buffer instead
of sharing a sample buffer with the CPU profiler, which did lead to samples
being quickly overwritten even if the memory hadn't yet been freed. This will
ensure allocations are accurately displayed in Observatory as the CPU profiler
will no longer overwrite samples associated with native memory allocations.

BUG=
R=zra@google.com

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

[bkonyi, 2017-07-10 17:31:52]

Committed patchset #2 (id:20001) manually as
cc68a66f2845acc7bed5697e3c504c37313ebd01 (presubmit successful).