Letting multiple Threads operate on a data set while one Thread sums it up

Question

I am trying to implement a banking system where I have a set of accounts. There are multiple threds trying to transfer money between accounts, while one thread continuosuly (or rather, at random times) tries to sum up the total money in the bank (Sum of all accounts balance).

The way to solve that sounded obvious at first; Using ReentrantReadWriteLocks with readLock for the threads doing the transactions and writeLock for the thread doing the summation. However after implementing it that way (see code below), I saw a huge drop in performance / "transaction-throughput" even versus doing transactions with only one thread.

• Code for above mentioned implementation:

  public class Account implements Compareable<Account>{
     private int id;
     private int balance;
  
     public Account(int id){
        this.id = id;
        this.balance = 0;
     }
  
     public synchronized int getBalance(){ return balance; }
  
     public synchronized setBalance(int balance){
        if(balance < 0){ throw new IllegalArgumentException("Negative balance"); }
        this.balance = balance;
     }
  
     public int getId(){ return this.id; }
  
     // To sort a collection of Accounts.
     public int compareTo(Account other){
        return (id < other.getId() ? -1 : (id == other.getId() ? 0 : 1));
     }
  }
  
  public class BankingSystem {
     protected List<Account> accounts;
     protected ReadWriteLock lock = new ReentrantReadWriteLock(); // !!
  
     public boolean transfer(Account from, Account to, int amount){
        if(from.getId() != to.getId()){
           synchronized(from){
              if(from.getBalance() < amount) return false;
              lock.readLock().lock(); // !!
              from.setBalance(from.getBalance() - amount);
           }
           synchronized(to){ 
              to.setBalance(to.getBalance() + amount);
              lock.readLock().unlock(); // !! 
           }
        }
        return true;
     }
  
     // Rest of class..
  }
  

Note that this is without even using the summation method yet, so no writeLock is ever being aquired. If I only delet the lines marked with a // !! and also do not call the summation method, suddenly the "transfer throughput" using multiple threads is a lot higher than when using a single thread, as is the goal.

My question now is, why does that simple introduction of a readWriteLock slow down the entire thing that much, if I never try to aquire a writeLock, and what I did wrong here, because I can't manage to find the problem.

• Sidenote: I had asked a question regarding this problem already here, but managed to ask the wrong question. I did however, get an amazing answer for that question I asked. I decided to not reduce question quality immensly, and to keep that great answer alive for people in need for help regardin that matter, I would not edit the question (yet again). Instead I opened this question, in the strong belief that this is NOT a duplicate, but instead an entirely different matter.

Answer 1

Locking is expensive but in your case, I assume there might be some kind of "almost deadlock" when you run the tests: If some thread is in the synchronized(from){} block of your code and another thread wants to unlock the from instance in it's synchronized(to){} block, then it won't be able to: The first synchronized will prevent thread #2 from entering the synchronized(to){} block and hence the lock won't be released very quickly.

That could lead to a lot of threads hanging in the queue of the lock which makes it slow to get/release locks.

Some more notes: Your code will cause problems when the second part (to.setBalance(to.getBalance() + amount);) isn't executed for some reason (exception, deadlock). You need to find out a way to create a transaction around the two operations to make sure they are either executed both or none are executed.

A good way to do this is to create a Balance value object. In your code, you can create two new ones, update both balances and then just call the two setters - since setters can't fail, either both balances will be updated or the code will fail before any setters could be called.

Answer 2

First of all, it’s correct to put the update into its own synchronized block, even if the getter and setter are synchronized on its own, so you avoid the check-then-act anti-pattern.

However, from a performance standpoint it’s not optimal as you acquire the same lock three times (four times for the from account). The JVM, or the HotSpot optimizer, knows the synchronization primitives and is able to optimize such patterns of nested synchronization, but (now we have to guess a bit) if you acquire another lock in-between, it might prevent these optimizations.

As already suggested at the other question, you may turn to a lock free update, but of course you have to fully understand it. The lock free updates are centered around one special operation, compareAndSet which performs an update only if the variable has the expected old value, in other words, no concurrent update has been performed in-between, whereas checking and updating is performed as one atomic operation. And that operation is not implemented using synchronized but utilizing a dedicated CPU instruction directly.

The use pattern is always like

1. read the current value
2. calculate the new value (or reject the update)
3. attempt to perform an update which will succeed if the current value is still the same

The drawback is that the update might fail which requires repeating the three steps but it’s acceptable if the computation is not too heavy and since a failed update indicates that another thread must have succeed with its update in-between, there will always be a progress.

This led to the example code for an account:

static void safeWithdraw(AtomicInteger account, int amount) {
    for(;;) { // a loop as we might have to repeat the steps
        int current=account.get(); // 1. read the current value
        if(amount>current) throw new IllegalStateException();// 2. possibly reject
        int newValue=current-amount; // 2. calculate new value
        // 3. update if current value didn’t change
        if(account.compareAndSet(current, newValue))
            return; // exit on success
    }
}

So, for supporting lock-free access it’s never sufficient to provide getBalance and setBalance operations as every attempt to compose an operation out of get and set operations without locking will fail. You have three options:

1. Provide every supported update operation as a dedicated method like the safeWithdraw method
2. Provide a compareAndSet method to allow callers to compose their own update operations using that method
3. Provide an update method that takes an update function as paramater, like the AtomicInteger does in Java 8; Of course, this comes especially handy when using Java 8 where you can use lambda expressions to implement the actual update function.

Note that AtomicInteger itself uses all options. There are dedicated update methods for common operations like increment and there’s the compareAndSet method allowing composing arbitrary update operations.

Answer 3

You would generally use either a lock or synchronized, it is unusual to use both at once.

To manage your scenario you would normally use a fine-grained lock on each account rather than a coarse one as you have. You would also implement the totalling mechanism using a listener.

public interface Listener {

    public void changed(int oldValue, int newValue);
}

public class Account {

    private int id;
    private int balance;
    protected ReadWriteLock lock = new ReentrantReadWriteLock();
    List<Listener> accountListeners = new ArrayList<>();

    public Account(int id) {
        this.id = id;
        this.balance = 0;
    }

    public int getBalance() {
        int localBalance;
        lock.readLock().lock();
        try {
            localBalance = this.balance;
        } finally {
            lock.readLock().unlock();
        }
        return localBalance;
    }

    public void setBalance(int balance) {
        if (balance < 0) {
            throw new IllegalArgumentException("Negative balance");
        }
        // Keep track of the old balance for the listener.
        int oldValue = this.balance;
        lock.writeLock().lock();
        try {
            this.balance = balance;
        } finally {
            lock.writeLock().unlock();
        }
        if (this.balance != oldValue) {
            // Inform all listeners of any change.
            accountListeners.stream().forEach((l) -> {
                l.changed(oldValue, this.balance);
            });
        }
    }

    public boolean lock() throws InterruptedException {
        return lock.writeLock().tryLock(1, TimeUnit.SECONDS);
    }

    public void unlock() {
        lock.writeLock().unlock();
    }

    public void addListener(Listener l) {
        accountListeners.add(l);
    }

    public int getId() {
        return this.id;
    }

}

public class BankingSystem {

    protected List<Account> accounts;

    public boolean transfer(Account from, Account to, int amount) throws InterruptedException {
        if (from.getId() != to.getId()) {
            if (from.lock()) {
                try {
                    if (from.getBalance() < amount) {
                        return false;
                    }
                    if (to.lock()) {
                        try {
                            // We have write locks on both accounts.
                            from.setBalance(from.getBalance() - amount);
                            to.setBalance(to.getBalance() + amount);
                        } finally {
                            to.unlock();
                        }

                    } else {
                        // Not sure what to do - failed to lock the account.
                    }
                } finally {
                    from.unlock();
                }

            } else {
                // Not sure what to do - failed to lock the account.
            }
        }
        return true;
    }

    // Rest of class..
}

Note that you can take a write lock in the same thread twice - the second one is also allowed. Locks only exclude access from other threads.