Do I need to synchronize ConcurrentMap when adding key only if needed?

Question

I have a ConcurrentMap<String, SomeObject> object. I want to write a method that would return the SomeObject value if it exists, or create a new SomeObject, put it in the Map, and return it if it doesn't exist.

Ideally, I could use ConcurrentMap's putIfAbsent(key, new SomeObject(key)), but that means that I create a new SomeObject(key) each time, which seems very wasteful.

So I resorted to the following code, but am not sure that it's the best way to handle this:

public SomeValue getSomevalue(String key){

  SomeValue result = concurrentMap.get(key);

  if (result != null)
    return result;

  synchronized(concurrentMap){

    SomeValue result = concurrentMap.get(key);

    if (result == null){
      result = new SomeValue(key);
      concurrentMap.put(key, result);
    }

    return result;
  }
}

The code is correct, but you could use putIfAbsent in your synchronized block. It will be more optimal. — Guillaume F., Mar 13 '17 at 04:04
@GuillaumeF. Can you explain what you mean by more optimal? At that point I know for a fact that the key is absent since I check for it inside a synchronized block and I get a null value. — isapir, Mar 13 '17 at 04:17
I meant remove the test inside the synchronized block. Since you already do a test before the synchronized, the probability that your second test returns something else than null is very low. It will be more optimal to use putIfAbsent with a new instance of your object since it will fail very rarely. You can also remove the synchronized block completely. — Guillaume F., Mar 13 '17 at 04:22

user2357112 · Accepted Answer · 2017-03-13T04:37:16.980

6

Ideally, I could use ConcurrentMap's putIfAbsent(key, new SomeObject(key)), but that means that I create a new SomeObject(key) each time, which seems very wasteful.

Then use computeIfAbsent:

concurrentMap.computeIfAbsent(key, SomeObject::new);

Using synchronized with a ConcurrentMap doesn't prevent other threads from performing operations on the map in the middle of the synchronized block. ConcurrentMap doesn't promise to use the map's monitor for synchronization, and neither ConcurrentHashMap nor ConcurrentSkipListMap synchronize on the map object.

Note that the ConcurrentMap interface doesn't promise that the value will only be computed once, or that the value won't be computed if the key is already present. ConcurrentHashMap makes these promises, but ConcurrentSkipListMap doesn't.

edited Mar 13 '17 at 04:37

answered Mar 13 '17 at 04:25

user2357112

260,549
28
431
505

Very nice. I think that this is exactly what I was looking for :) – isapir Mar 13 '17 at 04:36
Thanks for clarifying. I'm using `ConcurrentHashMap` so that works fine for me. – isapir Mar 13 '17 at 04:48
To clarify, since `computeIfAbsent` was added in Java 1.8, for Java 1.7 the code that I posted is the "way to go"? – isapir Mar 13 '17 at 04:50
@Igal: `synchronized` won't protect you. You should use `putIfAbsent` on Java 1.7. You can still pre-check with `get`, if profiling shows it to be faster than using `putIfAbsent` unconditionally. – user2357112 Mar 13 '17 at 04:53
I understand your point about the map's monitor not being used internally by ConcurrentMap and that makes sense, but this is the only point in the code where the map is modified, so synchronizing that point should do the trick. – isapir Mar 13 '17 at 05:00

Do I need to synchronize ConcurrentMap when adding key only if needed?

1 Answers1