2
\$\begingroup\$

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);
        }
    }

}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ I didn't consider using a loop, that looks a like it could really slow things down if multiple threads hit at the same time. \$\endgroup\$ Dec 5, 2014 at 20:25
  • \$\begingroup\$ @Mondain There's no solution without a loop. If the thread have something else to do, the contention can't be that high, unless you're running on tens of cores. In case of contention, one thread is guaranteed to success. For many cores, I know a better algorithm, but I strongly doubt you need it. Do you? \$\endgroup\$
    – maaartinus
    Dec 5, 2014 at 20:28
  • \$\begingroup\$ Fair enough, but I'm curious about the other routine. Even if I probably won't exceed 8 cores. \$\endgroup\$ Dec 5, 2014 at 20:30
  • \$\begingroup\$ Nicely done, hats off mister! \$\endgroup\$
    – janos
    Dec 5, 2014 at 21:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.