How is codahale metrics Meter mark() method threadsafe?

Question

I have recently begun to learn CodaHale/DropWizard metrics library. I cannot understand how is the Meter class thread-safe (it is according to the documentation), especially mark() and tickIfNecessary() methods here:

https://github.com/dropwizard/metrics/blob/3.2-development/metrics-core/src/main/java/com/codahale/metrics/Meter.java#L54-L77

public void mark(long n) {
    tickIfNecessary();
    count.add(n);
    m1Rate.update(n);
    m5Rate.update(n);
    m15Rate.update(n);
}

private void tickIfNecessary() {
    final long oldTick = lastTick.get();
    final long newTick = clock.getTick();
    final long age = newTick - oldTick;
    if (age > TICK_INTERVAL) {
        final long newIntervalStartTick = newTick - age % TICK_INTERVAL;
        if (lastTick.compareAndSet(oldTick, newIntervalStartTick)) {
            final long requiredTicks = age / TICK_INTERVAL;
            for (long i = 0; i < requiredTicks; i++) {
                m1Rate.tick();
                m5Rate.tick();
                m15Rate.tick();
            }
        }
    }
}

I can see that there is a lastTick of type AtomicLong, but still there can be a situation that m1-m15 rates are ticking a little bit longer so another thread can invoke those ticks as well as a part of next TICK_INTERVAL. Wouldn't that be a race condition since tick() method of Rates is not synchronized at all? https://github.com/dropwizard/metrics/blob/3.2-development/metrics-core/src/main/java/com/codahale/metrics/EWMA.java#L86-L95

public void tick() {
    final long count = uncounted.sumThenReset();
    final double instantRate = count / interval;
    if (initialized) {
        rate += (alpha * (instantRate - rate));
    } else {
        rate = instantRate;
        initialized = true;
    }
}

Thanks,

Marian

Answer 1

It is thread safe because this line from tickIfNecessary() returns true only once per newIntervalStartTick

if (lastTick.compareAndSet(oldTick, newIntervalStartTick))

What happens if two threads enter tickIfNecessary() at almost the same time?

Both threads read the same value from oldTick, decide that at least TICK_INTERVAL nanoseconds have passed and calculate a newIntervalStartTick.

Now both threads try to do lastTick.compareAndSet(oldTick, newIntervalStartTick). As the name compareAndSet implies, this method compares to current value of lastTick to oldTick and only if the value is equal to oldTick it gets atomically replaced with newIntervalStartTick and returns true.

Since this is an atomic instruction (at the hardware level!), only one thread can succeed. When the other thread executes this method it will already see newIntervalStartTick as the current value of lastTick. Since this value no longer matches oldTick the update fails and the method returns false and therefore this thread does not call m1Rate.tick() to m15Rate.tick().

The EWMA.update(n) method uses a java.util.concurrent.atomic.LongAdder to accumulate the event counts that gives similar thread safety guarantees.

Answer 2

As far as I can see you are right. If tickIfNecessary() is called such that age > TICK_INTERVAL while another call is still running, it is possible that m1Rate.tick() and the other tick() methods are called at the same time from multiple threads. So it boils down to wether tick() and its called routines/operations are safe.

Let's dissect tick():

public void tick() {
    final long count = uncounted.sumThenReset();
    final double instantRate = count / interval;
    if (initialized) {
        rate += (alpha * (instantRate - rate));
    } else {
        rate = instantRate;
        initialized = true;
    }
}

alpha and interval are set only on instance initialization and marked final those thread-safe since read-only. count and instantRate are local and those not visible to other threads anyway. rate and initialized are marked volatile and those writes should always be visible for following reads.

If I'm not wrong, pretty much from the first read of initialized to the last write on either initialized or rate this is open for races but some are without effect like when 2 threads race for the switch of initialized to true.

It seems the majority of effective races can happen in rate += (alpha * (instantRate - rate)); especially dropped or mixed calculations like:

1. Assumed: initialized is true
2. Thread1: calculates count, instantRate, checks initialized, does the first read of rate which we call previous_rate and for whatever reason stalls
3. Thread2: calculates count, instantRate, checks initialized, and calculates rate += (alpha * (instantRate - rate));
4. Thread1: continues its operation and calculates rate += (alpha * (instantRate - previous_rate));

A drop would occur if the reads and writes somehow get ordered such that rate is read on all threads and then written on all threads, effectively dropping one or more calculations.

But the probability for such races, meaning that both age > TICK_INTERVAL matches such that 2 Threads run into the same tick() method and especially the rate += (alpha * (instantRate - rate)) may be extremely low and depending on the values not noticeable.

The mark() method seems to be thread-safe as long as the LongAdderProxy uses a thread-safe Data-structure for update/add and for the tick() method in sumThenReset.

I think the only ones who can answer the Questions left open - wether the races are without noticeable effect or otherwise mitigated - are the project authors or people who have in depth knowledge of these parts of the project and the values calculated.