java thread safety between multiple variables

Question

I have a Metrics class that's supposed to keep track of how many transactions we process each second and how long they take. The relevant part of its structure looks like this:

public class Metrics {
    AtomicLong sent = new AtomicLong();
    AtomicLong totalElapsedMsgTime = new AtomicLong();

    AtomicLong sentLastSecond = new AtomicLong();
    AtomicLong avgTimeLastSecond = new AtomicLong();

    public void outTick(long elapsedMsgTime){
        sent.getAndIncrement();
        totalElapsedMsgTime.getAndAdd(elapsedMsgTime);
    }

    class CalcMetrics extends TimerTask {
       @Override
        public void run() {
            sentLastSecond.set(sent.getAndSet(0));
            long tmpElapsed = totalElapsedMsgTime.getAndSet(0);
            long tmpSent = sentLastSecond.longValue();

            if(tmpSent != 0) {
                avgTimeLastSecond.set(tmpElapsed / tmpSent);
            } else {
                avgTimeLastSecond.set(0);
            }
        }
    }
}

My issue is that the outTick function will get called hundreds of times a second from lots of different threads. Being AtomicLong already ensures that each variable is individually thread safe, and they don't interact with each other in that function, so I don't want a lock that will make one call to outTick block another thread's call to outTick. It's perfectly fine if a couple of different threads increment the sent variable and then they both add to the totalElapsedMsgTime variable.

However, once it gets into CalcMetrics run method (which only happens once each second), they do interact. I want to ensure that I can pick up and reset both of those variables without being in the middle of an outTick call or having another outTick call occur between picking up one variable and the next.

Is there any way of doing this? (Does my explanation even make sense?) Is there a way of saying that A cannot interleave with B but multiple B's can interleave with each other?

EDIT:

I went with the ReadWriteLock that James suggested. Here's what my result looks like for anyone interested:

public class Metrics {
    AtomicLong numSent = new AtomicLong();
    AtomicLong totalElapsedMsgTime = new AtomicLong();

    long sentLastSecond = 0;
    long avgTimeLastSecond = 0;

    private final ReadWriteLock readWriteLock = new ReentrantReadWriteLock();
    private final Lock readLock = readWriteLock.readLock();
    private final Lock writeLock = readWriteLock.writeLock();

    public void outTick(long elapsedMsgTime) {
        readLock.lock();
        try {
            numSent.getAndIncrement();
            totalElapsedMsgTime.getAndAdd(elapsedMsgTime);
        }
        finally
        {
            readLock.unlock();
        }
    }

    class CalcMetrics extends TimerTask {

       @Override
        public void run() {
            long elapsed;

            writeLock.lock();
            try {
                sentLastSecond = numSent.getAndSet(0);
                elapsed = totalElapsedMsgTime.getAndSet(0);
            }
            finally {
                writeLock.unlock();
            }

            if(sentLastSecond != 0) {
                avgTimeLastSecond = (elapsed / sentLastSecond);
            } else {
                avgTimeLastSecond = 0;
            }
        }
    }
}

ZhongYu · Accepted Answer · 2015-10-20 23:23:59Z

1

The usual solution is to wrap all variables as one atomic data type.

class Data
{
    long v1, v2;

    Data add(Data another){ ... }
}

AtomicReference<Data> aData = ...;

public void outTick(long elapsedMsgTime)
{
    Data delta = new Data(1, elapsedMsgTime);

    aData.accumulateAndGet( delta, Data:add );
}

In your case, it may not be much faster than just locking.

There is another interesting lock in java8 - StampedLock . The javadoc example pretty much matches your use case. Basically, you can do optimistic reads on multiple variables; afterwards, check to make sure that no writes were done during the reads. In your case, "hundreds" of writes per second, the optimistic reads mostly would succeed.

answered Oct 20, 2015 at 23:23

ZhongYu

19.8k5 gold badges35 silver badges61 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Solomon Slow · Accepted Answer · 2015-10-21 13:14:35Z

0

Sounds like you need a reader/writer lock. (java.util.concurrent.locks.ReentrantReadWriteLock).

Your outTick() function would lock the ReaderLock. Any number of threads are allowed to lock the ReaderLock at the same time.

Your calcMetrics() would lock the WriterLock. No new readers are allowed in once a thread is waiting for the writer lock, and the writer is not allowed in until all the readers are out.

You would still need the atomics to protect the individual counters that are incremented by outTick().

answered Oct 21, 2015 at 13:14

Solomon Slow

27.6k5 gold badges39 silver badges61 bronze badges

2 Comments

Lesley Over a year ago

Thanks. That looks like what I was looking for. The "read" and "write" are a bit of a misnomer in my particular use case, since I want the concurrency (and thus the ReaderLock) where I'm updating them, and the exclusive WriterLock when I'm collecting the results, but at least that ReadWriteLock addresses both areas of exclusivity and areas of concurrency with the same variables. (I'm still pretty new with this language - so much to learn.)

Solomon Slow Over a year ago

@Lesley, Sometimes it's also known as a "shared/exclusive" lock.

Daniel Centore · Accepted Answer · 2015-10-21 03:41:47Z

Use locks ( https://docs.oracle.com/javase/tutorial/essential/concurrency/locksync.html ). Once you implement locks you'll have finer control. An additional side effect will be that you won't need to use AtomicLong anymore (although you still can); you can use volatile long instead, which would be more efficient. I did not make that change in the example.

Basically just create a new Object:

private Object lock = new Object();

Then, use the synchronized keyword with that object around all the code that should never happen at the same time as another synchronized block with the same lock. Example:

synchronized(lock)
{
    sent.getAndIncrement();
    totalElapsedMsgTime.getAndAdd(elapsedMsgTime);
}

So your whole program will look like this (note: untested code)

public class Metrics {
    private Object lock = new Object();

    AtomicLong sent = new AtomicLong();
    AtomicLong totalElapsedMsgTime = new AtomicLong();

    AtomicLong sentLastSecond = new AtomicLong();
    AtomicLong avgTimeLastSecond = new AtomicLong();

    public void outTick(long elapsedMsgTime){
        synchronized (lock)
        {
            sent.getAndIncrement();
            totalElapsedMsgTime.getAndAdd(elapsedMsgTime);
        }
    }

    class CalcMetrics extends TimerTask {
       @Override
        public void run() {
            synchronized (lock)
            {
                sentLastSecond.set(sent.getAndSet(0));
                long tmpElapsed = totalElapsedMsgTime.getAndSet(0);
                long tmpSent = sentLastSecond.longValue();

                if(tmpSent != 0) {
                    avgTimeLastSecond.set(tmpElapsed / tmpSent);
                } else {
                    avgTimeLastSecond.set(0);
                }
            }
        }
    }
}

Edit: I threw together a quick (and ugly) efficiency test program and found that when I synchronize with locks, I get overall better performance. Note that the results of the first 2 runs are discarded because the timing results when the Java JIT still hasn't compiled all code paths to machine code are not representative of the long term runtime.

Results:

With Locks: 8365ms
AtomicLong: 21254ms

Code:

import java.util.concurrent.atomic.AtomicLong;

public class Main
{
    private AtomicLong testA_1 = new AtomicLong();
    private AtomicLong testB_1 = new AtomicLong();

    private volatile long testA_2 = 0;
    private volatile long testB_2 = 0;

    private Object lock = new Object();

    private volatile boolean a = false;
    private volatile boolean b = false;
    private volatile boolean c = false;

    private static boolean useLocks = false;

    public static void main(String args[])
    {
        System.out.println("Locks:");
        useLocks = true;
        test();

        System.out.println("No Locks:");
        useLocks = false;
        test();

        System.out.println("Locks:");
        useLocks = true;
        test();

        System.out.println("No Locks:");
        useLocks = false;
        test();
    }

    private static void test()
    {
        final Main main = new Main();

        new Thread()
        {
            public void run()
            {
                for (int i = 0; i < 80000000; ++i)
                    main.outTick(10);

                main.a = true;
            }
        }.start();

        new Thread()
        {
            public void run()
            {
                for (int i = 0; i < 80000000; ++i)
                    main.outTick(10);

                main.b = true;
            }
        }.start();

        new Thread()
        {
            public void run()
            {
                for (int i = 0; i < 80000000; ++i)
                    main.outTick(10);

                main.c = true;
            }
        }.start();

        long startTime = System.currentTimeMillis();

        // Okay this isn't the best way to do this, but it's good enough
        while (!main.a || !main.b || !main.c)
        {
            try
            {
                Thread.sleep(1);
            } catch (InterruptedException e)
            {
            }
        }

        System.out.println("Elapsed time: " + (System.currentTimeMillis() - startTime) + "ms");
        System.out.println("Test A: " + main.testA_1 + " " + main.testA_2);
        System.out.println("Test B: " + main.testB_1 + " " + main.testB_2);
        System.out.println();
    }

    public void outTick(long elapsedMsgTime)
    {
        if (!useLocks)
        {
            testA_1.getAndIncrement();
            testB_1.getAndAdd(elapsedMsgTime);
        }
        else
        {
            synchronized (lock)
            {
                ++testA_2;
                testB_2 += elapsedMsgTime;
            }
        }
    }
}

Something like that would be my fallback solution if I can't get what I want to work. My issue with it is that it blocks more than I want blocked. (Multiple calls to outTick would block each other out, which will happen far more often than the once a second timer.) Then again, maybe being able to change the AtomicLongs to volatile longs would gain back as much efficiency as the extra blocking costs. I'm not sure on that part.
They already block each other out. AtomicLong just does the blocking for you so you don't see it, but the efficiency will be identical. Not to mention that operating on primitive types is much more efficient than calling methods. Just make sure they're volatile.
@Lesley See my code demonstrating the better efficiency of locks. In fact, using locks with AtomicLong (and not volatile long, as the example code) also turns out to be more efficient than AtomicLong without locks, because AtomicLong's brute force conflict resolution is more inefficient than Java locks.

Collectives™ on Stack Overflow

java thread safety between multiple variables

3 Answers 3

Comments

2 Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

2 Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related