Removed MallocHookScope and replaced it with MallocLocker, which behaves like MutexLocker but also …

Removed MallocHookScope and replaced it with MallocLocker, which behaves like MutexLocker but also keeps track of which thread current holds the lock using MallocLocker.

BUG=
R=asiva@google.com

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

[bkonyi, 2017-02-22 21:10:06]

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

[zra, 2017-02-22 22:54:55]

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

https://codereview.chromium.org/2715493005/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:24: class MallocLocker : public ValueObject {
This seems generally useful. Should it go in os_thread.h with a more generic
name? OwnedLocker?

https://codereview.chromium.org/2715493005/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:46: DISALLOW_COPY_AND_ASSIGN(MallocLocker);
Is this redundant if this is inheriting from ValueObject?

[bkonyi, 2017-02-22 23:04:27]

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

https://codereview.chromium.org/2715493005/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:24: class MallocLocker : public ValueObject {
On 2017/02/22 22:54:54, zra wrote:
> This seems generally useful. Should it go in os_thread.h with a more generic
> name? OwnedLocker?

In its current state, I don't think that would work since owner_ is a static
variable. Mutex does have a owner_ field itself, but it's only defined in DEBUG,
which is part of the reason I created MallocLocker here. If we're okay adding a
ThreadId field to all mutexes, then we can just use MutexLocker and
Mutex::IsOwnedByCurrentThread.

https://codereview.chromium.org/2715493005/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:46: DISALLOW_COPY_AND_ASSIGN(MallocLocker);
On 2017/02/22 22:54:54, zra wrote:
> Is this redundant if this is inheriting from ValueObject?

Looks like it is. I had based MallocLocker on MutexLocker which included the
DISALLOW_COPY_AND_ASSIGN(MutexLocker), so it's redundant there as well.

[siva, 2017-02-22 23:53:04]

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

https://codereview.chromium.org/2715493005/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:24: class MallocLocker : public ValueObject {
On 2017/02/22 23:04:26, bkonyi wrote:
> On 2017/02/22 22:54:54, zra wrote:
> > This seems generally useful. Should it go in os_thread.h with a more generic
> > name? OwnedLocker?
> 
> In its current state, I don't think that would work since owner_ is a static
> variable. Mutex does have a owner_ field itself, but it's only defined in
DEBUG,
> which is part of the reason I created MallocLocker here. If we're okay adding
a
> ThreadId field to all mutexes, then we can just use MutexLocker and
> Mutex::IsOwnedByCurrentThread.

Discussed offline with Zach, For this CL we leave it the way it is here
and then in a new CL add a re-entrant lock class and use it here instead.

https://codereview.chromium.org/2715493005/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:168: ThreadId MallocLocker::owner_ =
OSThread::kInvalidThreadId;
Shouldn't this be clubbed just below the line :
Mutex* MallocHooksState::malloc_hook_mutex_ = new Mutex();

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

https://codereview.chromium.org/2715493005/diff/1/runtime/vm/os_thread.h#newc...
runtime/vm/os_thread.h:294: friend class MallocLocker;
Why is this needed here, doesn't seem right.

[bkonyi, 2017-02-23 00:28:46]

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

https://codereview.chromium.org/2715493005/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:24: class MallocLocker : public ValueObject {
On 2017/02/22 23:53:04, siva wrote:
> On 2017/02/22 23:04:26, bkonyi wrote:
> > On 2017/02/22 22:54:54, zra wrote:
> > > This seems generally useful. Should it go in os_thread.h with a more
generic
> > > name? OwnedLocker?
> > 
> > In its current state, I don't think that would work since owner_ is a static
> > variable. Mutex does have a owner_ field itself, but it's only defined in
> DEBUG,
> > which is part of the reason I created MallocLocker here. If we're okay
adding
> a
> > ThreadId field to all mutexes, then we can just use MutexLocker and
> > Mutex::IsOwnedByCurrentThread.
> 
> Discussed offline with Zach, For this CL we leave it the way it is here
> and then in a new CL add a re-entrant lock class and use it here instead.

That makes sense to me.

https://codereview.chromium.org/2715493005/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:46: DISALLOW_COPY_AND_ASSIGN(MallocLocker);
On 2017/02/22 22:54:54, zra wrote:
> Is this redundant if this is inheriting from ValueObject?

Done.

https://codereview.chromium.org/2715493005/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:168: ThreadId MallocLocker::owner_ =
OSThread::kInvalidThreadId;
On 2017/02/22 23:53:04, siva wrote:
> Shouldn't this be clubbed just below the line :
> Mutex* MallocHooksState::malloc_hook_mutex_ = new Mutex();

Done.

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

https://codereview.chromium.org/2715493005/diff/1/runtime/vm/os_thread.h#newc...
runtime/vm/os_thread.h:294: friend class MallocLocker;
On 2017/02/22 23:53:04, siva wrote:
> Why is this needed here, doesn't seem right.

This is needed for MallocLocker to be able to access the various methods needed
to take/release a Mutex as they are all private. Otherwise, we can't perform any
operations with the Mutex within MallocLocker.

[siva, 2017-02-23 01:46:36]

lgtm with one comment.

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

https://codereview.chromium.org/2715493005/diff/40001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:44: static ThreadId owner_;
It might be cleaner to not declare this as static here and instead
pass the owner as a parameter to the constructor similajr to mutex_
and move the static field owner_ to MallocHooksState.

[bkonyi, 2017-02-23 02:03:27]

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

https://codereview.chromium.org/2715493005/diff/40001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:44: static ThreadId owner_;
On 2017/02/23 01:46:36, siva wrote:
> It might be cleaner to not declare this as static here and instead
> pass the owner as a parameter to the constructor similajr to mutex_
> and move the static field owner_ to MallocHooksState.

Done.

[bkonyi, 2017-02-23 22:45:13]

Description was changed from

==========
Removed MallocHookScope and replaced it with MallocLocker, which behaves like
MutexLocker but also keeps track of which thread current holds the lock using
MallocLocker.

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

to

==========
Removed MallocHookScope and replaced it with MallocLocker, which behaves like
MutexLocker but also keeps track of which thread current holds the lock using
MallocLocker.

BUG=
R=asiva@google.com

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

[bkonyi, 2017-02-23 22:45:14]

Committed patchset #4 (id:60001) manually as
b19e4990428be2436a63111caef9f5eb6bb06775 (presubmit successful).