[vm] Don't enable the profiler by default. Enable the profiler at startup with --observe, or later …

[vm] Don't enable the profiler by default. Enable the profiler at startup with --observe, or later through Observatory.

Note there is no option to disable the profiler. The profiler on Mac, Linux, and Android is based on SIGPROF, and there is no way to wait for all the outstanding signals as required to be able to safely free the sample buffer.

Saves ~20MB on 64-bit VMs.

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

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

[rmacnak, 2017-07-28 21:35:56]

Description was changed from

==========
[vm] Don't enable the profiler by default. Enable the profiler at startup with
--observe, or later through Observatory.

Note there is no option to disable the profiler. The profiler on Mac, Linux, and
Android is based on SIGPROF, and there is no way to wait for all the outstanding
signals as required to be able to safely free the sample buffer.
==========

to

==========
[vm] Don't enable the profiler by default. Enable the profiler at startup with
--observe, or later through Observatory.

Note there is no option to disable the profiler. The profiler on Mac, Linux, and
Android is based on SIGPROF, and there is no way to wait for all the outstanding
signals as required to be able to safely free the sample buffer.

Saves ~20MB on 64-bit VMs.
==========

[rmacnak, 2017-07-28 22:15:25]

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

[zra, 2017-07-31 15:03:11]

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

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/dart.cc#newc...
runtime/vm/dart.cc:354: #ifndef PRODUCT
Is there any reason to use this rather than the usual !defined(PRODUCT) style?

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/dart.cc#newc...
runtime/vm/dart.cc:360: #endif
// !defined(PRODUCT)

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:205: #ifdef PRODUCT
ditto

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:206: bool stack_traces_only = true;
const

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

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:72: #if !defined(TARGET_ARCH_DBC)
I thought the profiler was kind of working somewhat for DBC. Does this CL turn
it off?

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:104: // Note we do not free the sample buffer because
there may be SIGPROFs
Can you set a "shutting down" bit somewhere that the signal handler will look
at? If it's set, then it can e.g. return immediately without touching the sample
buffer.

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

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/profiler.h#n...
runtime/vm/profiler.h:58: static void EnsureEnabled() {
It looks like this is only used for tests, so think the definition should
probably go in the source file.

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/program_visi...
File runtime/vm/program_visitor.cc (right):

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/program_visi...
runtime/vm/program_visitor.cc:601: #ifdef PRODUCT
if defined(PRODUCT)

[rmacnak, 2017-07-31 21:12:13]

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

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/dart.cc#newc...
runtime/vm/dart.cc:354: #ifndef PRODUCT
On 2017/07/31 15:03:11, zra wrote:
> Is there any reason to use this rather than the usual !defined(PRODUCT) style?

No reason, switched.

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:205: #ifdef PRODUCT
On 2017/07/31 15:03:11, zra wrote:
> ditto

Done.

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

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:72: #if !defined(TARGET_ARCH_DBC)
On 2017/07/31 15:03:11, zra wrote:
> I thought the profiler was kind of working somewhat for DBC. Does this CL turn
> it off?

Removed; this was a bad merge. I was working from a CL I started a long time ago
before the profiler support was added for DBC. The profiler indeed works for
DBC, though it only has function-level resolution instead of instruction-level
resolution since we avoid flushing the instruction pointer to memory the
profiler can read.

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:104: // Note we do not free the sample buffer because
there may be SIGPROFs
On 2017/07/31 15:03:11, zra wrote:
> Can you set a "shutting down" bit somewhere that the signal handler will look
> at? If it's set, then it can e.g. return immediately without touching the
sample
> buffer.

The signal handler could be between that check and writing the sample when we
want to free the sample buffer. We'd need a critical section around both the
check and the use of the sample buffer (which is nearly all of the signal
handler). thread_interrupt.cc has a lengthy comment explaining that we can't
take locks.

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

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/profiler.h#n...
runtime/vm/profiler.h:58: static void EnsureEnabled() {
On 2017/07/31 15:03:11, zra wrote:
> It looks like this is only used for tests, so think the definition should
> probably go in the source file.

Done.

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/program_visi...
File runtime/vm/program_visitor.cc (right):

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/program_visi...
runtime/vm/program_visitor.cc:601: #ifdef PRODUCT
On 2017/07/31 15:03:11, zra wrote:
> if defined(PRODUCT)

Done.

[cbernaschina, 2017-07-31 21:19:13]

https://codereview.chromium.org/2989093002/diff/60001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart (right):

https://codereview.chromium.org/2989093002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart:139: if
(tree.root.children.length == 1) {
Consider switch or else if

[zra, 2017-07-31 21:59:28]

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

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:104: // Note we do not free the sample buffer because
there may be SIGPROFs
On 2017/07/31 21:12:12, rmacnak wrote:
> On 2017/07/31 15:03:11, zra wrote:
> > Can you set a "shutting down" bit somewhere that the signal handler will
look
> > at? If it's set, then it can e.g. return immediately without touching the
> sample
> > buffer.
> 
> The signal handler could be between that check and writing the sample when we
> want to free the sample buffer. We'd need a critical section around both the
> check and the use of the sample buffer (which is nearly all of the signal
> handler). thread_interrupt.cc has a lengthy comment explaining that we can't
> take locks.

Maybe some thing like:

Signal handler:
AtomicInc(&some_counter);
if (shutting_down) {
  AtomicDec(&some_counter);
  return;
}
...
AtomicDec(&some_counter);


Thread Interrupter thread:
RemoveSignalHandler();
shutting_down = true;
while (some_counter > 0) {
  usleep(1000);
}
free(sample_buffer);

[rmacnak, 2017-08-01 19:58:58]

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

https://codereview.chromium.org/2989093002/diff/40001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:104: // Note we do not free the sample buffer because
there may be SIGPROFs
On 2017/07/31 21:59:28, zra wrote:
> On 2017/07/31 21:12:12, rmacnak wrote:
> > On 2017/07/31 15:03:11, zra wrote:
> > > Can you set a "shutting down" bit somewhere that the signal handler will
> look
> > > at? If it's set, then it can e.g. return immediately without touching the
> > sample
> > > buffer.
> > 
> > The signal handler could be between that check and writing the sample when
we
> > want to free the sample buffer. We'd need a critical section around both the
> > check and the use of the sample buffer (which is nearly all of the signal
> > handler). thread_interrupt.cc has a lengthy comment explaining that we can't
> > take locks.
> 
> Maybe some thing like:
> 
> Signal handler:
> AtomicInc(&some_counter);
> if (shutting_down) {
>   AtomicDec(&some_counter);
>   return;
> }
> ...
> AtomicDec(&some_counter);
> 
> 
> Thread Interrupter thread:
> RemoveSignalHandler();
> shutting_down = true;
> while (some_counter > 0) {
>   usleep(1000);
> }
> free(sample_buffer);

Added TODO(30309)

https://codereview.chromium.org/2989093002/diff/60001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart (right):

https://codereview.chromium.org/2989093002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart:139: if
(tree.root.children.length == 1) {
On 2017/07/31 21:19:13, cbernaschina wrote:
> Consider switch or else if

Done.

[zra, 2017-08-01 20:10:16]

lgtm

[cbernaschina, 2017-08-01 20:13:25]

On 2017/08/01 20:10:16, zra wrote:
> lgtm

lgtm

[rmacnak, 2017-08-02 00:57:13]

Description was changed from

==========
[vm] Don't enable the profiler by default. Enable the profiler at startup with
--observe, or later through Observatory.

Note there is no option to disable the profiler. The profiler on Mac, Linux, and
Android is based on SIGPROF, and there is no way to wait for all the outstanding
signals as required to be able to safely free the sample buffer.

Saves ~20MB on 64-bit VMs.
==========

to

==========
[vm] Don't enable the profiler by default. Enable the profiler at startup with
--observe, or later through Observatory.

Note there is no option to disable the profiler. The profiler on Mac, Linux, and
Android is based on SIGPROF, and there is no way to wait for all the outstanding
signals as required to be able to safely free the sample buffer.

Saves ~20MB on 64-bit VMs.

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

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

[rmacnak, 2017-08-02 00:57:15]

Committed patchset #5 (id:80001) manually as
cc08f2ddd87d7a5c692b48b2e8dd629addf4d827.