Does this usage of AtomicLong look thread-safe?

Question

I wanted to ensure that lastActivityTime is always the "latest" value based on current time, but I wonder if my "update" method is thread-safe? If it's not, what's a better solution besides just throwing "synchronized" around.

import java.util.concurrent.atomic.AtomicLong;

public class Widget {

    private AtomicLong lastActivityTime = new AtomicLong();

    public long getLastActivityTime() {
        return lastActivityTime.get();
    }

    public void update() {
        long now = System.currentTimeMillis();

        if (getLastActivityTime() < now) {
            lastActivityTime.set(now);
        }
    }

}

Answer 1

No way! Use

public void update() {
    long now = System.currentTimeMillis();

    while (true) {
        long stored = lastActivityTime.get();
        if (stored >= now) break;
        if (lastActivityTime.compareAndSet(stored, now)) break;
    }
}

Explanation:

Your code is obviously racy as between the test and the store, the value may change and you may overwrite a bigger value with a smaller one.

For computing the running maximum, I compare stored >= now and if it's true, there's nothing to do. The compareAndSet operation only succeeds (i.e., does anything and returns true), if the stored value hasn't changed in the meantime. Otherwise, I simply retry.

Look at how addAndGet and similar operations work.

An optimizations for really many cores

If there are many cores and the threads have nothing to do but content for our poor AtomicLong, a significant slowdown could occur. There's a simple solution inspired by the LongAdder.

private static final int PADDING = 8; // fight false sharing
private static final int CONCURRENCY = 8; // use 8 counters
private AtomicLongArray lastActivityTimes = new AtomicLongArray(CONCURRENCY * PADDING);

public long getLastActivityTime() {
    long result = Long.MIN_VALUE;
    // the biggest of the values contains the maximum
    for (int i=0; i<CONCURRENCY; ++i) {
        result = Math.max(result, lastActivityTimes.get(i*PADDING);
    }
    return result;
}
public void update() {
    long now = System.currentTimeMillis();
    Random random = ThreadLocalRandom.get();    

    while (true) {
        int i = PADDING * random.nextInt(CONCURRENCY);
        long stored = lastActivityTimes.get(i);
        if (stored >= now) break;
        if (lastActivityTimes.compareAndSet(i, stored, now)) break;
    }
}

It's a simple splitting to multiple slots where the result is given by the biggest value. When updating a slot is selected at random. There are better strategies, but this can get pretty complicated and can lead to memory leaks

To avoid false sharing, only every 8th long gets used.