RE: Patch: PR libgcj/23367

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

Tom -

If I understand this correctly, the cache is only referenced by
thread-local memory?

Ideally, the collector should handle this correctly, but I don't believe
that it does so consistently across platforms.  (The problem is that it
tends to be very hard to enumerate thread local storage.)  I would add a
pointer to each of the thread local caches to some global data
structure, e.g. linked list.  That adds a bit of overhead, and a lock
acquisition to the cache initialization.  But that hopefully won't
matter much.

Hans

> -----Original Message-----
> From: java-patches-owner@gcc.gnu.org 
> [mailto:java-patches-owner@gcc.gnu.org] On Behalf Of Tom Tromey
> Sent: Monday, September 19, 2005 12:52 PM
> To: Java Patch List
> Subject: Patch: PR libgcj/23367
> 
> 
> This patch fixes PR 23367, a thread safety bug in 
> _Jv_FindMethodInCache.
> 
> It works by changing the method cache to be per-thread and to 
> be stored in TLS.  It also makes the method cache much smaller.
> 
> I tested this on a system with TLS to ensure that the method 
> cache was not prematurely collected by the GC (I did this by 
> registering a finalizer that called abort).  This seemed to work.
> 
> I'm not checking this in yet as I want to re-use some 
> configury from mudflap; a patch to move this into gcc/config/ 
> is pending.
> 
> Tom
> 
> Index: ChangeLog
> from  Tom Tromey  <tromey@redhat.com>
> 
> 	PR libgcj/23367:
> 	* java/lang/natClass.cc (MCACHE_SIZE): Conditional on HAVE_TLS.
> 	(struct _Jv_mcache): Likewise.
> 	(method_cache): Likewise.
> 	(_Jv_FindMethodInCache): Do nothing unless TLS is available.
> 	(_Jv_AddMethodToCache): Likewise.
> 	* aclocal.m4, configure, include/config.h.in: Rebuilt.
> 	* configure.ac: Invoke GCC_CHECK_TLS.
> 
> Index: configure.ac 
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/configure.ac,v
> retrieving revision 1.37
> diff -u -r1.37 configure.ac
> --- configure.ac 22 Aug 2005 22:39:11 -0000 1.37
> +++ configure.ac 19 Sep 2005 19:55:03 -0000
> @@ -1412,6 +1412,8 @@
>    multilib_arg=
>  fi
>  
> +# See if we support thread-local storage.
> +GCC_CHECK_TLS
>  
>  
>  here=`${PWDCMD-pwd}`
> Index: java/lang/natClass.cc 
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/java/lang/natClass.cc,v
> retrieving revision 1.89
> diff -u -r1.89 natClass.cc
> --- java/lang/natClass.cc 14 Jun 2005 18:51:54 -0000 1.89
> +++ java/lang/natClass.cc 19 Sep 2005 19:55:03 -0000
> @@ -879,8 +879,10 @@
>    return NULL;
>  }
>  
> +#ifdef HAVE_TLS
> +
>  // NOTE: MCACHE_SIZE should be a power of 2 minus one. 
> -#define MCACHE_SIZE 1023
> +#define MCACHE_SIZE 31
>  
>  struct _Jv_mcache
>  {
> @@ -888,37 +890,42 @@
>    _Jv_Method *method;
>  };
>  
> -static _Jv_mcache method_cache[MCACHE_SIZE + 1];
> +static __thread _Jv_mcache *method_cache;
> +#endif // HAVE_TLS
>  
>  static void *
>  _Jv_FindMethodInCache (jclass klass,
>                         _Jv_Utf8Const *name,
>                         _Jv_Utf8Const *signature)
>  {
> -  int index = name->hash16 () & MCACHE_SIZE;
> -  _Jv_mcache *mc = method_cache + index;
> -  _Jv_Method *m = mc->method;
> -
> -  if (mc->klass == klass
> -      && m != NULL             // thread safe check
> -      && _Jv_equalUtf8Consts (m->name, name)
> -      && _Jv_equalUtf8Consts (m->signature, signature))
> -    return mc->method->ncode;
> +#ifdef HAVE_TLS
> +  _Jv_mcache *cache = method_cache;
> +  if (cache)
> +    {
> +      int index = name->hash16 () & MCACHE_SIZE;
> +      _Jv_mcache *mc = &cache[index];
> +      _Jv_Method *m = mc->method;
> +
> +      if (mc->klass == klass
> +	  && _Jv_equalUtf8Consts (m->name, name)
> +	  && _Jv_equalUtf8Consts (m->signature, signature))
> +	return mc->method->ncode;
> +    }
> +#endif // HAVE_TLS
>    return NULL;
>  }
>  
>  static void
> -_Jv_AddMethodToCache (jclass klass,
> -                       _Jv_Method *method)
> +_Jv_AddMethodToCache (jclass klass, _Jv_Method *method)
>  {
> -  _Jv_MonitorEnter (&java::lang::Class::class$); 
> -
> +#ifdef HAVE_TLS
> +  if (method_cache == NULL)
> +    method_cache = (_Jv_mcache *) _Jv_AllocBytes((MCACHE_SIZE + 1)
> +						 * sizeof (_Jv_mcache));
>    int index = method->name->hash16 () & MCACHE_SIZE;
> -
>    method_cache[index].method = method;
>    method_cache[index].klass = klass;
> -
> -  _Jv_MonitorExit (&java::lang::Class::class$);
> +#endif // HAVE_TLS
>  }
>  
>  void *
>