JDK-8161372 : ConcurrentHashMap.computeIfAbsent(k,f) locks bin when k present
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.util.concurrent
  • Affected Version: 8,9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2016-07-13
  • Updated: 2019-06-03
  • Resolved: 2016-07-26
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 9
9 b129Fixed
Related Reports
Duplicate :  
Description
FULL PRODUCT VERSION :
java version "1.8.0_92"
Java(TM) SE Runtime Environment (build 1.8.0_92-b14)
Java HotSpot(TM) 64-Bit Server VM (build 25.92-b14, mixed mode)

ADDITIONAL OS VERSION INFORMATION :
OS X 10.11.5

A DESCRIPTION OF THE PROBLEM :
Generally, retrieval operations on ConcurrentHashMap are expected to be non-blocking.

The method computeIfAbsent(k,f) is a non-mutating retrieval operation if the key is already present in the map. That's a common case when the map is used as a lazy-loading cache, which is a common use case for that method.

However, blocking occurs even when the key is already present. 

Ideally, this should be fixed.  If it can't be, then at least the method documentation should declare this behavior.  In online forums, I find considerable misinformation about this, with people thinking that it won't block when the key is already present.

This is copied from the thread dump showing contention:

  java.lang.Thread.State: BLOCKED (on object monitor)
    at java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1674)
    - waiting to lock <0x0000000795f80540> (a java.util.concurrent.ConcurrentHashMap$Node)
    at local.ComputIfAbsentTest.benchComputeAbsent(ComputIfAbsentTest.java:87)



STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
To reproduce, run a test with 20 threads contending for the same (already-present) values in a ConcurrentHashMap.  

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
Expected no blocking.
ACTUAL -
The contention is clearly visible in Flight Recorder data, as well as in thread dumps:

  java.lang.Thread.State: BLOCKED (on object monitor)
    at java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1674)
    - waiting to lock <0x0000000795f80540> (a java.util.concurrent.ConcurrentHashMap$Node)
    at local.ComputIfAbsentTest.benchComputeAbsent(ComputIfAbsentTest.java:87)



REPRODUCIBILITY :
This bug can be reproduced always.

---------- BEGIN SOURCE ----------
package local;
import java.util.ArrayList;
import java.util.concurrent.ConcurrentHashMap;
public class Main {
    static final int MAP_SIZE=20;
    static final int THREADS=20;
    static final ConcurrentHashMap<Integer,Integer> map = new ConcurrentHashMap<>();
    static {
        for (int i = 0; i < MAP_SIZE; i++) map.put(i, i);
    }
    static class TestThread extends Thread {
        public void run() {
            int i=0; int result =0;
            while(result<Integer.MAX_VALUE) {
                i = (i+1) % MAP_SIZE;
                result += map.computeIfAbsent(i, (key)->key+key);
            }
        }
    }
    public static void main(String[]v) throws InterruptedException {
        ArrayList<Thread> threads = new ArrayList<>();
        for (int i=0;i<THREADS;i++) {
            TestThread t = new TestThread();
            threads.add(t);
            t.start();
        }
        threads.get(0).join();
    }
}
---------- END SOURCE ----------

CUSTOMER SUBMITTED WORKAROUND :
The following gives 6x better throughput (in a 20-thread test) for the case that the keys already exist (but is probably slower for the case that they don't):

        public V computeIfAbsent2(K key, Function<? super K, ? extends V> mappingFunction) {
            V value = get(key);
            if (value == null) {
                value = mappingFunction.apply(key);
                put(key, value);
            }
            return value;
        }



Comments
This is too risky to backport before we pick up the JSR166 JCK tests into 8u JDK repo: JDK-8146467. Deferring until the future 8u release.
03-06-2019

There's no one-to-one correspondence between cvs commits and hg commits. Ideally each separate meaningful change would be in a single hg commit, but that's a lot of work for the integrator (me!) so I don't usually bother unless I expect the change to be backported. Usually we shy away from performance backports in concurrent code due to the risk of hard-to-detect concurrency bugs in the backport. When we were working on jdk9, we didn't expect jdk8 to have 10 years of life beyond jdk9 end-of-life, so would have planned differently. All cvs commits are visible in http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurrent/ConcurrentHashMap.java?view=log
30-04-2019

The committed changeset contains the fixes for JDK-8161372 (this one) and JDK-8160751 (keySet.removeAll). We either have to consider to backport both fixes (*and* the follow up bugs for removeAll fix, e.g. JDK-8163353), or separate it during the backports. It is much easier for backports to have one changeset per issue to avoid dealing with this in future.
30-04-2019

Well, the change set claims to be a fix for this issue. Why separate out the remove improvement?
30-04-2019

We have users complaining about computeIfAbsent performance. Checking the first node seems to be simple enough to alleviate the performance loss. Let's consider backporting this to 8u. NOTE: The changeset contains fixes for two issues (dang, let's avoid this in future), so we need to separate only this one.
30-04-2019

Checked this for 8,8u92,9ea on Windows 7 and could confirm the issue as reported by the submitter. Steps to reporudce: ************************* - Run the attached test case(DeadlockMain.java) > javac DeadlockMain.java > java -XX:FlightRecorderOptions=defaultrecording=true,dumponexit=true,dumponexitpath="C:\JavaIncident\Core-libs\" DeadlockMain > JMC > Start JFR Result: ********* OS : Windows 7 64 bit JDK: 8 b132 : Fail 8u92 b31 : Fail 9ea+126 : Fail ======================================================================================================================== Output: ======================================================================================================================== c:\ji\core-libs>javac DeadlockMain.java c:\ji\core-libs>java -XX:FlightRecorderOptions=defaultrecording=true,dumponexit=true,dumponexitpath="c:\ji" DeadlockMain c:\ji\core-libs> ========================================================================================================================= Dump file : hotspot-pid-8788-id-0-2016_07_14_17_39_10.jfr ========================================================================================================================= Moving this issue to JDK for dev engineer's further review.
27-09-2016

It's a tradeoff. If we know that k is present, then it's best to always try read-only traversal first. If we know that k is absent, then it's best to unconditionally lock the bin. Examining just the first element before acquiring the lock is an effective practical compromise.
18-07-2016

It looks like computeIfAbsent (unlike get) locks the bin containing the key. In general, mutating operations are designed to do this, and in the normal case e.g. put() will update the map, in which case unconditionally locking seems reasonable. But in the case of computeIfAbsent the normal case may be that the key is present, and it seems reasonable to optimistically traverse the bin looking for the element in "get" mode and redo if necessary in locked update mode. But this will definitely be slower when the key is absent; it's a tradeoff. Anyways, I suspect we can do better here somehow.
16-07-2016

computeIfAbsent is not a retrieval operation so why would you expect it to be non-blocking? From the docs: ".. Some attempted update operations on this map by other threads may be blocked while computation is in progress, so the computation should be short and simple, and must not attempt to update any other mappings of this map." the corollary to which is that the computation may block if other update operations are in progress. The suggested "workaround" is not an atomic operation.
15-07-2016