Does a ConcurrentHashMap need to be wrapped in a synchronized block?

Question

Do all non-retreival operations on a ConcurrentHashMap (put(), remove() etc.) need to be wrapped in a synchronized(this) block? I understand that all of these operations are thread-safe, so is there any real benefit/need in doing so? The only operations used are put() and remove().

protected final Map<String, String> mapDataStore = new ConcurrentHashMap<String, String>();

public void updateDataStore(final String key, final String value) {
    ...
    synchronized (this) {
        mapDataStore.put(key, value);
    }
    ...
}

Solution 1

No, you are losing the benefits of ConcurrentHashMap by doing that. You may as well be using a HashMap with synchronized or synchronizedMap() to lock the whole table (which is what you do when wrapping operations in synchronized, since the monitor implied is the entire object instance.)

The purpose of ConcurrentHashMap is to increase the throughput of your concurrent code by allowing concurrent read/writes on the table without locking the entire table. The table supports this internally by using lock striping (multiple locks instead of one, with each lock assigned to a set of hash buckets - see Java Concurrency in Practice by Goetz et al).

Once you are using ConcurrentHashMap, all standard map methods (put(), remove(), etc.) become atomic by virtue of the lock striping etc. in the implementation. The only tradeoffs are that methods like size() and isEmpty() may not necessarily return accurate results, since the only way they could would be for all operations to lock the whole table.

The ConcurrentMap interface interface also adds new atomic compound operations like putIfAbsent() (put something only if it the key is not already in the map), remove() accepting both key and value (remove an entry only if its value equals a parameter you pass), etc. These operations used to require locking the whole table because they needed two method calls to accomplish (e.g. putIfAbsent() would need calls to both containsKey() and put(), wrapped inside one synchronized block, if you were using a standard Map implementation.) Once again, you gain greater throughput using these methods, by avoiding locking the entire table.

Solution 2

Synchronizing those operations has no benefit here - it actually degrades performance if you don't need synchronization.

The reason ConcurrentHashMap was created, is that synchronized maps (either implemented by hand like in the question or instantiated in the usual way with Collections.synchronizedMap(map)) show bad performance when accessed by many threads. Put and get operations are blocking, so all other threads have to wait and can't access the map concurrently. The ConcurrentHashMap - as the name suggest - allows concurrent access on the other hand. You lose this benefit if you add synchronization.

Comments

[Gaurav]

What if a code-block contain both containsKey() and put() method? Should that be synchronized or not?

[kapex]

@gaurav That should probably be synchonized. But instead of synchronize I would rather use the methods putIfAbsent or computeIfAbsent (which both are atomic operations for ConcurrentMap).

[Gaurav]

2: And what could the option if one is using Java 1.7?