CODEWITHSUNDEEP
javamultithreading
dave
dave

Reputation: 11975

How do I reduce the granularity of locking while maintaining thread safety?

I have a simple, managed group of Stacks that need to be accessed in a thread-safe manner. My first implementation is working correctly but uses synchronized methods for all access, ie. locking is at the most coarse level. I'd like to make locking as granular as possible but I'm unsure of the best way to go about it.

Here's the basics of my Stack manager class (with some details elided for brevity):

public class StackManager {
    private final Map<String, Deque<String>> myStacks;
    public StackManager() {
        myStacks = new ConcurrentHashMap<String, Deque<String>>();
    }
    public synchronized void addStack(String name) {
        if (myStacks.containsKey(name)) {
            throw new IllegalArgumentException();
        }
        myStacks.put(name, new ConcurrentLinkedDeque<String>());
    }
    public synchronized void removeStack(String name) {
        if (!myStacks.containsKey(name)) {
            throw new IllegalArgumentException();
        }
        myStacks.remove(name);
    }
    public synchronized void push(String stack, String payload) {
        if (!myStacks.containsKey(stack)) {
            throw new IllegalArgumentException();
        }
        myStacks.get(stack).push(payload);
    }
    public synchronized String pop(String stack) {
        if (!myStacks.containsKey(stack)) {
            throw new IllegalArgumentException();
        }
        return myStacks.get(stack).pop();
    }
}

The stack-level methods (addStack(), removeStack()) are not used that often. However I'd like to know if their level of locking can be reduced. For example, if these methods were unsynchronized and established a lock on myStacks would this reduce contention? For example,

    public void addStack(String name) {
        synchronized(myStacks) {
            if (myStacks.containsKey(name)) {
                throw new IllegalArgumentException();
            }
            myStacks.put(name, new ConcurrentLinkedDeque<String>());
        }
    }

The per-stack methods (push(), pop()) are where I feel the most gains can be made. I'd like to achieve per-stack locking if I could. That is, only lock the single stack within the stack manager that is being operated on. However I cannot see a good way to do this. Any suggestions?

While we're here, is it necessary to use the concurrent implementations of both Map and Deque?

Upvotes: 1

Views: 285

Answers (1)

Juan
Juan

Reputation: 430

Both data structures are thread safe. So, every isolated operation on the is thread safe.

The problem is performing more than one operation when there's a dependency between them.

In your case, checking for existance must be atomic with the actual operation to avoid race conditions. To add a new stack, you can use the method putIfAbsent, which is atomic and not synchronized.

To remove a stack, you don't need to check for existance. If you want to know whether it existed, just return remove method return value. If it's null, it didn't exist.

To perform push and pop, you just have to get the stack first and assign to a local variable. If it's null, it didn't exist. If it's nonnull, you can safely push or pop.

The attribute myStacks must be either final or volatile to be thread safe.

Now you don't need any synchronization. And I would choose a solution without exceptions. Only to add a new stack it seems more necessary. If it can happen in a correct program, it should be a checked exception. Runtime exception is more suitable when it is supposed to be a bug.

Oh, and triplecheck and test it, as concurrent programming is tricky.

Upvotes: 1

Related Questions