Re: Fwd: merging the classpath threadlocal changes

[Andrew Haley <aph at redhat dot com>]

Andrew John Hughes wrote:
> Forgot to CC this list...
> 
> 
> ---------- Forwarded message ----------
> From: Andrew John Hughes <gnu_andrew@member.fsf.org>
> Date: 2008/9/29
> Subject: Re: merging the classpath threadlocal changes
> To: Matthias Klose <doko@ubuntu.com>
> Cc: java@gcc.gnu.org
> 
> 
> On 17:07 Sun 28 Sep     , Matthias Klose wrote:
>> Andrew John Hughes schrieb:
>>> 2008/9/24 Matthias Klose <doko@ubuntu.com>:
>>>> this is a pending patch for some files which are not used in libgcj, and the
>>>> files are shadowed except for one new file. Just merging the patch does result
>>>> in a build failure, because libgcj picks up this one new file. can makemake.tcl
>>>> blacklist this, or should libgcj just get an empty file/class?
>>>>
>>>>  Matthias
>>>>
>>> What is the build failure? I assume the new ThreadLocalMap (or
>>> whatever it's called)
>>> is referring back to ThreadLocal, though the map itself is unused.
>>> Maybe gcj's version
>>> could support the needed methods without using the map itself?
>> Didn't save the build logs. Afaicr these were differences in return and
>> parameter types for some thread related classes.
> 
> Here's the patches just committed (forgot to add the new files at first...).
> The failures you were seeing were due to the ThreadLocalMap requiring a hash
> value from java.lang.Thread.  Also InheritableThreadLocal is not shadowed,
> so the changes in that were causing a build error due to a method that's
> in ThreadLocalMap but not WeakIdentityHashMap (inherit).
> 
> Although in the best case scenario GCJ uses system thread-local storage
> via POSIX threads, it still uses the Classpath map solution in a number of
> situations.  First of all, it is always used for set (the native version
> is stored first, followed by a call to internalSet(Object)). For get(),
> it will only be used if POSIX threads are unavailable on build (the
> working implementation is #ifdef on _POSIX_PTHREAD_SEMANTICS),
> initialisation fails or the value can't be retrieved.  So although the map
> is not usually used as much by gcj, the Classpath version still has a minor
> effect (and is thus not as irrelevant as it may first have seemed).
> 
> With this patch, I ported the hash value changes to GCJ's java.lang.Thread
> and updated internalGet, internalSet and internalRemove to use ThreadLocalMap
> instead.  The latter can be dropped if necessary, but it seems sensible to
> try and stay close to Classpath where possible.

Nearly right.  :-)

gcj creates its ThreadLocals dynamically, but InheritableThreadLocal uses
a nasty back-door access to Thread.locals that assumes locals has already
been created:

  static void newChildThread(Thread childThread)
  {
    // The currentThread is the parent of the new thread.
    Thread parentThread = Thread.currentThread();
    childThread.locals.inherit(parentThread.locals);
  }

My fix, herein attached, changes gcj-local code but leaves Classpath code
alone.

I would like to suggest that in the future we try to have a rule that
fields in an implementation are private, and access to them be provided
by an accessor method.  This would also allow fields to be created
lazily.

Andrew.


2008-10-06  Andrew Haley  <aph@redhat.com>

	* java/lang/Thread.java (Thread): Always create the ThreadLocalMap
	when creating a thread.
	(getThreadLocals) Don't lazily create the ThreadLocalMap.

Index: java/lang/Thread.java
===================================================================
--- java/lang/Thread.java	(revision 140904)
+++ java/lang/Thread.java	(working copy)
@@ -431,6 +431,12 @@
         this.threadId = nextThreadId++;
       }

+    // Always create the ThreadLocalMap when creating a thread; the
+    // previous code did this lazily when getThreadLocals was called,
+    // but this is a divergence from Classpath's implementation of
+    // ThreadLocal.
+    this.locals = new ThreadLocalMap();
+
     if (current != null)
       {
 	group.checkAccess();
@@ -1023,10 +1029,7 @@
   {
     Thread thread = currentThread();
     ThreadLocalMap locals = thread.locals;
-    if (locals == null)
-      {
-        locals = thread.locals = new ThreadLocalMap();
-      }
+
     return locals;
   }