2009/3/12 Martin Buchholz <marti...@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.ll...@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<Integer> threadIds = new
>> ThreadLocal<Integer>();
>> + private static final AtomicLong globalSequenceNumber = new
>> AtomicLong();
>> + private static final AtomicInteger nextThreadId = new
>> AtomicInteger(10);
>> + private static final ThreadLocal<Integer> threadIds = new
>> ThreadLocal<Integer>();
>>
>> /**
>> * @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<Integer> threadIds = new
>> ThreadLocal<Integer>();
>> + private static final AtomicLong globalSequenceNumber = new
>> AtomicLong();
>> + private static final AtomicInteger nextThreadId = new
>> AtomicInteger(10);
>> + private static final ThreadLocal<Integer> threadIds = new
>> ThreadLocal<Integer>();
>>
>> /**
>> * @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
