Synchronized on HashMap value object

Question

I've got a question about synchronization of objects inside a Map (same objects I later change value of). I want to atomically read, do checks and possibly do updates to a value from a map without locking the entire map. Is this a valid way to work with synchronization of objects?

    private final Map<String, AtomicInteger> valueMap = new HashMap<>();

    public Response addValue(@NotNull String key, @NotNull Integer value) {
        AtomicInteger currentValue = valueMap.get(key);
        if (currentValue == null) {
            synchronized (valueMap) {
                // Doublecheck that value hasn't been changed before entering synchronized
                currentValue = valueMap.get(key);
                if (currentValue == null) {
                    currentValue = new AtomicInteger(0);
                    valueMap.put(key, currentValue);
                }
            }
        }
        synchronized (valueMap.get(key)) {
            // Check that value hasn't been changed when changing synchronized blocks
            currentValue = valueMap.get(key);
            if (currentValue.get() + value > MAX_LIMIT) {
                return OVERFLOW;
            }
            currentValue.addAndGet(value);
            return OK;
        }
    }

Answer 1

I fail to see much of a difference between your approach and that of a standard ConcurrentHashMap - asides from the fact that ConcurrentHashMap has been heavily tested, and can be configured for minimal overhead with the exact number of threads you want to run the code with.

In a ConcurrentHashMap, you would use the replace(K key, V old, V new) method to atomically update key to new only when the old value has not changed.

The space savings due to removing all those AtomicIntegers and the time savings due to lower synchronization overhead will probably compensate having to wrap the replace(k, old, new) calls within while-loops:

ConcurrentHashMap<String, Integer> valueMap = 
      new ConcurrentHashMap<>(16, .75f, expectedConcurrentThreadCount);

public Response addToKey(@NotNull String key, @NotNull Integer value) {
     if (value > MAX_LIMIT) {
        // probably should set value to MAX_LIMIT-1 before failing
        return OVERFLOW; 
     }
     boolean updated = false;
     do {
        Integer old = putIfAbsent(key, value);
        if (old == null) {
             // it was absent, and now it has been updated to value: ok
             updated = true;
        } else if (old + value > MAX_LIMIT) {
             // probably should set value to MAX_LIMIT-1 before failing
             return OVERFLOW;
        } else {
             updated = valueMap.replace(key, old, old+value);
        }
     } while (! updated);

     return OK;
}

Also, on the plus side, this code works even if the key was removed after checking it (yours throws an NPE in this case).