[PATCH 1/1] Get rid of synchronization in java.util.logging.LogRecord constructor (original) (raw)

Andrew John Hughes gnu_andrew at member.fsf.org
Fri Mar 13 00:13:39 UTC 2009

2009/3/12 Martin Buchholz <martinrb at google.com>: > This looks fine, as long as there is no dependency of the implementation > on the two atomic counters being incremented in concert, as seems likely. >> Martin >> On Thu, Mar 12, 2009 at 15:35, David M. Lloyd <david.lloyd at redhat.com> wrote: >> Switch to atomic ops for the various sequence numbers, as opposed to >> synchronizing on the class object on every object construction. >>>> - DML >> -- >>>> diff -r dde3fe2e8164 src/share/classes/java/util/logging/LogRecord.java >> --- a/src/share/classes/java/util/logging/LogRecord.java Wed Feb 25 >> 14:32:01 2009 +0000 >> +++ b/src/share/classes/java/util/logging/LogRecord.java Thu Mar 12 >> 17:12:22 2009 -0500 >> @@ -25,6 +25,8 @@ >>>> package java.util.logging; >> import java.util.;* >> +import java.util.concurrent.atomic.AtomicLong; >> +import java.util.concurrent.atomic.AtomicInteger; >> import java.io.;* >>>> /** >> @@ -64,9 +66,9 @@ >> / >>>> public class LogRecord implements java.io.Serializable {* >> - private static long globalSequenceNumber; >> - private static int nextThreadId=10; >> - private static ThreadLocal threadIds = new >> ThreadLocal(); >> + private static final AtomicLong globalSequenceNumber = new >> AtomicLong(); >> + private static final AtomicInteger nextThreadId = new >> AtomicInteger(10); >> + private static final ThreadLocal threadIds = new >> ThreadLocal(); >>>> /** >> * @serial Logging message level >> @@ -144,15 +146,13 @@ >> this.level = level; >> message = msg; >> // Assign a thread ID and a unique sequence number. >> - synchronized (LogRecord.class) { >> - sequenceNumber = globalSequenceNumber++; >> - Integer id = threadIds.get(); >> - if (id == null) { >> - id = new Integer(nextThreadId++); >> - threadIds.set(id); >> - } >> - threadID = id.intValue(); >> + sequenceNumber = globalSequenceNumber.getAndIncrement(); >> + Integer id = threadIds.get(); >> + if (id == null) { >> + id = Integer.valueOf(nextThreadId.getAndIncrement()); >> + threadIds.set(id); >> } >> + threadID = id.intValue(); >> millis = System.currentTimeMillis(); >> needToInferCaller = true; >> } >>>> diff -r dde3fe2e8164 src/share/classes/java/util/logging/LogRecord.java >> --- a/src/share/classes/java/util/logging/LogRecord.java Wed Feb 25 >> 14:32:01 2009 +0000 >> +++ b/src/share/classes/java/util/logging/LogRecord.java Thu Mar 12 >> 17:12:22 2009 -0500 >> @@ -25,6 +25,8 @@ >>>> package java.util.logging; >> import java.util.;* >> +import java.util.concurrent.atomic.AtomicLong; >> +import java.util.concurrent.atomic.AtomicInteger; >> import java.io.;* >>>> /** >> @@ -64,9 +66,9 @@ >> / >>>> public class LogRecord implements java.io.Serializable {* >> - private static long globalSequenceNumber; >> - private static int nextThreadId=10; >> - private static ThreadLocal threadIds = new >> ThreadLocal(); >> + private static final AtomicLong globalSequenceNumber = new >> AtomicLong(); >> + private static final AtomicInteger nextThreadId = new >> AtomicInteger(10); >> + private static final ThreadLocal threadIds = new >> ThreadLocal(); >>>> /** >> * @serial Logging message level >> @@ -144,15 +146,13 @@ >> this.level = level; >> message = msg; >> // Assign a thread ID and a unique sequence number. >> - synchronized (LogRecord.class) { >> - sequenceNumber = globalSequenceNumber++; >> - Integer id = threadIds.get(); >> - if (id == null) { >> - id = new Integer(nextThreadId++); >> - threadIds.set(id); >> - } >> - threadID = id.intValue(); >> + sequenceNumber = globalSequenceNumber.getAndIncrement(); >> + Integer id = threadIds.get(); >> + if (id == null) { >> + id = Integer.valueOf(nextThreadId.getAndIncrement()); >> + threadIds.set(id); >> } >> + threadID = id.intValue(); >> millis = System.currentTimeMillis(); >> needToInferCaller = true; >> } >>>>> This is actually an interesting rare case where two atomic variables can replace a synchronised block. Looking at the code, there seems to be no issue with the thread being descheduled and then resumed during the operation of this constructor. Both atomic variables are only used within the constructor. The global sequence number is incremented and retrieve atomically and then assigned to a local variable. The rest of the code deals with allocating an ID to the thread creating the LogRequest object and doesn't depend on the global sequence number, so it doesn't matter if this is incremented by another thread before the constructor completes. Note that Thread.currentThread.getId() now provides an identifier for threads as well, but this will reuse the identifiers of dead threads and could thus lead to possible confusion between log messages.

Andrew :-)

Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com)

Support Free Java! Contribute to GNU Classpath and the OpenJDK http://www.gnu.org/software/classpath http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net) Fingerprint: F8EF F1EA 401E 2E60 15FA 7927 142C 2591 94EF D9D8