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?
None of those synchronized
is needed in this case.
BTW synchronized
on this
is bad practice.