Lock+HasMap or ConcurrentHashMap in my case?

Question

I have a Map<String, Queue<?>> and each time I have to put a couple (key, value) I need to get the not thread-safe Queue associated with that key and add a value to it (if key exist). Because I need to update an existing value (queue) I think that the best way is to use a ReentrantLock (the synchronized block or synchronized (object) in Java < 1.5) and not ConcurrentHashMap.

Is that correct or can I use ConcurrentHashMap instead of HashMap + Lock? I think ConcurrentHashMap is more efficient in get operation but I don't know if could be the right solution here.

    private static ReentrantLock lock_Vqueue = new ReentrantLock();
    private static HashMap<String,Queue<DocumentObjectHolder>> cacheData = new HashMap<String,Queue<DocumentObjectHolder>>();
        /**
         * insert element in the tail 
         * sort the elements by priority 
         * @param obj a DocumentObjectHolder Object
         */
        public static void add(String key, DocumentObjectHolder obj){
            ReentrantLock lock = lock_Vqueue; //performance side effect
            try{
                Queue<DocumentObjectHolder>priorityProcessingVirtualQueue;
                lock.lock();
                    if (!cacheData.containsKey(key)){
                        priorityProcessingVirtualQueue = new PriorityQueue<DocumentObjectHolder>(1, new PriorityComparator());
                        cacheData.put(key, priorityProcessingVirtualQueue);
                    }
                    priorityProcessingVirtualQueue = cacheData.get(key);
                    priorityProcessingVirtualQueue.add(obj);
                    cacheData.put(key, priorityProcessingVirtualQueue);
            }finally {
                lock.unlock();
            }
        }

        /**
         * 
         * @return DocumentObjectHolder instance from head of list (FIFO)
         */
        public DocumentObjectHolder get(String key){
            ReentrantLock lock = lock_Vqueue; //performance side effect
            Queue<DocumentObjectHolder>priorityProcessingVirtualQueue;
            try {
                lock.lock(); 
                 if (cacheData.containsKey(key)){
                     priorityProcessingVirtualQueue = cacheData.get(key);
                     return priorityProcessingVirtualQueue.poll();
                 }
                 return null;
            }finally{
                lock.unlock();
            }
        }
}

Is this code right or could it be more performant?

Answer 1

So you have two separate atomic instructions which need to be synchronized with.

1. Putting a new queue into the Map
2. Putting an object into the Queue

Here are my suggestions.

1. Continue to use a ConcurrentHashMap. You still need to put into the map and you may as well do it safely with a CHM.

2. Use a BlockingQueue. In this case you can use a PriorityBlockingQueue.

3. If you cannot do (2) then synchronize on the queue from the Map.

So 1 & 3 would look like:

    public static void add(String key, DocumentObjectHolder obj){
        Queue<DocumentObjectHolder> priorityProcessingVirtualQueue= cacheData.get(key);
        if(priorityProcessingVirtualQueue== null){
             Queue<DocumentObjectHolder> temp = new PriorityQueue<DocumentObjectHolder>(1, new PriorityComparator());
             queue = cacheData.putIfAbsent(key, temp);
             if(priorityProcessingVirtualQueue== null){ 
                  priorityProcessingVirtualQueue= temp;
             } 
        }
        synchronized(priorityProcessingVirtualQueue){
            priorityProcessingVirtualQueue.add(obj);
        }
    }

The only difference needed for 1 & 2 is the absence of synchronized.

The reason we know this is thread-safe is because even if 2 or more threads enter if(queue == null) only one will succeed in putIfAbsent. The threads which lose will assign queue to equal the Queue that was successfully put in. If a thread wins (queue == null is true) then we will assign the queue to be the one we created (we being the winning thread).