How is the metake mark () method the mark mark () method unsafe?
I recently started looking into the CodaHale / DropWizard metrics library. I can't figure out how the Meter class is thread safe (according to the documentation), especially the mark () and tickIfNecessary () marks here:
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 see that there is a last tick of the AtomicLong type, but there could still be a situation where the m1-m15 bets tick a little longer, so another thread can trigger these ticks as well as part of the next TICK_INTERVAL. Wouldn't that be a race condition since the Tick () method of bids 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;
}
}
Thank,
Marian
source to share
As far as I can see, you are right. If tickIfNecessary()
called in such a way that age > TICK_INTERVAL
while another call is still running, it is possible that m1Rate.tick()
other methods tick()
are being called simultaneously from multiple threads. This way it boils down to more tick()
and its called procedures / operations are safe.
Let it dissipate 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 when the instance is initialized and marked final are thread safe because they are read-only. count
and instantRate
are local, and in any case they are not visible to others. rate
and initialized
are marked volatile and these records should always be visible for future reads.
If I am not mistaken, to a large extent from the first reading initialized
to the last recording initialized
, or rate
it is open to the races, but some of them do not have the effect, for example, when the 2 streams involved in the switch initialized
to true
.
It seems that most effective races can occur in rate += (alpha * (instantRate - rate));
especially cleaned or mixed calculations, such as:
- Assumed:
initialized
yestrue
- Thread1: calculates
count
,instantRate
verifiesinitialized
, first readingrate
, which we callprevious_rate
, and for some reason stops - Thread2: calculates
count
,instantRate
verifiesinitialized
and evaluatesrate += (alpha * (instantRate - rate));
- Thread1: keeps running and calculates
rate += (alpha * (instantRate - previous_rate));
A blob will occur if reads and writes are somehow ordered in such a way that rate
reads in all threads and then writes to all threads, effectively discarding one or more computation.
But the likelihood of such races, which means that both are the age > TICK_INTERVAL
same, so that 2 threads running in the same method tick()
, and especially rate += (alpha * (instantRate - rate))
, can be extremely low and depending on values that are not noticeable.
A method mark()
appears to be thread safe if it LongAdderProxy
uses a thread safe data structure for update
/ add
and for a method tick()
in sumThenReset
.
I think the only ones who can answer the questions left open is that races have no discernible effect or are otherwise mitigated - are the authors of the project or people who have a deep knowledge of these parts of the project and the calculated values.
source to share
It's thread-safe because this string from tickIfNecessary()
returns true only once at a timenewIntervalStartTick
if (lastTick.compareAndSet(oldTick, newIntervalStartTick))
What happens if two threads enter at tickIfNecessary()
almost the same time?
Both streams read the same value from oldTick
, decide that at least TICK_INTERVAL
nanoseconds have passed, and calculate newIntervalStartTick
.
Now both threads are trying to do lastTick.compareAndSet(oldTick, newIntervalStartTick)
. As the name suggests compareAndSet
, this method compares to the current value from lastTick
to oldTick
, and only if the value is equal oldTick
, it gets an atomic replacement with newIntervalStartTick
and returns true.
Since this is an atomic instruction (in hardware!), Only one thread can succeed. When another thread executes this method, it will already see it newIntervalStartTick
as the current value 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()
before m15Rate.tick()
.
The method is EWMA.update(n)
used java.util.concurrent.atomic.LongAdder
to accumulate event counters, which provide similar guarantees for thread safety.
source to share