Remove synchronization bottlenecks in Logger

[Martin Buchholz]

Hi java.util.logging maintainers,

(Will this get Serguei or Graham to come out of retirement?!  Maybe not.)

Description:
The removal of unnecessary optimization below
(replacing synchronized with j.u.c. or volatile replacements)
made a big difference in one of our local applications with
many concurrently logging threads.

As well, making levelObject volatile slightly improves the thread-safety
of Logger.

Fix written by Jeremy Manson and reviewed by Bill Pugh
(both of JSR-133 fame), and by myself.

There are further concurrency improvements that can be made to this code,
but we are not that ambitious.

# HG changeset patch
# User martin
# Date 1232071338 28800
# Node ID 7ac6b44ba1709d554282cc16b5b8b2c613bc1079
# Parent  7f6969c090750e1d389a93c3a657b60426d3d593
6666666: Remove synchronization bottlenecks in Logger
Reviewed-by: xxxxx
Contributed-by: jeremymanson at google.com

diff --git a/src/share/classes/java/util/logging/Logger.java
b/src/share/classes/java/util/logging/Logger.java
--- a/src/share/classes/java/util/logging/Logger.java
+++ b/src/share/classes/java/util/logging/Logger.java
@@ -27,6 +27,7 @@
 package java.util.logging;

 import java.util.*;
+import java.util.concurrent.CopyOnWriteArrayList;
 import java.security.*;
 import java.lang.ref.WeakReference;

@@ -165,10 +166,11 @@
     private static final int offValue = Level.OFF.intValue();
     private LogManager manager;
     private String name;
-    private ArrayList<Handler> handlers;
+    private final CopyOnWriteArrayList<Handler> handlers =
+        new CopyOnWriteArrayList<Handler>();
     private String resourceBundleName;
-    private boolean useParentHandlers = true;
-    private Filter filter;
+    private volatile boolean useParentHandlers = true;
+    private volatile Filter filter;
     private boolean anonymous;

     private ResourceBundle catalog;     // Cached resource bundle
@@ -180,9 +182,9 @@
     private static Object treeLock = new Object();
     // We keep weak references from parents to children, but strong
     // references from children to parents.
-    private Logger parent;    // our nearest parent.
+    private volatile Logger parent;    // our nearest parent.
     private ArrayList<WeakReference<Logger>> kids;   //
WeakReferences to loggers that have us as parent
-    private Level levelObject;
+    private volatile Level levelObject;
     private volatile int levelValue;  // current effective level value

     /**
@@ -438,7 +440,7 @@
      * @exception  SecurityException  if a security manager exists and if
      *             the caller does not have LoggingPermission("control").
      */
-    public synchronized void setFilter(Filter newFilter) throws
SecurityException {
+    public void setFilter(Filter newFilter) throws SecurityException {
         checkAccess();
         filter = newFilter;
     }
@@ -448,7 +450,7 @@
      *
      * @return  a filter object (may be null)
      */
-    public synchronized Filter getFilter() {
+    public Filter getFilter() {
         return filter;
     }

@@ -465,10 +467,9 @@
         if (record.getLevel().intValue() < levelValue || levelValue
== offValue) {
             return;
         }
-        synchronized (this) {
-            if (filter != null && !filter.isLoggable(record)) {
-                return;
-            }
+        Filter theFilter = filter;
+        if (theFilter != null && !theFilter.isLoggable(record)) {
+            return;
         }

         // Post the LogRecord to all our Handlers, and then to
@@ -1182,13 +1183,10 @@
      * @exception  SecurityException  if a security manager exists and if
      *             the caller does not have LoggingPermission("control").
      */
-    public synchronized void addHandler(Handler handler) throws
SecurityException {
+    public void addHandler(Handler handler) throws SecurityException {
         // Check for null handler
         handler.getClass();
         checkAccess();
-        if (handlers == null) {
-            handlers = new ArrayList<Handler>();
-        }
         handlers.add(handler);
     }

@@ -1201,12 +1199,9 @@
      * @exception  SecurityException  if a security manager exists and if
      *             the caller does not have LoggingPermission("control").
      */
-    public synchronized void removeHandler(Handler handler) throws
SecurityException {
+    public void removeHandler(Handler handler) throws SecurityException {
         checkAccess();
         if (handler == null) {
-            return;
-        }
-        if (handlers == null) {
             return;
         }
         handlers.remove(handler);
@@ -1217,11 +1212,8 @@
      * <p>
      * @return  an array of all registered Handlers
      */
-    public synchronized Handler[] getHandlers() {
-        if (handlers == null) {
-            return emptyHandlers;
-        }
-        return handlers.toArray(new Handler[handlers.size()]);
+    public Handler[] getHandlers() {
+        return handlers.toArray(emptyHandlers);
     }

     /**
@@ -1235,7 +1227,7 @@
      * @exception  SecurityException  if a security manager exists and if
      *             the caller does not have LoggingPermission("control").
      */
-    public synchronized void setUseParentHandlers(boolean useParentHandlers) {
+    public void setUseParentHandlers(boolean useParentHandlers) {
         checkAccess();
         this.useParentHandlers = useParentHandlers;
     }
@@ -1246,7 +1238,7 @@
      *
      * @return  true if output is to be sent to the logger's parent
      */
-    public synchronized boolean getUseParentHandlers() {
+    public boolean getUseParentHandlers() {
         return useParentHandlers;
     }

@@ -1354,9 +1346,12 @@
      * @return nearest existing parent Logger
      */
     public Logger getParent() {
-        synchronized (treeLock) {
-            return parent;
-        }
+        // Note: this used to be synchronized on treeLock.  However, this only
+        // provided memory semantics, as there was no guarantee that the caller
+        // would synchronize on treeLock (in fact, there is no way for external
+        // callers to so synchronize.  Therefore, we have made parent volatile
+        // instead.
+        return parent;
     }

     /**