Patch [ecj]: Refactor Parking Code + Win32 Implementation

[Mohan Embar <gnustuff at thisiscool dot com>]

Hi Andrew,

Hopefully, the third time will be a charm. I removed the call
to getUnsafe() in natThread.cc, slightly changed the variable
names in POSIX ParkHelper and made a much-more-efficient
Win32 implementation (which looks a lot like the POSIX code).

By the way, out of curiosity (posix-threads.cc, park()):

======================================
  pthread_mutex_lock (&mutex);
  if (compare_and_swap 
      (ptr, Thread::THREAD_PARK_RUNNING, Thread::THREAD_PARK_PARKED))
    {
      if (millis == 0 && nanos == 0)
	pthread_cond_wait (&cond, &mutex);
      else
	pthread_cond_timedwait (&cond, &mutex, &ts);
      /* 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 (&mutex);
======================================

...why do you lock and unlock the mutex outside of the if statement
rather than within it?

Tested on FC5 and Win32. Thanks for your patience with this. If you let me commit this,
then my remaining outstanding patch for the branch is:

http://gcc.gnu.org/ml/java-patches/2006-q4/msg00221.html

-- 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.
	* 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,21 @@
 	}
     }
       
-  pthread_mutex_lock (&nt->park_mutex);
+  pthread_mutex_lock (&mutex);
   if (compare_and_swap 
       (ptr, Thread::THREAD_PARK_RUNNING, Thread::THREAD_PARK_PARKED))
     {
       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);
       /* 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);
+  pthread_mutex_unlock (&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);
 }