[#519] Speed up ConcurrentHashMap#computeIfAbsent#766
Conversation
Codecov Report
@@ Coverage Diff @@
## master #766 +/- ##
============================================
+ Coverage 60.67% 63.09% +2.41%
- Complexity 1901 1954 +53
============================================
Files 239 230 -9
Lines 13031 11346 -1685
Branches 1091 1119 +28
============================================
- Hits 7907 7159 -748
+ Misses 4686 3790 -896
+ Partials 438 397 -41
... and 27 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
eff4784 to
bc7abd0
Compare
|
cc @zuston |
|
Great work! Thank you very much for your contribution. |
| @Override | ||
| public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) { | ||
| V result; | ||
| if (null == (result = get(key))) { |
There was a problem hiding this comment.
@turboFei did you run the benchmark tests provided in the JDK bug report: DeadLockMain.java?
Would you mind post a comparison result here?
There was a problem hiding this comment.
FYI:
import java.util.ArrayList;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Function;
public class DeadlockMainV2 {
private static class ConcurrentHashMapForJDK8<K, V> extends ConcurrentHashMap<K, V> {
@Override
public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) {
V result;
if (null == (result = get(key))) {
result = super.computeIfAbsent(key, mappingFunction);
}
return result;
}
}
static final int MAP_SIZE=20;
static final int THREADS=20;
static final ConcurrentHashMap<Integer,Integer> map = new ConcurrentHashMapForJDK8();
static {
for (int i = 0; i < MAP_SIZE; i++) map.put(i, i);
}
static class TestThread extends Thread {
@Override
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 {
long startTime = System.currentTimeMillis();
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();
long endTime = System.currentTimeMillis();
System.out.println(String.format("Total cost: %d s", (endTime - startTime) / 1000));
}
}
The result is expected.
There was a problem hiding this comment.
Thanks. What's the total cost for the original impl?
There was a problem hiding this comment.
for the original impl, it can not finish in 10 minutes, so I interrupt it.
Just like the class name, DeadLockMain.
### What changes were proposed in this pull request? Speed up ConcurrentHashMap#computeIfAbsent by checking key existence. ### Why are the changes needed? Fix: apache#519. According to the bug mentioned in https://bugs.openjdk.org/browse/JDK-8161372, we could check the key existence before invoking computeIfAbsent, especially for the critical path like ShuffleTaskManager#refreshAppId. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing UT.

What changes were proposed in this pull request?
Speed up ConcurrentHashMap#computeIfAbsent by checking key existence.
Why are the changes needed?
Fix: #519
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing UT.