AtomicLong operations

Question

I need to perform the following operation:

// average, total, elapsed are Long's

average = ( ( total * average ) + elapsed ) / (++total);

But I want to use AtomicLong

This is what I'm trying but I don't quite get if it is correct:

 average.set( (( total.get() * average.get() ) + elapsed) / total.getAndIncrement() );

How can I tell if this is correct?

Answer 1

Presumably you are using AtomicLong because these numbers are being accessed concurrently. Since you have two numbers involved and you are using both a get and incrementAndGet in the same statement, I don't think AtomicLong is the right choice.

I have found AtomicXXX to be very helpful in many situations. But here, I think you need to do it the hard way. Make your numbers simple "long" private variables, create a guard object, then make sure to synchronize on the guard object any time you access the numbers.

I think that is the only way you can be certain that these operations are truly atomic.

Answer 2

Firstly note that on some platforms AtomicLong is implemented with a lock, so you may see a significant variation in performance.

You appear to be attempting to update two variables at once. Although many modern processors support that, the Java library does not. The version with a lock is trivial, so I'll elide that. You may also want to calculate the average on the get, and just keep a running sum and total, but I'll ignore that for the moment.

The most literal implementation would be using an AtomicReference to an immutable value. Note this is going to cause an allocation, so may have great performance, particularly under low contention.

final class Average { // Find a better name...
    private final long average;
    private final long total;
    public Average(long average, long total) {
        this.average = average
        this.total = total;
    }
    public long average() {
        return average;
    }
    public long total() {
        return total;
    }
}
...
private final AtomicReference<Average> averageRef = new AtomicReference<>();
private void elapsed(final long elapsed) {
    Average prev;
    Average next;
    do {
        prev = average.get();
        next = new Average(
            ((prev.total() * prev.average()) + elapsed ) / (prev.total() + 1),
            prev.total() + 1
        );
    } while (!average.compareAndSet(prev, next));
}

Possibly a better solution is to keep a thread local (preferably not ThreadLocal but an instance you've given to a particular thread to mutate). That can very quickly be locked and unlocked, because it'll be from the same thread. A thread infrequently requiring an average can then lock and read/read current values from all threads.

class Average { // Choose better name
    private long sum;
    private long total;
    public synchronized void elapsed(final long elapsed) {
         sum += elapsed;
         ++total;
    }
    public static long average(Iterable<Average> averages) {
        long sum = 0;
        long total = 0;
        for (Average average : averages) {
            synchronized (average) {
                sum += averages.sum;
                total += average.total;
            }
        }
        return total==0 ? 0 : (sum/total);
    }
}

(Disclaimer: Not checked or tested or compiled.)

Answer 3

The assumption is that these calculations are going to be called by multiple threads concurrently. I initially did not take that into effect.

If you want to use the AtomicLong to do the pre-increment calculation, then you should do something like:

long value = total.getAndIncrement();
average.set((value * average.get()) + elapsed) / (value + 1));

This still has race conditions however since the average could be updated by someone else between the average.get() and the average.set() call which would not be taken into effect in the update.

To be completely sure, you need to (as @user1657364 mentioned in their answer) lock around a guard object.