saberduck saberduck - 3 months ago 10
Java Question

Thread-safe implementation of max

I need to implement global object collecting statistics for web server. I have

Statistics
singleton, which has method
addSample(long sample)
, which subsequently call
updateMax
. This has to be obviously thread-safe. I have this method for updating maximum of whole Statistics:



AtomicLong max;

private void updateMax(long sample) {
while (true) {
long curMax = max.get();
if (curMax < sample) {
boolean result = max.compareAndSet(curMax, sample);
if (result) break;
} else {
break;
}
}
}


Is this implementation correct? I am using java.util.concurrent, because I believe it would be faster than simple
synchronized
. Is there some other / better way to implement this?

Answer

I think it's correct, but I'd probably rewrite it a little for clarity, and definitely add comments:

private void updateMax(long sample) {
    while (true) {
        long curMax = max.get();
        if (curMax >= sample) {
            // Current max is higher, so whatever other threads are
            // doing, our current sample can't change max.
            break;
        }

        // Try updating the max value, but only if it's equal to the
        // one we've just seen. We don't want to overwrite a potentially
        // higher value which has been set since our "get" call.
        boolean setSuccessful = max.compareAndSet(curMax, sample);

        if (setSuccessful) {
            // We managed to update the max value; no other threads
            // got in there first. We're definitely done.
            break;
        }

        // Another thread updated the max value between our get and
        // compareAndSet calls. Our sample can still be higher than the
        // new value though - go round and try again.
    }
}

EDIT: Usually I'd at least try the synchronized version first, and only go for this sort of lock-free code when I'd found that it was causing a problem.