I wrote: > I'd like to propose a different way to implement ThreadLocal. > The patch is attached.
I realised that my new version would "leak" the ThreadLocal objects, so I've attached a new version of the patch that uses a WeakHashMap in Thread to store the locals. I liked the previous patch better, but experimentation shows that the JDK also collects thread locals that aren't referenced anymore. Regards, Jeroen
Index: java/lang/InheritableThreadLocal.java =================================================================== RCS file: /cvsroot/classpath/classpath/java/lang/InheritableThreadLocal.java,v retrieving revision 1.7 diff -u -r1.7 InheritableThreadLocal.java --- java/lang/InheritableThreadLocal.java 9 Aug 2003 18:47:04 -0000 1.7 +++ java/lang/InheritableThreadLocal.java 17 Feb 2005 12:44:38 -0000 @@ -37,12 +37,8 @@ package java.lang; -import java.util.ArrayList; -import java.util.Collections; -import java.util.Iterator; -import java.util.List; -import java.util.Map; import java.util.WeakHashMap; +import java.util.Iterator; /** * A ThreadLocal whose value is inherited by child Threads. The value of the @@ -64,30 +60,11 @@ public class InheritableThreadLocal extends ThreadLocal { /** - * Maps Threads to a List of InheritableThreadLocals (the heritage of that - * Thread). Uses a WeakHashMap so if the Thread is garbage collected the - * List can be collected, too. Maps to a list in case the user overrides - * equals. - */ - private static final Map threadMap - = Collections.synchronizedMap(new WeakHashMap()); - - /** * Creates a new InheritableThreadLocal that has no values associated * with it yet. */ public InheritableThreadLocal() { - Thread currentThread = Thread.currentThread(); - // Note that we don't have to synchronize, as only this thread will - // ever modify the returned heritage and threadMap is a synchronizedMap. - List heritage = (List) threadMap.get(currentThread); - if (heritage == null) - { - heritage = new ArrayList(); - threadMap.put(currentThread, heritage); - } - heritage.add(this); } /** @@ -116,25 +93,22 @@ { // The currentThread is the parent of the new thread. Thread parentThread = Thread.currentThread(); - // Note that we don't have to synchronize, as only this thread will - // ever modify the returned heritage and threadMap is a synchronizedMap. - ArrayList heritage = (ArrayList) threadMap.get(parentThread); - if (heritage != null) + if (parentThread.locals != null) { - threadMap.put(childThread, heritage.clone()); - // Perform the inheritance. - Iterator it = heritage.iterator(); - int i = heritage.size(); - while (--i >= 0) + Iterator keys = parentThread.locals.keySet().iterator(); + while (keys.hasNext()) { - InheritableThreadLocal local = (InheritableThreadLocal) it.next(); - Object parentValue = local.valueMap.get(parentThread); - if (parentValue != null) + Key key = (Key)keys.next(); + if (key.get() instanceof InheritableThreadLocal) { + InheritableThreadLocal local = (InheritableThreadLocal)key.get(); + Object parentValue = parentThread.locals.get(key); Object childValue = local.childValue(parentValue == NULL ? null : parentValue); - local.valueMap.put(childThread, (childValue == null - ? NULL : parentValue)); + if (childThread.locals == null) + childThread.locals = new WeakHashMap(); + childThread.locals.put(key, (childValue == null + ? NULL : childValue)); } } } Index: java/lang/Thread.java =================================================================== RCS file: /cvsroot/classpath/classpath/java/lang/Thread.java,v retrieving revision 1.12 diff -u -r1.12 Thread.java --- java/lang/Thread.java 31 Dec 2004 21:32:17 -0000 1.12 +++ java/lang/Thread.java 17 Feb 2005 12:44:38 -0000 @@ -38,6 +38,8 @@ package java.lang; +import java.util.Map; +import java.util.WeakHashMap; /* Written using "Java Class Libraries", 2nd edition, ISBN 0-201-31002-3 * "The Java Language Specification", ISBN 0-201-63451-1 @@ -131,6 +133,11 @@ /** The next thread number to use. */ private static int numAnonymousThreadsCreated; + /** Thread local storage. Package accessible for use by + * InheritableThreadLocal. + */ + WeakHashMap locals; + /** * Allocates a new <code>Thread</code> object. This constructor has * the same effect as <code>Thread(null, null,</code> @@ -973,5 +980,20 @@ { group.removeThread(this); vmThread = null; + locals = null; + } + + /** + * Returns the map used by ThreadLocal to store the thread local values. + */ + static Map getThreadLocals() + { + Thread thread = currentThread(); + Map locals = thread.locals; + if (locals == null) + { + locals = thread.locals = new WeakHashMap(); + } + return locals; } } Index: java/lang/ThreadLocal.java =================================================================== RCS file: /cvsroot/classpath/classpath/java/lang/ThreadLocal.java,v retrieving revision 1.5 diff -u -r1.5 ThreadLocal.java --- java/lang/ThreadLocal.java 15 Oct 2004 07:44:17 -0000 1.5 +++ java/lang/ThreadLocal.java 17 Feb 2005 12:44:38 -0000 @@ -37,7 +37,6 @@ package java.lang; -import java.util.Collections; import java.util.Map; import java.util.WeakHashMap; @@ -98,18 +97,24 @@ static final Object NULL = new Object(); /** - * The stored value. Package visible for use by InheritableThreadLocal. */ - Object value; - - /** - * Maps Threads to values. Uses a WeakHashMap so if a Thread is garbage - * collected the reference to the Value will disappear. A null value means - * uninitialized, while NULL means a user-specified null. Only the - * <code>set(Thread, Object)</code> and <code>get(Thread)</code> methods - * access it. Package visible for use by InheritableThreadLocal. + * Serves as a key for the Thread.locals WeakHashMap. + * We can't use "this", because a subclass may override equals/hashCode + * and we need to use object identity for the map. */ - final Map valueMap = Collections.synchronizedMap(new WeakHashMap()); - + final Key key = new Key(); + + class Key + { + Key() + { + } + + ThreadLocal get() + { + return ThreadLocal.this; + } + } + /** * Creates a ThreadLocal object without associating any value to it yet. */ @@ -140,14 +145,14 @@ */ public Object get() { - Thread currentThread = Thread.currentThread(); + Map map = Thread.getThreadLocals(); // Note that we don't have to synchronize, as only this thread will - // ever modify the returned value and valueMap is a synchronizedMap. - Object value = valueMap.get(currentThread); + // ever modify the map. + Object value = map.get(key); if (value == null) { value = initialValue(); - valueMap.put(currentThread, value == null ? NULL : value); + map.put(key, value == null ? NULL : value); } return value == NULL ? null : value; } @@ -162,8 +167,9 @@ */ public void set(Object value) { + Map map = Thread.getThreadLocals(); // Note that we don't have to synchronize, as only this thread will - // ever modify the returned value and valueMap is a synchronizedMap. - valueMap.put(Thread.currentThread(), value == null ? NULL : value); + // ever modify the map. + map.put(key, value == null ? NULL : value); } }
_______________________________________________ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches