JDK-6737839 : improve ConcurrentHashMap.putIfAbsent documentation
  • Type: Enhancement
  • Component: core-libs
  • Sub-Component: java.util.concurrent
  • Affected Version: 7
  • Priority: P4
  • Status: Open
  • Resolution: Unresolved
  • OS: linux
  • CPU: x86
  • Submitted: 2008-08-15
  • Updated: 2010-04-04
Description
A DESCRIPTION OF THE PROBLEM :
ConcurrentHashMap.putIfAbsent holds a lock for the duration of the compound action it's described in terms of, though my experience suggests that people assume that the "get" part is done without locking, and a lock is only taken if an insertion appears necessary.

also, if calling code is expected to make the optimistic call to ConcurrentHashMap.get in cases where it's likely the mapping is already present, it would be worth mentioning this in the documentation.

here's a suggested patch:

--- ConcurrentHashMap.java	2007-10-31 14:20:31.000000000 -0700
+++ ConcurrentHashMap.java	2008-08-13 21:28:08.000000000 -0700
@@ -912,7 +912,27 @@
     }
 
     /**
-     * {@inheritDoc}
+     * If the specified key is not already associated
+     * with a value, associate it with the given value.
+     * This is equivalent to
+     * <pre>
+     *   if (!map.containsKey(key))
+     *       return map.put(key, value);
+     *   else
+     *       return map.get(key);</pre>
+     * except that the action is performed atomically, and with a lock
+     * held for the duration of the action.
+     *
+     * The recommended idiom is thus
+     * <pre>
+     *   if (map.get(key) == null)
+     *       map.putIfAbsent(key, value);</pre>
+     * if the key is likely to have already been associated with a value
+     * because the normal case will then involve no locking.
+     *
+     * @param key key with which the specified value is to be associated
+     * @param value value to be associated with the specified key
+     *
      *
      * @return the previous value associated with the specified key,
      *         or <tt>null</tt> if there was no mapping for the key


EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
see diff.
ACTUAL -
see diff.

URL OF FAULTY DOCUMENTATION :
http://java.sun.com/javase/6/docs/api/java/util/concurrent/ConcurrentHashMap.html
Comments
EVALUATION There are two components to this CR: 1. Document the use of a lock by putIfAbsent: + * except that the action is performed atomically, and with a lock + * held for the duration of the action. The use of the lock is an implementation detail so at best it would be documented in the form "In this implementation ...". But saying a lock is used does not provide sufficient information, as there is no indication in the suggested wording as to the scope of the lock involved. The actual lock is a per-segment lock, so only part of the CHM is locked during the operation. But I would be very reluctant to document that level of detail in the class documentation as it might preclude changes to the locking strategy. 2. If calling code is expected to make the optimistic call to ConcurrentHashMap.get in cases where it's likely the mapping is already present, it would be worth mentioning this in the documentation. In usage like the Memoizer pattern, then main optimization in using get() first is to avoid the construction overhead of the object to be stored in the map. Even then this is done by storing a "lighweight" object in the map itself and then generating the real object elsewhere - as the construction can still be performed by more than one thread. Unless the map is extremely heavily contended on the segment in question, then the overhead of the locking in putIfAbsent is minimal. However example usage (which is lacking in ConcurrentMap and ConcurrentHashMap) never hurts. I will forward this RFE to the JSR-166 "maintenance" group for consideration for OpenJDK.
18-08-2008