JDK-6636370 : minor corrections and simplification of code in AppContext
  • Type: Bug
  • Component: client-libs
  • Sub-Component: java.awt
  • Affected Version: 7
  • Priority: P4
  • Status: Closed
  • Resolution: Fixed
  • OS: linux
  • CPU: x86
  • Submitted: 2007-12-01
  • Updated: 2011-05-18
  • Resolved: 2011-05-18
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 7
7 b25Fixed
Description
Working on AppContext code I've noticed that:

1. mainAppContext, isDisposed, and numAppContexts should be made volatile.
2. mostRecentThreadAppContext should be rewritten using ThreadLocal to simplify the code.

Comments
SUGGESTED FIX src/share/classes/sun/awt/AppContext.java Print this page @@ -147,11 +147,11 @@ } /* The main "system" AppContext, used by everything not otherwise contained in another AppContext. */ - private static AppContext mainAppContext = null; + private static volatile AppContext mainAppContext = null; /* * The hash map associated with this AppContext. A private delegate * is used instead of subclassing HashMap so as to avoid all of * HashMap's potentially risky methods, such as clear(), elements(), @@ -172,17 +172,16 @@ private PropertyChangeSupport changeSupport = null; public static final String DISPOSED_PROPERTY_NAME = "disposed"; public static final String GUI_DISPOSED = "guidisposed"; - private boolean isDisposed = false; // true if AppContext is disposed + private volatile boolean isDisposed = false; // true if AppContext is disposed public boolean isDisposed() { return isDisposed; } - static { // On the main Thread, we get the ThreadGroup, make a corresponding // AppContext, and instantiate the Java EventQueue. This way, legacy // code is unaffected by the move to multiple AppContext ability. AccessController.doPrivileged(new PrivilegedAction() { @@ -207,11 +206,11 @@ * incremented at the beginning of the constructor, and decremented * at the end of dispose(). getAppContext() checks to see if this * number is 1. If so, it returns the sole AppContext without * checking Thread.currentThread(). */ - private static int numAppContexts; + private static volatile int numAppContexts; /* * The context ClassLoader that was used to create this AppContext. */ private final ClassLoader contextClassLoader; @@ -234,18 +233,19 @@ this.threadGroup = threadGroup; threadGroup2appContext.put(threadGroup, this); this.contextClassLoader = - (ClassLoader) AccessController.doPrivileged(new PrivilegedAction() { - public Object run() { + AccessController.doPrivileged(new PrivilegedAction<ClassLoader>() { + public ClassLoader run() { return Thread.currentThread().getContextClassLoader(); } }); } - private static MostRecentThreadAppContext mostRecentThreadAppContext = null; + private static final ThreadLocal<AppContext> threadAppContext = + new ThreadLocal<AppContext>(); /** * Returns the appropriate AppContext for the caller, * as determined by its ThreadGroup. If the main "system" AppContext * would be returned and there's an AWTSecurityManager installed, it @@ -258,33 +258,21 @@ */ public final static AppContext getAppContext() { if (numAppContexts == 1) // If there's only one system-wide, return mainAppContext; // return the main system AppContext. - final Thread currentThread = Thread.currentThread(); - - AppContext appContext = null; + AppContext appContext = threadAppContext.get(); - // Note: this most recent Thread/AppContext caching is thread-hot. - // A simple test using SwingSet found that 96.8% of lookups - // were matched using the most recent Thread/AppContext. By - // instantiating a simple MostRecentThreadAppContext object on - // cache misses, the cache hits can be processed without - // synchronization. - - MostRecentThreadAppContext recent = mostRecentThreadAppContext; - if ((recent != null) && (recent.thread == currentThread)) { - appContext = recent.appContext; // Cache hit - } else { - appContext = (AppContext)AccessController.doPrivileged( - new PrivilegedAction() { - public Object run() { + if (null == appContext) { + appContext = AccessController.doPrivileged(new PrivilegedAction<AppContext>() + { + public AppContext run() { // Get the current ThreadGroup, and look for it and its // parents in the hash from ThreadGroup to AppContext -- // it should be found, because we use createNewContext() // when new AppContext objects are created. - ThreadGroup currentThreadGroup = currentThread.getThreadGroup(); + ThreadGroup currentThreadGroup = Thread.currentThread().getThreadGroup(); ThreadGroup threadGroup = currentThreadGroup; AppContext context = threadGroup2appContext.get(threadGroup); while (context == null) { threadGroup = threadGroup.getParent(); if (threadGroup == null) { @@ -305,12 +293,11 @@ } // Now we're done, so we cache the latest key/value pair. // (we do this before checking with any AWTSecurityManager, so if // this Thread equates with the main AppContext in the cache, it // still will) - mostRecentThreadAppContext = - new MostRecentThreadAppContext(currentThread, context); + threadAppContext.set(context); return context; } }); } @@ -319,13 +306,13 @@ // Before we return the main "system" AppContext, check to // see if there's an AWTSecurityManager installed. If so, // allow it to choose the AppContext to return. SecurityManager securityManager = System.getSecurityManager(); if ((securityManager != null) && - (securityManager instanceof AWTSecurityManager)) { - AWTSecurityManager awtSecMgr = - (AWTSecurityManager)securityManager; + (securityManager instanceof AWTSecurityManager)) + { + AWTSecurityManager awtSecMgr = (AWTSecurityManager)securityManager; AppContext secAppContext = awtSecMgr.getAppContext(); if (secAppContext != null) { appContext = secAppContext; // Return what we're told } } @@ -442,11 +429,11 @@ // Next, we sleep 10ms at a time, waiting for all of the active // Threads in the ThreadGroup to exit. long startTime = System.currentTimeMillis(); - long endTime = startTime + (long)THREAD_INTERRUPT_TIMEOUT; + long endTime = startTime + THREAD_INTERRUPT_TIMEOUT; while ((this.threadGroup.activeCount() > 0) && (System.currentTimeMillis() < endTime)) { try { Thread.sleep(10); } catch (InterruptedException e) { } @@ -457,11 +444,11 @@ // Next, we sleep 10ms at a time, waiting for all of the active // Threads in the ThreadGroup to die. startTime = System.currentTimeMillis(); - endTime = startTime + (long)THREAD_INTERRUPT_TIMEOUT; + endTime = startTime + THREAD_INTERRUPT_TIMEOUT; while ((this.threadGroup.activeCount() > 0) && (System.currentTimeMillis() < endTime)) { try { Thread.sleep(10); } catch (InterruptedException e) { } @@ -476,14 +463,11 @@ threadGroup2appContext.remove(subGroups[subGroup]); } } threadGroup2appContext.remove(this.threadGroup); - MostRecentThreadAppContext recent = mostRecentThreadAppContext; - if ((recent != null) && (recent.appContext == this)) - mostRecentThreadAppContext = null; - // If the "most recent" points to this, clear it for GC + threadAppContext.set(null); // Finally, we destroy the ThreadGroup entirely. try { this.threadGroup.destroy(); } catch (IllegalThreadStateException e) { @@ -662,10 +646,11 @@ /** * Returns a string representation of this AppContext. * @since 1.2 */ + @Override public String toString() { return getClass().getName() + "[threadGroup=" + threadGroup.getName() + "]"; } /** @@ -767,19 +752,10 @@ } return changeSupport.getPropertyChangeListeners(propertyName); } } -final class MostRecentThreadAppContext { - final Thread thread; - final AppContext appContext; - MostRecentThreadAppContext(Thread key, AppContext value) { - thread = key; - appContext = value; - } -} - final class MostRecentKeyValue { Object key; Object value; MostRecentKeyValue(Object k, Object v) { key = k;
01-12-2007

EVALUATION descripion contains all necessary information
01-12-2007