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

[Andrew Haley <aph at redhat dot com>]

Mohan Embar writes:
 > 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?

I have no idea.  Perhaps at this point my best defence would be to
plead insanity...

 > 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

I was waiting for Tom Tromey to comment.

 > 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.

Looks good to me.

Andrew.