javaconcurrencysynchronized

Unsure if there's a need using synchronized with concurrentMap


I have reviewed an implementation of map of maps in java and seen an excessive use of synchronized:

public class NestedMap<K1, K2, V> {
  private Map<K1, Map<K2, V>> data = new ConcurrentHashMap<>();
  
  synchronized public V put(K1 k1, K2 k2, V v) {
    Map<K2, V> map = data.computeIfAbsent(k1, key -> new ConcurrentHashMap<>());
    return map.put(k2, v);
  }

  synchronized public V get(K1 k1, K2 k2) {
    Map<K2, V> map = data.get(k1);
    if (null == map) return null;
    return map.get(k2);
  }

  synchronized public V remove(K1 k1, K2 k2) {
    Map<K2, V> map = data.get(k1);
    if (null == map) return null;
    return map.remove(k2);
  }

  synchronized public void remove(K1 k1) {
    data.remove(k1);
  }

  synchronized public Map<K2, V> get(K1 k1) {
    return this.data.get(k1);
  }

  synchronized public V computeIfAbsent(K1 k1, K2 k2, Function<? super K2, ? extends V> mappingFunction) {
    Map<K2, V> map = data.computeIfAbsent(k1, key -> new ConcurrentHashMap<>());
    return map.computeIfAbsent(k2, mappingFunction);
  }

  synchronized public V putIfAbsent(K1 k1, K2 k2, V v) {
    Map<K2, V> map = data.computeIfAbsent(k1, key -> new ConcurrentHashMap<>());
    return map.putIfAbsent(k2, v);
  }

  synchronized public List<V> getAll() {
    List<V> result = new ArrayList<>();
    for (Map<K2, V> map : data.values()) {
      for (V v : map.values()) {
        result.add(v);
      }
    }
    return result;
  }

  synchronized public List<V> getAll(K1 k1) {
    List<V> result = new ArrayList<>();
    Map<K2, V> map = data.get(k1);
    if (null == map) return result;
    result.addAll(map.values());
    return result;
  }

  synchronized public void clear() {
    data.clear();
  }

  public Map<K1, Map<K2, V>> getData() {
    return data;
  }
}

As my knowledge, synchronized on a method locks itself to enforce only one thread can access it at the same time. But negativity would be the downside of the performance.

I have even pasted it to copilot and it suggested removal of all synchronized, but I think it is wrong.

However, I believe not all methods require synchronized to enforce atomics. Not to mention remove and get, although put contains two atomic operations, I don't think there is a must to use synchronized for all methods. But I was told it is necessary.

To enforce consistence, I would prefer synchronized on this class' caller instead.

Could you commit for this class or idea of optimization as well?


Solution

  • None of those synchronized is needed in this case.

    BTW synchronized on this is bad practice.