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.