RE: patch: hash synchronization for mingw/cygwin

["Boehm, Hans" <hans_boehm at hp dot com>]

I'm not sure whether I can approve this, but it looks fine to me.  I have two minor suggestions:

1) I would add a comment near the first use of JV_LINKER_CANNOT_8BYTE_ALIGN_STATICS that this only addresses misalignment of statics, not heap objects.  And it works for statics only because registering it for finalization is a noop, no matter what the least significant bits are.  I think the code is correct, but the reasons are more subtle than I would like to see without documentation.

2) If you haven't done so, it would be good to verify that GetCurrentThreadId is not a bottleneck.  If it is, you may want to clone something like the SLOW_PTHREAD_SELF code from posix-threads.h.

Hans

> -----Original Message-----
> From: Adam Megacz [mailto:adam@megacz.com]
> Sent: Friday, October 11, 2002 6:10 PM
> To: java@gcc.gnu.org
> Subject: patch: hash synchronization for mingw/cygwin
> 
> 
> 
> This patch enables hash synchronization on mingw/cygwin.  The
> JV_LINKER_CANNOT_8BYTE_ALIGN_STATICS hack to work around the broken
> Win32 linker has been discussed before:
> 
>   http://gcc.gnu.org/ml/java/2002-03/msg00038.html
> 
> Ok to commit?
> 
>   - a
> 
> 2002-10-11  Adam Megacz <adam@xwt.org>
> 
>         * java/lang/natObject.cc (_Jv_MonitorEnter, _Jv_MonitorExit,
>         heavy_lock_obj_finalization_proc, wait, notify, notifyAll):
>         removed some posix-isms, added code to clear bottom 
> three bits if
>         platform has a broken linker.
>         * include/win32-threads.h (_Jv_ThreadId_t): added.
>         * configure.in: add 
> JV_LINKER_CANNOT_8BYTE_ALIGN_STATICS, permit
>         -fhash-sync on non-posix OSes.
> 
> Index: include/win32-threads.h
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/include/win32-threads.h,v
> retrieving revision 1.5
> diff -u -r1.5 win32-threads.h
> --- include/win32-threads.h	2 Feb 2002 04:27:34 -0000	1.5
> +++ include/win32-threads.h	12 Oct 2002 01:08:51 -0000
> @@ -33,6 +33,14 @@
>    java::lang::Thread *thread_obj;
>  } _Jv_Thread_t;
>  
> +typedef DWORD _Jv_ThreadId_t;
> +
> +inline _Jv_ThreadId_t
> +_Jv_ThreadSelf (void)
> +{
> +  return GetCurrentThreadId();
> +}
> +
>  typedef void _Jv_ThreadStartFunc (java::lang::Thread *);
>  
>  //
> Index: java/lang/natObject.cc
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/java/lang/natObject.cc,v
> retrieving revision 1.22
> diff -u -r1.22 natObject.cc
> --- java/lang/natObject.cc	10 Mar 2002 03:53:13 -0000	1.22
> +++ java/lang/natObject.cc	12 Oct 2002 01:09:00 -0000
> @@ -305,9 +305,9 @@
>  #include <assert.h>
>  #include <limits.h>
>  #include <unistd.h>	// for usleep, sysconf.
> -#include <sched.h>	// for sched_yield.
>  #include <gcj/javaprims.h>
>  #include <sysdep/locks.h>
> +#include <java/lang/Thread.h>
>  
>  // Try to determine whether we are on a multiprocessor, i.e. whether
>  // spinning may be profitable.
> @@ -525,14 +525,14 @@
>      }
>    else if (n < yield_limit)
>      {
> -      sched_yield();
> +      _Jv_ThreadYield();
>      }
>    else
>      {
>        unsigned duration = MIN_SLEEP_USECS << (n - yield_limit);
>        if (n >= 15 + yield_limit || duration > MAX_SLEEP_USECS)
> -	duration = MAX_SLEEP_USECS;
> -      usleep(duration);
> +        duration = MAX_SLEEP_USECS;
> +      java::lang::Thread::sleep(0, duration);
>      }
>  }
>  
> @@ -574,7 +574,11 @@
>  heavy_lock_obj_finalization_proc (void *obj, void *cd)
>  {
>    heavy_lock *hl = (heavy_lock *)cd;
> +#ifdef JV_LINKER_CANNOT_8BYTE_ALIGN_STATICS
> +  obj_addr_t addr = (obj_addr_t)obj & ~((obj_addr_t)0x7);
> +#else
>    obj_addr_t addr = (obj_addr_t)obj;
> +#endif
>    hash_entry *he = light_locks + JV_SYNC_HASH(addr);
>    obj_addr_t he_address = (he -> address & ~LOCKED);
>  
> @@ -753,7 +757,11 @@
>  void
>  _Jv_MonitorEnter (jobject obj)
>  {
> +#ifdef JV_LINKER_CANNOT_8BYTE_ALIGN_STATICS
> +  obj_addr_t addr = (obj_addr_t)obj & ~((obj_addr_t)FLAGS);
> +#else
>    obj_addr_t addr = (obj_addr_t)obj;
> +#endif
>    obj_addr_t address;
>    unsigned hash = JV_SYNC_HASH(addr);
>    hash_entry * he = light_locks + hash;
> @@ -898,7 +906,11 @@
>  void
>  _Jv_MonitorExit (jobject obj)
>  {
> +#ifdef JV_LINKER_CANNOT_8BYTE_ALIGN_STATICS
> +  obj_addr_t addr = (obj_addr_t)obj & ~((obj_addr_t)FLAGS);
> +#else
>    obj_addr_t addr = (obj_addr_t)obj;
> +#endif
>    _Jv_ThreadId_t self = _Jv_ThreadSelf();
>    unsigned hash = JV_SYNC_HASH(addr);
>    hash_entry * he = light_locks + hash;
> @@ -1078,7 +1090,11 @@
>  void
>  java::lang::Object::wait (jlong timeout, jint nanos)
>  {
> +#ifdef JV_LINKER_CANNOT_8BYTE_ALIGN_STATICS
> +  obj_addr_t addr = (obj_addr_t)this & ~((obj_addr_t)FLAGS);
> +#else
>    obj_addr_t addr = (obj_addr_t)this;
> +#endif
>    _Jv_ThreadId_t self = _Jv_ThreadSelf();
>    unsigned hash = JV_SYNC_HASH(addr);
>    hash_entry * he = light_locks + hash;
> @@ -1155,7 +1171,11 @@
>  void
>  java::lang::Object::notify (void)
>  {
> +#ifdef JV_LINKER_CANNOT_8BYTE_ALIGN_STATICS
> +  obj_addr_t addr = (obj_addr_t)this & ~((obj_addr_t)FLAGS);
> +#else
>    obj_addr_t addr = (obj_addr_t)this;
> +#endif
>    _Jv_ThreadId_t self = _Jv_ThreadSelf();
>    unsigned hash = JV_SYNC_HASH(addr);
>    hash_entry * he = light_locks + hash;
> @@ -1200,7 +1220,11 @@
>  void
>  java::lang::Object::notifyAll (void)
>  {
> +#ifdef JV_LINKER_CANNOT_8BYTE_ALIGN_STATICS
> +  obj_addr_t addr = (obj_addr_t)this & ~((obj_addr_t)FLAGS);
> +#else
>    obj_addr_t addr = (obj_addr_t)this;
> +#endif
>    _Jv_ThreadId_t self = _Jv_ThreadSelf();
>    unsigned hash = JV_SYNC_HASH(addr);
>    hash_entry * he = light_locks + hash;
> Index: configure.in
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/configure.in,v
> retrieving revision 1.140
> diff -u -r1.140 configure.in
> --- configure.in	17 Sep 2002 06:58:39 -0000	1.140
> +++ configure.in	12 Oct 2002 01:09:02 -0000
> @@ -226,6 +226,8 @@
>              PLATFORM=Win32
>              PLATFORMOBJS=win32.lo
>  	    PLATFORMH=win32.h
> +        AC_DEFINE(JV_LINKER_CANNOT_8BYTE_ALIGN_STATICS, 1,
> +                  [Indicate that linker is not able to 
> 8-byte align static data])
>        ;;
>        *)
>              PLATFORM=Posix
> @@ -444,7 +446,7 @@
>  
>  HASH_SYNC_SPEC=
>  # Hash synchronization is only useful with posix threads right now.
> -if test "$enable_hash_synchronization" = yes && test 
> "$THREADS" = "posix"; then
> +if test "$enable_hash_synchronization" = yes; then
>     HASH_SYNC_SPEC=-fhash-synchronization
>     AC_DEFINE(JV_HASH_SYNCHRONIZATION, 1, [Define if hash 
> synchronization is in use])
>  fi
> 
> 
> -- 
> "Through your rags I see your vanity"  -- Socrates
>