Re: Patch [ecj]: Refactor Parking Code + Win32 Implementation

[Mohan Embar <gnustuff at thisiscool dot com>]

Hi Andrew,

> > Thanks again for reviewing this. If you want me to put the mutex
> > locking and unlocking inside the if statement and rerun my test on
> > FC5, let me know.
>
>That would be interesting.  If it works, feel free to commit.

I committed this slightly modified version, which passes the
simple test I emailed earlier. It moves the locking and unlocking
to inside the if statement (and removes an extra semicolon in one
of the Win32 files).

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/

2006-12-08  Mohan Embar  <gnustuff@thisiscool.com>

	* posix-threads.cc (_Jv_ThreadUnpark): Removed.
	(ParkHelper::unpark): Ported from _Jv_ThreadUnpark.
	(ParkHelper::deactivate): Implemented.
	(_Jv_ThreadPark): Removed.
	(ParkHelper::park): Ported from _Jv_ThreadPark; moved
	mutex locking and unlocking to inside if statement.
	* win32-threads.cc (compare_and_exchange): New helper function.
	(_Jv_ThreadUnpark, _Jv_ThreadPark): Removed.
	(ParkHelper::init): Implemented.
	(ParkHelper::init_event): Implemented.
	(ParkHelper::deactivate): Implemented.
	(ParkHelper::destroy): Implemented.
	(ParkHelper::unpark): Implemented.
	(ParkHelper::park): Implemented.
	* java/lang/natThread.cc (initialize_native): Use ParkHelper
	instead of POSIX synchronization constructs.
	(finalize_native):  Likewise.
	(interrupt): Use ParkHelper method instead of _Jv_ThreadUnpark().
	(finish_): Use ParkHelper::deactivate().
	* include/jvm.h (struct natThread): Use ParkHelper instead of POSIX
	synchronization constructs.
	* include/posix-threads.h: Include sysdep/locks.h
	(_Jv_ThreadUnpark, _Jv_ThreadPark): Removed.
	(ParkHelper): New struct.
	(ParkHelper::init): Implemented.
	(ParkHelper::destroy): Implemented.
	* include/win32-threads.h (ParkHelper): New struct.
	(TEXT): undefined this macro.
	* sun/misc/natUnsafe.cc (unpark): Use ParkHelper instead of
	_Jv_ThreadUnpark.
	(park):  Use ParkHelper instead of _Jv_ThreadPark.

Index: posix-threads.cc
===================================================================
--- posix-threads.cc	(revision 119567)
+++ posix-threads.cc	(working copy)
@@ -347,13 +347,11 @@
  *
  * @param thread the thread to unblock.
  */
-
 void
-_Jv_ThreadUnpark (::java::lang::Thread *thread)
+ParkHelper::unpark ()
 {
   using namespace ::java::lang;
-  natThread *nt = (natThread *) thread->data;
-  volatile obj_addr_t *ptr = &nt->park_permit;
+  volatile obj_addr_t *ptr = &permit;
 
   /* If this thread is in state RUNNING, give it a permit and return
      immediately.  */
@@ -366,13 +364,22 @@
   if (compare_and_swap 
       (ptr, Thread::THREAD_PARK_PARKED, Thread::THREAD_PARK_RUNNING))
     {
-      pthread_mutex_lock (&nt->park_mutex);
-      pthread_cond_signal (&nt->park_cond);
-      pthread_mutex_unlock (&nt->park_mutex);
+      pthread_mutex_lock (&mutex);
+      pthread_cond_signal (&cond);
+      pthread_mutex_unlock (&mutex);
     }
 }
 
 /**
+ * Sets our state to dead.
+ */
+void
+ParkHelper::deactivate ()
+{
+  permit = ::java::lang::Thread::THREAD_PARK_DEAD;
+}
+
+/**
  * Blocks the thread until a matching _Jv_ThreadUnpark() occurs, the
  * thread is interrupted or the optional timeout expires.  If an
  * unpark call has already occurred, this also counts.  A timeout
@@ -387,14 +394,11 @@
  * @param time either the number of nanoseconds to wait, or a time in
  *             milliseconds from the epoch to wait for.
  */
-
 void
-_Jv_ThreadPark (jboolean isAbsolute, jlong time)
+ParkHelper::park (jboolean isAbsolute, jlong time)
 {
   using namespace ::java::lang;
-  Thread *thread = Thread::currentThread();
-  natThread *nt = (natThread *) thread->data;
-  volatile obj_addr_t *ptr = &nt->park_permit;
+  volatile obj_addr_t *ptr = &permit;
 
   /* If we have a permit, return immediately.  */
   if (compare_and_swap 
@@ -444,22 +448,22 @@
 	}
     }
       
-  pthread_mutex_lock (&nt->park_mutex);
   if (compare_and_swap 
       (ptr, Thread::THREAD_PARK_RUNNING, Thread::THREAD_PARK_PARKED))
     {
+      pthread_mutex_lock (&mutex);
       if (millis == 0 && nanos == 0)
-	pthread_cond_wait (&nt->park_cond, &nt->park_mutex);
+	pthread_cond_wait (&cond, &mutex);
       else
-	pthread_cond_timedwait (&nt->park_cond, &nt->park_mutex, 
-					&ts);
+	pthread_cond_timedwait (&cond, &mutex, &ts);
+      pthread_mutex_unlock (&mutex);
+      
       /* If we were unparked by some other thread, this will already
 	 be in state THREAD_PARK_RUNNING.  If we timed out, we have to
 	 do it ourself.  */
       compare_and_swap 
 	(ptr, Thread::THREAD_PARK_PARKED, Thread::THREAD_PARK_RUNNING);
     }
-  pthread_mutex_unlock (&nt->park_mutex);
 }
 
 static void
Index: win32-threads.cc
===================================================================
--- win32-threads.cc	(revision 119567)
+++ win32-threads.cc	(working copy)
@@ -61,6 +61,16 @@
 #define FLAG_DAEMON  0x02
 
 //
+// Helper
+//
+inline bool
+compare_and_exchange(LONG volatile* dest, LONG cmp, LONG xchg)
+{
+  return InterlockedCompareExchange((LONG*) dest, xchg, cmp) == cmp;
+    // Seems like a bug in the MinGW headers that we have to do this cast.
+}
+
+//
 // Condition variables.
 //
 
@@ -420,6 +430,42 @@
   LeaveCriticalSection (&data->interrupt_mutex);
 }
 
+// park() / unpark() support
+
+void
+ParkHelper::init ()
+{
+  // We initialize our critical section, but not our event.
+  InitializeCriticalSection (&cs);
+  event = NULL;
+}
+
+void
+ParkHelper::init_event()
+{
+  EnterCriticalSection (&cs);
+  if (!event)
+    {
+      // Create an auto-reset event.
+      event = CreateEvent(NULL, 0, 0, NULL);
+      if (!event) JvFail("CreateEvent() failed");
+    }
+  LeaveCriticalSection (&cs);
+}
+
+void
+ParkHelper::deactivate ()
+{
+  permit = ::java::lang::Thread::THREAD_PARK_DEAD;
+}
+
+void
+ParkHelper::destroy()
+{
+  if (event) CloseHandle (event);
+  DeleteCriticalSection (&cs);
+}
+
 /**
  * Releases the block on a thread created by _Jv_ThreadPark().  This
  * method can also be used to terminate a blockage caused by a prior
@@ -428,11 +474,26 @@
  *
  * @param thread the thread to unblock.
  */
-
 void
-_Jv_ThreadUnpark (::java::lang::Thread *thread)
+ParkHelper::unpark ()
 {
-  // WRITEME:
+  using namespace ::java::lang;
+  LONG volatile* ptr = &permit;
+
+  // If this thread is in state RUNNING, give it a permit and return
+  // immediately.
+  if (compare_and_exchange
+      (ptr, Thread::THREAD_PARK_RUNNING, Thread::THREAD_PARK_PERMIT))
+    return;
+  
+  // If this thread is parked, put it into state RUNNING and send it a
+  // signal.
+  if (compare_and_exchange 
+      (ptr, Thread::THREAD_PARK_PARKED, Thread::THREAD_PARK_RUNNING))
+    {
+      init_event ();
+      SetEvent (event);
+    }
 }
 
 /**
@@ -450,9 +511,57 @@
  * @param time either the number of nanoseconds to wait, or a time in
  *             milliseconds from the epoch to wait for.
  */
-
 void
-_Jv_ThreadPark (jboolean isAbsolute, jlong time)
+ParkHelper::park (jboolean isAbsolute, jlong time)
 {
-  // WRITEME:
+  using namespace ::java::lang;
+  LONG volatile* ptr = &permit;
+
+  // If we have a permit, return immediately.
+  if (compare_and_exchange 
+      (ptr, Thread::THREAD_PARK_PERMIT, Thread::THREAD_PARK_RUNNING))
+    return;
+
+  // Determine the number of milliseconds to wait.
+  jlong millis = 0, nanos = 0;
+  
+  if (time)
+    {
+      if (isAbsolute)
+	{
+	  millis = time - ::java::lang::System::currentTimeMillis();
+	  nanos = 0;
+	}
+      else
+	{
+	  millis = 0;
+	  nanos = time;
+	}
+	
+      if (nanos)
+        {
+          millis += nanos / 1000000;
+          if (millis == 0)
+            millis = 1;
+            // ...otherwise, we'll block indefinitely.
+        }
+    }
+    
+  if (millis < 0) return;
+      // Can this ever happen?
+      
+  if (compare_and_exchange 
+      (ptr, Thread::THREAD_PARK_RUNNING, Thread::THREAD_PARK_PARKED))
+    {
+      init_event();
+      
+      DWORD timeout = millis==0 ? INFINITE : (DWORD) millis;
+      WaitForSingleObject (event, timeout);
+      
+      // If we were unparked by some other thread, this will already
+      // be in state THREAD_PARK_RUNNING.  If we timed out, we have to
+      // do it ourself.
+      compare_and_exchange 
+	(ptr, Thread::THREAD_PARK_PARKED, Thread::THREAD_PARK_RUNNING);
+    }
 }
Index: java/lang/natThread.cc
===================================================================
--- java/lang/natThread.cc	(revision 119567)
+++ java/lang/natThread.cc	(working copy)
@@ -54,8 +54,7 @@
   _Jv_MutexInit (&nt->join_mutex);
   _Jv_CondInit (&nt->join_cond);
 
-  pthread_mutex_init (&nt->park_mutex, NULL);
-  pthread_cond_init (&nt->park_cond, NULL);
+  nt->park_helper.init();
 
   nt->thread = _Jv_ThreadInitData (this);
   // FIXME: if JNI_ENV is set we will want to free it.  It is
@@ -75,9 +74,8 @@
   _Jv_MutexDestroy (&nt->join_mutex);
 #endif
   _Jv_FreeJNIEnv((JNIEnv*)nt->jni_env);
-
-  pthread_mutex_destroy (&nt->park_mutex);
-  pthread_cond_destroy (&nt->park_cond);
+  
+  nt->park_helper.destroy();
 }
 
 jint
@@ -131,7 +129,7 @@
 
       // Even though we've interrupted this thread, it might still be
       // parked.
-      _Jv_ThreadUnpark (this);
+      nt->park_helper.unpark ();
     }
 }
 
@@ -214,7 +212,7 @@
   __sync_synchronize();
   natThread *nt = (natThread *) data;
   
-  nt->park_permit = THREAD_PARK_DEAD;
+  nt->park_helper.deactivate ();
   group->removeThread (this);
 
 #ifdef ENABLE_JVMPI  
Index: include/jvm.h
===================================================================
--- include/jvm.h	(revision 119567)
+++ include/jvm.h	(working copy)
@@ -774,9 +774,7 @@
   _Jv_ConditionVariable_t join_cond;
 
   // These are used by Unsafe.park() and Unsafe.unpark().
-  volatile obj_addr_t park_permit;
-  pthread_mutex_t park_mutex;
-  pthread_cond_t park_cond;
+  ParkHelper park_helper;
 
   // This is private data for the thread system layer.
   _Jv_Thread_t *thread;
Index: include/posix-threads.h
===================================================================
--- include/posix-threads.h	(revision 119567)
+++ include/posix-threads.h	(working copy)
@@ -19,6 +19,7 @@
 
 #include <pthread.h>
 #include <sched.h>
+#include <sysdep/locks.h>
 
 //
 // Typedefs.
@@ -350,7 +351,33 @@
 
 void _Jv_ThreadInterrupt (_Jv_Thread_t *data);
 
-void _Jv_ThreadUnpark (::java::lang::Thread *thread);
-void _Jv_ThreadPark (jboolean isAbsolute, jlong time);
+// park() / unpark() support
 
+struct ParkHelper
+{
+  volatile obj_addr_t permit;
+  pthread_mutex_t mutex;
+  pthread_cond_t cond;
+  
+  void init ();
+  void deactivate ();
+  void destroy ();
+  void park (jboolean isAbsolute, jlong time);
+  void unpark ();
+};
+
+inline void
+ParkHelper::init ()
+{
+  pthread_mutex_init (&mutex, NULL);
+  pthread_cond_init (&cond, NULL);
+}
+
+inline void
+ParkHelper::destroy ()
+{
+  pthread_mutex_destroy (&mutex);
+  pthread_cond_destroy (&cond);
+}
+
 #endif /* __JV_POSIX_THREADS__ */
Index: include/win32-threads.h
===================================================================
--- include/win32-threads.h	(revision 119567)
+++ include/win32-threads.h	(working copy)
@@ -193,6 +193,28 @@
 // See java/lang/natWin32Process.cc (waitFor) for an example.
 HANDLE _Jv_Win32GetInterruptEvent (void);
 
+// park() / unpark() support
+
+struct ParkHelper
+{
+  // We use LONG instead of obj_addr_t to avoid pulling in locks.h,
+  // which depends on size_t, ...
+  volatile LONG permit;
+
+  // The critical section is used for lazy initialization of our event
+  CRITICAL_SECTION cs;
+  HANDLE event;
+  
+  void init ();
+  void deactivate ();
+  void destroy ();
+  void park (jboolean isAbsolute, jlong time);
+  void unpark ();
+  
+private:
+  void init_event();
+};
+
 // Remove defines from <windows.h> that conflict with various things in libgcj code
 
 #undef TRUE
@@ -204,5 +226,6 @@
 #undef interface
 #undef STRICT
 #undef VOID
+#undef TEXT
 
 #endif /* __JV_WIN32_THREADS__ */
Index: sun/misc/natUnsafe.cc
===================================================================
--- sun/misc/natUnsafe.cc	(revision 119567)
+++ sun/misc/natUnsafe.cc	(working copy)
@@ -238,11 +238,15 @@
 void
 sun::misc::Unsafe::unpark (::java::lang::Thread *thread)
 {
-  _Jv_ThreadUnpark (thread);
+  natThread *nt = (natThread *) thread->data;
+  nt->park_helper.unpark ();
 }
 
 void
 sun::misc::Unsafe::park (jboolean isAbsolute, jlong time)
 {
-  _Jv_ThreadPark (isAbsolute, time);
+  using namespace ::java::lang;
+  Thread *thread = Thread::currentThread();
+  natThread *nt = (natThread *) thread->data;
+  nt->park_helper.park (isAbsolute, time);
 }