Does ConcurrentHashMap need synchronization when incrementing its values?

Question

I know ConcurrentHashMap is thread-safe e.g.putIfAbsent,Replace etc., but I was wondering, is a block of code like the one below safe?

if (accumulator.containsKey(key)) { //accumulator is a ConcurrentHashMap
    accumulator.put(key, accumulator.get(key)+1); 
} else {
    accumulator.put(key, 0); 
}

Keep in mind that the accumulator value for a key may be asked by two different threads simultaneously, which would cause a problem in a normal HashMap. So do I need something like this?

ConcurrentHashMap<Integer,Object> locks;
...
locks.putIfAbsent(key,new Object());
synchronized(locks.get(key)) {
    if (accumulator.containsKey(key)) { 
        accumulator.put(key, accumulator.get(key)+1); 
    } else {
        accumulator.put(key, 0); 
    }
}

Answer 1

if (accumulator.containsKey(key)) { //accumulator is a ConcurrentHashMap
    accumulator.put(key, accumulator.get(key)+1); 
} else {
    accumulator.put(key, 0); 
}

No, this code is not thread-safe; accumulator.get(key) can be changed in between the get and the put, or the entry can be added between the containsKey and the put. If you're in Java 8, you can write accumulator.compute(key, (k, v) -> (v == null) ? 0 : v + 1), or any of the many equivalents, and it'll work. If you're not, the thing to do is write something like

while (true) {
  Integer old = accumulator.get(key);
  if (old == null) {
    if (accumulator.putIfAbsent(key, 0) == null) {
      // note: it's a little surprising that you want to put 0 in this case,
      // are you sure you don't mean 1?
      break;
    }
  } else if (accumulator.replace(key, old, old + 1)) {
    break;
  }
}

...which loops until it manages to make the atomic swap. This sort of loop is pretty much how you have to do it: it's how AtomicInteger works, and what you're asking for is AtomicInteger across many keys.

Alternately, you can use a library: e.g. Guava has AtomicLongMap and ConcurrentHashMultiset, which also do things like this.