Don't consider --profile-vm when collecting allocation samples; introduce a separate --profile-vm-al

Don't consider --profile-vm when collecting allocation samples; introduce a separate --profile-vm-allocation instead.

This flag, which is currently enabled by default in Flutter, causes allocation tracing to collect the C++ stacks inside the VM rather than Dart stacks.

R=cbernaschina@google.com, zra@google.com

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

[rmacnak, 2017-06-15 20:41:04]

rmacnak@google.com changed reviewers:
+ cbernaschina@google.com, zra@google.com

[cbernaschina, 2017-06-15 20:43:44]

On 2017/06/15 20:41:05, rmacnak wrote:

lgtm

[zra, 2017-06-15 20:50:21]

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

https://codereview.chromium.org/2943543002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:1089: if (FLAG_profile_vm && false) {
Should we add a separate flag for this instead that is turned off in the Flutter
engine?

Maybe --profile_allocation_vm or --profile_vm_allocation. Not sure what a good
name would be.

[rmacnak, 2017-07-11 18:14:53]

Description was changed from

==========
Restore some functionality to the allocation profiler by considering only Dart
frames, which always have proper frame pointers.

Issue flutter/flutter#10744
==========

to

==========
Don't consider --profile-vm when collecting allocation samples; introduce a
separate --profile-vm-allocation instead.

This flag, which is currently enabled by default in Flutter, causes allocation
tracing to collect the C++ stacks inside the VM rather than Dart stacks.
==========

[rmacnak, 2017-07-11 18:17:18]

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

https://codereview.chromium.org/2943543002/diff/1/runtime/vm/profiler.cc#newc...
runtime/vm/profiler.cc:1089: if (FLAG_profile_vm && false) {
On 2017/06/15 20:50:21, zra wrote:
> Should we add a separate flag for this instead that is turned off in the
Flutter
> engine?
> 
> Maybe --profile_allocation_vm or --profile_vm_allocation. Not sure what a good
> name would be.

So even with good frame pointer, using the native stack walker doesn't produce
the traces we intended when introduce allocation tracing because the Dart code
is hidden behind too many C++ frames to show up in the profile. Removed the
reference to the frame pointers in Flutter Android bug and introduced a separate
flag as Zach suggests.

[zra, 2017-07-11 19:45:17]

On 2017/07/11 18:17:18, rmacnak wrote:
> https://codereview.chromium.org/2943543002/diff/1/runtime/vm/profiler.cc
> File runtime/vm/profiler.cc (right):
> 
>
https://codereview.chromium.org/2943543002/diff/1/runtime/vm/profiler.cc#newc...
> runtime/vm/profiler.cc:1089: if (FLAG_profile_vm && false) {
> On 2017/06/15 20:50:21, zra wrote:
> > Should we add a separate flag for this instead that is turned off in the
> Flutter
> > engine?
> > 
> > Maybe --profile_allocation_vm or --profile_vm_allocation. Not sure what a
good
> > name would be.
> 
> So even with good frame pointer, using the native stack walker doesn't produce
> the traces we intended when introduce allocation tracing because the Dart code
> is hidden behind too many C++ frames to show up in the profile. Removed the
> reference to the frame pointers in Flutter Android bug and introduced a
separate
> flag as Zach suggests.

Did you mean to upload these changes, or are they on a different CL?

[rmacnak, 2017-07-11 23:10:17]

On 2017/07/11 19:45:17, zra wrote:
> On 2017/07/11 18:17:18, rmacnak wrote:
> > https://codereview.chromium.org/2943543002/diff/1/runtime/vm/profiler.cc
> > File runtime/vm/profiler.cc (right):
> > 
> >
>
https://codereview.chromium.org/2943543002/diff/1/runtime/vm/profiler.cc#newc...
> > runtime/vm/profiler.cc:1089: if (FLAG_profile_vm && false) {
> > On 2017/06/15 20:50:21, zra wrote:
> > > Should we add a separate flag for this instead that is turned off in the
> > Flutter
> > > engine?
> > > 
> > > Maybe --profile_allocation_vm or --profile_vm_allocation. Not sure what a
> good
> > > name would be.
> > 
> > So even with good frame pointer, using the native stack walker doesn't
produce
> > the traces we intended when introduce allocation tracing because the Dart
code
> > is hidden behind too many C++ frames to show up in the profile. Removed the
> > reference to the frame pointers in Flutter Android bug and introduced a
> separate
> > flag as Zach suggests.
> 
> Did you mean to upload these changes, or are they on a different CL?

I forgot to upload :)  The changes are on this CL now.

[rmacnak, 2017-07-19 21:00:27]

Description was changed from

==========
Don't consider --profile-vm when collecting allocation samples; introduce a
separate --profile-vm-allocation instead.

This flag, which is currently enabled by default in Flutter, causes allocation
tracing to collect the C++ stacks inside the VM rather than Dart stacks.
==========

to

==========
Don't consider --profile-vm when collecting allocation samples; introduce a
separate --profile-vm-allocation instead.

This flag, which is currently enabled by default in Flutter, causes allocation
tracing to collect the C++ stacks inside the VM rather than Dart stacks.
==========

[zra, 2017-07-19 21:21:48]

lgtm

[rmacnak, 2017-07-19 22:00:58]

Description was changed from

==========
Don't consider --profile-vm when collecting allocation samples; introduce a
separate --profile-vm-allocation instead.

This flag, which is currently enabled by default in Flutter, causes allocation
tracing to collect the C++ stacks inside the VM rather than Dart stacks.
==========

to

==========
Don't consider --profile-vm when collecting allocation samples; introduce a
separate --profile-vm-allocation instead.

This flag, which is currently enabled by default in Flutter, causes allocation
tracing to collect the C++ stacks inside the VM rather than Dart stacks.

R=cbernaschina@google.com, zra@google.com

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

[rmacnak, 2017-07-19 22:00:59]

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