Skip to content

[#519] Speed up ConcurrentHashMap#computeIfAbsent#766

Merged
zuston merged 2 commits into
apache:masterfrom
turboFei:compute_if_absent
Mar 27, 2023
Merged

[#519] Speed up ConcurrentHashMap#computeIfAbsent#766
zuston merged 2 commits into
apache:masterfrom
turboFei:compute_if_absent

Conversation

@turboFei

@turboFei turboFei commented Mar 27, 2023

Copy link
Copy Markdown
Member

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.

@codecov-commenter

codecov-commenter commented Mar 27, 2023

Copy link
Copy Markdown

Codecov Report

Merging #766 (4ec3a20) into master (567872b) will increase coverage by 2.41%.
The diff coverage is 90.32%.

❗ Current head 4ec3a20 differs from pull request most recent head bc7abd0. Consider uploading reports for the commit bc7abd0 to get more accurate results

@@             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     
Impacted Files Coverage Δ
...java/org/apache/uniffle/common/util/JavaUtils.java 35.71% <16.66%> (-14.29%) ⬇️
...apache/uniffle/storage/common/AbstractStorage.java 51.35% <83.33%> (ø)
...g/apache/hadoop/mapred/SortWriteBufferManager.java 79.89% <100.00%> (ø)
...he/uniffle/client/impl/ShuffleWriteClientImpl.java 33.66% <100.00%> (ø)
...org/apache/uniffle/common/metrics/GRPCMetrics.java 47.88% <100.00%> (ø)
...apache/uniffle/coordinator/ApplicationManager.java 83.88% <100.00%> (ø)
.../apache/uniffle/coordinator/ClientConfManager.java 92.95% <100.00%> (ø)
...a/org/apache/uniffle/coordinator/QuotaManager.java 84.41% <100.00%> (ø)
...ache/uniffle/coordinator/SimpleClusterManager.java 75.31% <100.00%> (ø)
...uniffle/coordinator/metric/CoordinatorMetrics.java 94.28% <100.00%> (ø)
... and 10 more

... and 27 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@turboFei turboFei force-pushed the compute_if_absent branch from eff4784 to bc7abd0 Compare March 27, 2023 03:56
@turboFei

Copy link
Copy Markdown
Member Author

cc @zuston

@zuston

zuston commented Mar 27, 2023

Copy link
Copy Markdown
Member

Great work! Thank you very much for your contribution.

@zuston zuston merged commit ece8d07 into apache:master Mar 27, 2023
@Override
public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) {
V result;
if (null == (result = get(key))) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@turboFei did you run the benchmark tests provided in the JDK bug report: DeadLockMain.java?

Would you mind post a comparison result here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will try it,thanks

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
  }
}

output:
image

The result is expected.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. What's the total cost for the original impl?

@turboFei turboFei May 24, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the original impl, it can not finish in 10 minutes, so I interrupt it.

Just like the class name, DeadLockMain.

xianjingfeng pushed a commit to xianjingfeng/uniffle that referenced this pull request Apr 5, 2023
### 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improvement] Speed up ConcurrentHashMap#computeIfAbsent

4 participants