- Add assertions in MutexLocker/MonitorLocker to ensure that the code enclosed

- Add assertions in MutexLocker/MonitorLocker to ensure that the code enclosed
  in these blocks will not have a safepoint operation
- changed boxed_field_list_monitor_ to boxed_field_list_mutex_ as we only
  need a mutex for guarding access to boxed_field_list_ as changed the
  lock to use SafepointMutexLocker as the list addition code could potentially
  allocate and result in GC (safepoint operation)
- Added a SafepointMonitorLocker as we have a function Isolate::VisitIsolates
  which could potentially have safepoints in the enclosed block.
- Make the lock around MegamorphicCacheTable::Lookup a SafepointMutexLocker
  as the look up code seems to be allocating memory while the lock is held.
- Changed the ThreadPool and MessageHandler code to account for the new
  MonitorLocker usage standard
- Fixed PortMap::PrintPortsForMessageHandler to use SafepointMutexLocker as
  the code it encloses calls into Dart
- Removed profiler_ field in class Profiler as it doesn't seem to be used.

R=johnmccutchan@google.com, srdjan@google.com

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

[siva, 2016-03-01 00:36:51]

asiva@google.com changed reviewers:
+ johnmccutchan@google.com, srdjan@google.com

[Cutch, 2016-03-01 15:08:07]

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.cc
File runtime/vm/lockers.cc (right):

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.cc#n...
runtime/vm/lockers.cc:82: SafepointHandler* handler =
thread->isolate()->safepoint_handler();
This 'fast update of the thread state' code is the same as SafepointMutexLocker.
Maybe factor it out into a common routine?

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h
File runtime/vm/lockers.h (right):

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h#ne...
runtime/vm/lockers.h:29: *      m->Enter();
Why don't we make the Enter and Exit on the Mutex/Monitor classes protected and
force all users to use the Lockers?

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h#ne...
runtime/vm/lockers.h:45: explicit MutexLocker(Mutex* mutex, bool
no_safepoint_scope = true)
Can we use an enum rather than a boolean as the second parameter here and on the
MonitorLocker?

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h#ne...
runtime/vm/lockers.h:141: #if defined(DEBUG)
here and elsewhere: 

DEBUG_ONLY(
)

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h#ne...
runtime/vm/lockers.h:193: * This locker abstraction should be used when the lock
is more coarse
I don't see why the granularity of the lock matters. How about:

This locker abstraction should be used when the enclosing code could potentially
trigger a safepoint.

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h#ne...
runtime/vm/lockers.h:229: * NOTE: please do not use the passed in monitor object
independent of the locker
line length

[srdjan, 2016-03-01 16:46:23]

lgtm

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h
File runtime/vm/lockers.h (right):

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h#ne...
runtime/vm/lockers.h:18: * This locker abstraction should only be used when the
enclosing code will
s/will not/cannot/

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h#ne...
runtime/vm/lockers.h:96: * This locker abstraction should only be used when the
enclosing code will
ditto?

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/os_thread_an...
File runtime/vm/os_thread_android.cc (right):

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/os_thread_an...
runtime/vm/os_thread_android.cc:341: // Return false if the lock is busy and
locking failed.
Maybe copy this comment into class definition, as description of TryEnter

[siva, 2016-03-01 18:59:54]

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.cc
File runtime/vm/lockers.cc (right):

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.cc#n...
runtime/vm/lockers.cc:82: SafepointHandler* handler =
thread->isolate()->safepoint_handler();
On 2016/03/01 15:08:07, Cutch wrote:
> This 'fast update of the thread state' code is the same as
SafepointMutexLocker.
> Maybe factor it out into a common routine?

Done.

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h
File runtime/vm/lockers.h (right):

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h#ne...
runtime/vm/lockers.h:18: * This locker abstraction should only be used when the
enclosing code will
On 2016/03/01 16:46:23, srdjan wrote:
> s/will not/cannot/

Done.

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h#ne...
runtime/vm/lockers.h:29: *      m->Enter();
On 2016/03/01 15:08:07, Cutch wrote:
> Why don't we make the Enter and Exit on the Mutex/Monitor classes protected
and
> force all users to use the Lockers?

I tried this but there are a number of places where mutexes are used with the
locker object, e.g timeline, pagespace, unit tests etc.
Maybe I should try this in a different cl.

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h#ne...
runtime/vm/lockers.h:45: explicit MutexLocker(Mutex* mutex, bool
no_safepoint_scope = true)
On 2016/03/01 15:08:07, Cutch wrote:
> Can we use an enum rather than a boolean as the second parameter here and on
the
> MonitorLocker?

An enum here seems like an overkill, we want the no safepoint scope
increment/decrement always, There is just one place (when a thread is scheduled
and unscheduled from an isolate) where the check should not happen because we
have a current thread on entry but none on exit, I have added a comment in those
calls to make it more clear.

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h#ne...
runtime/vm/lockers.h:96: * This locker abstraction should only be used when the
enclosing code will
On 2016/03/01 16:46:23, srdjan wrote:
> ditto?

Done.

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h#ne...
runtime/vm/lockers.h:141: #if defined(DEBUG)
On 2016/03/01 15:08:07, Cutch wrote:
> here and elsewhere: 
> 
> DEBUG_ONLY(
> )

I looked at uses of DEBUG_ONLY and looks like the pattern is to use it when an
entire function is used in DEBUG only, that is not the case here and would
prefer to leave it as such.

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h#ne...
runtime/vm/lockers.h:193: * This locker abstraction should be used when the lock
is more coarse
On 2016/03/01 15:08:07, Cutch wrote:
> I don't see why the granularity of the lock matters. How about:
> 
> This locker abstraction should be used when the enclosing code could
potentially
> trigger a safepoint.

Changed the comment to remove the coarse wording.

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/lockers.h#ne...
runtime/vm/lockers.h:229: * NOTE: please do not use the passed in monitor object
independent of the locker
On 2016/03/01 15:08:07, Cutch wrote:
> line length

Done.

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/os_thread_an...
File runtime/vm/os_thread_android.cc (right):

https://codereview.chromium.org/1748953003/diff/20001/runtime/vm/os_thread_an...
runtime/vm/os_thread_android.cc:341: // Return false if the lock is busy and
locking failed.
On 2016/03/01 16:46:23, srdjan wrote:
> Maybe copy this comment into class definition, as description of TryEnter

Done.

[Cutch, 2016-03-01 19:15:54]

lgtm

[siva, 2016-03-01 20:33:53]

Description was changed from

==========
- Add assertions in MutexLocker/MonitorLocker to ensure that the code enclosed
  in these blocks will not have a safepoint operation
- changed boxed_field_list_monitor_ to boxed_field_list_mutex_ as we only
  need a mutex for guarding access to boxed_field_list_ as changed the
  lock to use SafepointMutexLocker as the list addition code could potentially
  allocate and result in GC (safepoint operation)
- Added a SafepointMonitorLocker as we have a function Isolate::VisitIsolates
  which could potentially have safepoints in the enclosed block.
- Make the lock around MegamorphicCacheTable::Lookup a SafepointMutexLocker
  as the look up code seems to be allocating memory while the lock is held.
- Changed the ThreadPool and MessageHandler code to account for the new
  MonitorLocker usage standard
- Fixed PortMap::PrintPortsForMessageHandler to use SafepointMutexLocker as
  the code it encloses calls into Dart
- Removed profiler_ field in class Profiler as it doesn't seem to be used.
==========

to

==========
- Add assertions in MutexLocker/MonitorLocker to ensure that the code enclosed
  in these blocks will not have a safepoint operation
- changed boxed_field_list_monitor_ to boxed_field_list_mutex_ as we only
  need a mutex for guarding access to boxed_field_list_ as changed the
  lock to use SafepointMutexLocker as the list addition code could potentially
  allocate and result in GC (safepoint operation)
- Added a SafepointMonitorLocker as we have a function Isolate::VisitIsolates
  which could potentially have safepoints in the enclosed block.
- Make the lock around MegamorphicCacheTable::Lookup a SafepointMutexLocker
  as the look up code seems to be allocating memory while the lock is held.
- Changed the ThreadPool and MessageHandler code to account for the new
  MonitorLocker usage standard
- Fixed PortMap::PrintPortsForMessageHandler to use SafepointMutexLocker as
  the code it encloses calls into Dart
- Removed profiler_ field in class Profiler as it doesn't seem to be used.

R=johnmccutchan@google.com, srdjan@google.com

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

[siva, 2016-03-01 20:33:55]

Committed patchset #3 (id:40001) manually as
e83151cc77438e5e9a5293eabfb1066cda071ed4 (presubmit successful).