Disabled MallocHooks code in Release due to performance concerns. Added enable_malloc_hooks flag to…

Disabled MallocHooks code in Release due to performance concerns. Added enable_malloc_hooks flag to allow users to enable the hooks in Release mode if they so desire.

BUG=
R=zra@google.com

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

[bkonyi, 2017-02-17 23:27:24]

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

[rmacnak, 2017-02-17 23:39:22]

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

https://codereview.chromium.org/2700033003/diff/1/runtime/vm/malloc_hooks_tes...
runtime/vm/malloc_hooks_test.cc:28: UNIT_TEST_CASE(BasicMallocHookTest) {
If it's available in release mode, it should be tested in release mode too.

bool saved_enable_malloc_hooks = FLAG_enable_malloc_hooks;
FLAG_enable_malloc_hooks = true;
...
FLAG_enable_malloc_hooks = saved_enable_malloc_hooks

[zra, 2017-02-17 23:43:27]

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

https://codereview.chromium.org/2700033003/diff/1/runtime/vm/flag_list.h#newc...
runtime/vm/flag_list.h:78: "Enable native memory statistic collection. NOTE:
This will likely result" \
I'd remove the note. It's not quite *that* bad... =)

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

https://codereview.chromium.org/2700033003/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:206: if (FLAG_enable_malloc_hooks) {
if (!FLAG_enable_malloc_hooks) {
  return;
}

https://codereview.chromium.org/2700033003/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:224: if (FLAG_enable_malloc_hooks) {
ditto

https://codereview.chromium.org/2700033003/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:242: if (FLAG_enable_malloc_hooks) {
ditto

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

https://codereview.chromium.org/2700033003/diff/1/runtime/vm/malloc_hooks_tes...
runtime/vm/malloc_hooks_test.cc:28: UNIT_TEST_CASE(BasicMallocHookTest) {
On 2017/02/17 23:39:22, rmacnak wrote:
> If it's available in release mode, it should be tested in release mode too.
> 
> bool saved_enable_malloc_hooks = FLAG_enable_malloc_hooks;
> FLAG_enable_malloc_hooks = true;
> ...
> FLAG_enable_malloc_hooks = saved_enable_malloc_hooks

+1

[bkonyi, 2017-02-18 00:01:11]

I've gone ahead and re-enabled the malloc hook tests for release mode, as
requested.

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

https://codereview.chromium.org/2700033003/diff/1/runtime/vm/flag_list.h#newc...
runtime/vm/flag_list.h:78: "Enable native memory statistic collection. NOTE:
This will likely result" \
On 2017/02/17 23:43:27, zra wrote:
> I'd remove the note. It's not quite *that* bad... =)

Well, 50% reduction in performance in some areas isn't exactly what I'd call
great :). Done.

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

https://codereview.chromium.org/2700033003/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:206: if (FLAG_enable_malloc_hooks) {
On 2017/02/17 23:43:27, zra wrote:
> if (!FLAG_enable_malloc_hooks) {
>   return;
> }

Done.

https://codereview.chromium.org/2700033003/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:224: if (FLAG_enable_malloc_hooks) {
On 2017/02/17 23:43:27, zra wrote:
> ditto

Done.

https://codereview.chromium.org/2700033003/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:242: if (FLAG_enable_malloc_hooks) {
On 2017/02/17 23:43:27, zra wrote:
> ditto

Done.

[zra, 2017-02-18 00:03:33]

lgtm

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

https://codereview.chromium.org/2700033003/diff/1/runtime/vm/flag_list.h#newc...
runtime/vm/flag_list.h:78: "Enable native memory statistic collection. NOTE:
This will likely result" \
On 2017/02/18 00:01:11, bkonyi wrote:
> On 2017/02/17 23:43:27, zra wrote:
> > I'd remove the note. It's not quite *that* bad... =)
> 
> Well, 50% reduction in performance in some areas isn't exactly what I'd call
> great :). Done.

They're microbenchmarks. Not likely to be that bad in any real app.

[bkonyi, 2017-02-18 00:07:48]

Description was changed from

==========
Disabled MallocHooks code in Release due to performance concerns. Added
enable_malloc_hooks flag to allow users to enable the hooks in Release mode if
they so desire.

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

to

==========
Disabled MallocHooks code in Release due to performance concerns. Added
enable_malloc_hooks flag to allow users to enable the hooks in Release mode if
they so desire.

BUG=
R=zra@google.com

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

[bkonyi, 2017-02-18 00:07:50]

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