Thread-safe implementation of max

Question

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 1

As of Java 8, LongAccumulator has been introduced. It is advised as

This class is usually preferable to AtomicLong when multiple threads update a common value that is used for purposes such as collecting statistics, not for fine-grained synchronization control. Under low update contention, the two classes have similar characteristics. But under high contention, expected throughput of this class is significantly higher, at the expense of higher space consumption.

You can use it as follows:

LongAccumulator maxId = new LongAccumulator(Long::max, 0); //replace 0 with desired initial value
maxId.accumulate(newValue); //from each thread

Answer 2

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.

Answer 3

With Java 8 you can take advantage of functional interfaces and a simple lamda expression to solve this with one line and no looping:

private void updateMax(long sample) {
    max.updateAndGet(curMax -> (sample > curMax) ? sample : curMax);
}

The solution uses the updateAndGet(LongUnaryOperator) method. The current value is contained in curMax and using the conditional operator a simple test is performed replacing the current max value with the sample value if the sample value is greater than the current max value.

Answer 4

as if you didn't have your pick of answers, here's mine:

// while the update appears bigger than the atomic, try to update the atomic.
private void max(AtomicDouble atomicDouble, double update) {
    double expect = atomicDouble.get();
    while (update > expect) {
        atomicDouble.weakCompareAndSet(expect, update);
        expect = atomicDouble.get();
    }
}

it's more or less the same as the accepted answer, but doesn't use break or while(true) which I personally don't like.

EDIT: just discovered DoubleAccumulator in java 8. the documentation even says this is for summary statistics problems like yours:

DoubleAccumulator max = new DoubleAccumulator(Double::max, Double.NEGATIVE_INFINITY);
parallelStream.forEach(max::accumulate);
max.get();

Answer 5

I believe what you did is correct, but this is a simpler version that I also think is correct.

private void updateMax(long sample){
      //this takes care of the case where between the comparison and update steps, another thread updates the max

      //For example:
      //if the max value is set to a higher max value than the current value in between the comparison and update step
      //sample will be the higher value from the other thread
      //this means that the sample will now be higher than the current highest (as we just set it to the value passed into this function)
      //on the next iteration of the while loop, we will update max to match the true max value
      //we will then fail the while loop check, and be done with trying to update.
      while(sample > max.get()){
          sample = max.getAndSet(sample);  
      }
}