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

runtime/vm/lockers.h

Index: runtime/vm/lockers.h
diff --git a/runtime/vm/lockers.h b/runtime/vm/lockers.h
index fd324f44811551673dd7a25f823abac28bac1b2e..4ad2751c1410920ff0ffef5061dd6b8eb1f56465 100644
--- a/runtime/vm/lockers.h
+++ b/runtime/vm/lockers.h
@@ -13,37 +13,153 @@
 
 namespace dart {
 
+/*
+ * Normal mutex locker :
+ * This locker abstraction should only be used when the enclosing code will

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

s/will not/cannot/

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

Done.

+ * not trigger a safepoint. In debug mode this class increments the
+ * no_safepoint_scope_depth variable for the current thread when the lock is
+ * taken and decrements it when the lock is released. NOTE: please do not use
+ * the passed in mutex object independent of the locker class, For example the
+ * code below will not assert correctly:
+ *    {
+ *      MutexLocker ml(m);
+ *      ....
+ *      m->Exit();
+ *      ....
+ *      m->Enter();

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

Why don't we make the Enter and Exit on the Mutex/Monitor classes protected and
force all users to use the Lockers?

[siva, 2016/03/01 18:59:53]

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.

+ *      ...
+ *    }
+ * Always use the locker object even when the lock needs to be released
+ * temporarily, e.g:
+ *    {
+ *      MutexLocker ml(m);
+ *      ....
+ *      ml.Exit();
+ *      ....
+ *      ml.Enter();
+ *      ...
+ *    }
+ */
 class MutexLocker : public ValueObject {
  public:
-  explicit MutexLocker(Mutex* mutex) : mutex_(mutex) {
+  explicit MutexLocker(Mutex* mutex, bool no_safepoint_scope = true)

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

Can we use an enum rather than a boolean as the second parameter here and on the
MonitorLocker?

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

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.

+      : mutex_(mutex), no_safepoint_scope_(no_safepoint_scope) {
     ASSERT(mutex != NULL);
-    // TODO(iposva): Consider adding a no GC scope here.
+#if defined(DEBUG)
+    if (no_safepoint_scope_) {
+      Thread* thread = Thread::Current();
+      if (thread != NULL) {
+        thread->IncrementNoSafepointScopeDepth();
+      } else {
+        no_safepoint_scope_ = false;
+      }
+    }
+#endif
     mutex_->Lock();
   }
 
   virtual ~MutexLocker() {
     mutex_->Unlock();
-    // TODO(iposva): Consider decrementing the no GC scope here.
+#if defined(DEBUG)
+    if (no_safepoint_scope_) {
+      Thread::Current()->DecrementNoSafepointScopeDepth();
+    }
+#endif
+  }
+
+  void Lock() const {
+#if defined(DEBUG)
+    if (no_safepoint_scope_) {
+      Thread::Current()->IncrementNoSafepointScopeDepth();
+    }
+#endif
+    mutex_->Lock();
+  }
+  void Unlock() const {
+    mutex_->Unlock();
+#if defined(DEBUG)
+    if (no_safepoint_scope_) {
+      Thread::Current()->DecrementNoSafepointScopeDepth();
+    }
+#endif
   }
 
  private:
   Mutex* const mutex_;
+  bool no_safepoint_scope_;
 
   DISALLOW_COPY_AND_ASSIGN(MutexLocker);
 };
 
-
+/*
+ * Normal monitor locker :
+ * This locker abstraction should only be used when the enclosing code will

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

ditto?

[siva, 2016/03/01 18:59:53]

Done.

+ * not trigger a safepoint. In debug mode this class increments the
+ * no_safepoint_scope_depth variable for the current thread when the lock is
+ * taken and decrements it when the lock is released. NOTE: please do not use
+ * the passed in mutex object independent of the locker class, For example the
+ * code below will not assert correctly:
+ *    {
+ *      MonitorLocker ml(m);
+ *      ....
+ *      m->Exit();
+ *      ....
+ *      m->Enter();
+ *      ...
+ *    }
+ * Always use the locker object even when the lock needs to be released
+ * temporarily, e.g:
+ *    {
+ *      MonitorLocker ml(m);
+ *      ....
+ *      ml.Exit();
+ *      ....
+ *      ml.Enter();
+ *      ...
+ *    }
+ */
 class MonitorLocker : public ValueObject {
  public:
-  explicit MonitorLocker(Monitor* monitor) : monitor_(monitor) {
+  explicit MonitorLocker(Monitor* monitor, bool no_safepoint_scope = true)
+      : monitor_(monitor), no_safepoint_scope_(no_safepoint_scope) {
     ASSERT(monitor != NULL);
-    // TODO(iposva): Consider adding a no GC scope here.
+#if defined(DEBUG)
+    if (no_safepoint_scope_) {
+      Thread* thread = Thread::Current();
+      if (thread != NULL) {
+        thread->IncrementNoSafepointScopeDepth();
+      } else {
+        no_safepoint_scope_ = false;
+      }
+    }
+#endif
     monitor_->Enter();
   }
 
   virtual ~MonitorLocker() {
     monitor_->Exit();
-    // TODO(iposva): Consider decrementing the no GC scope here.
+#if defined(DEBUG)

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

here and elsewhere: 

DEBUG_ONLY(
)

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

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.

+    if (no_safepoint_scope_) {
+      Thread::Current()->DecrementNoSafepointScopeDepth();
+    }
+#endif
+  }
+
+  void Enter() const {
+#if defined(DEBUG)
+    if (no_safepoint_scope_) {
+      Thread::Current()->IncrementNoSafepointScopeDepth();
+    }
+#endif
+    monitor_->Enter();
+  }
+  void Exit() const {
+    monitor_->Exit();
+#if defined(DEBUG)
+    if (no_safepoint_scope_) {
+      Thread::Current()->DecrementNoSafepointScopeDepth();
+    }
+#endif
   }
 
   Monitor::WaitResult Wait(int64_t millis = Monitor::kNoTimeout) {
@@ -67,16 +183,29 @@ class MonitorLocker : public ValueObject {
 
  private:
   Monitor* const monitor_;
+  bool no_safepoint_scope_;
 
   DISALLOW_COPY_AND_ASSIGN(MonitorLocker);
 };
 
-
-// SafepointMutexLocker objects are used in code where the locks are
-// more coarse grained and a safepoint operation could be potentially
-// triggered while holding this lock. This ensures that other threads
-// which try to acquire the same lock will be marked as being at a
-// safepoint when they are blocked.
+/*
+ * Safepoint mutex locker :
+ * This locker abstraction should be used when the lock is more coarse

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

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.

[siva, 2016/03/01 18:59:53]

Changed the comment to remove the coarse wording.

+ * grained and the enclosing code could potentially trigger a safepoint.
+ * This locker ensures that other threads that try to acquire the same lock
+ * will be marked as being at a safepoint if they get blocked trying to
+ * acquire the lock.
+ * NOTE: please do not use the passed in mutex object independent of the locker
+ * class, For example the code below will not work correctly:
+ *    {
+ *      SafepointMutexLocker ml(m);
+ *      ....
+ *      m->Exit();
+ *      ....
+ *      m->Enter();
+ *      ...
+ *    }
+ */
 class SafepointMutexLocker : public ValueObject {
  public:
   explicit SafepointMutexLocker(Mutex* mutex);
@@ -90,6 +219,39 @@ class SafepointMutexLocker : public ValueObject {
   DISALLOW_COPY_AND_ASSIGN(SafepointMutexLocker);
 };
 
+/*
+ * Safepoint monitor locker :
+ * This locker abstraction should be used when the lock is more coarse
+ * grained and the enclosing code could potentially trigger a safepoint.
+ * This locker ensures that other threads that try to acquire the same lock
+ * will be marked as being at a safepoint if they get blocked trying to
+ * acquire the lock.
+ * NOTE: please do not use the passed in monitor object independent of the locker

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

line length

[siva, 2016/03/01 18:59:53]

Done.

+ * class, For example the code below will not work correctly:
+ *    {
+ *      SafepointMonitorLocker ml(m);
+ *      ....
+ *      m->Exit();
+ *      ....
+ *      m->Enter();
+ *      ...
+ *    }
+ */
+class SafepointMonitorLocker : public ValueObject {
+ public:
+  explicit SafepointMonitorLocker(Monitor* monitor);
+  virtual ~SafepointMonitorLocker() {
+    monitor_->Exit();
+  }
+
+  Monitor::WaitResult Wait(int64_t millis = Monitor::kNoTimeout);
+
+ private:
+  Monitor* const monitor_;
+
+  DISALLOW_COPY_AND_ASSIGN(SafepointMonitorLocker);
+};
+
 }  // namespace dart