Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Improvement] Speed up ConcurrentHashMap#computeIfAbsent #519

Closed
2 of 3 tasks
zuston opened this issue Jan 30, 2023 · 3 comments · Fixed by #766
Closed
2 of 3 tasks

[Improvement] Speed up ConcurrentHashMap#computeIfAbsent #519

zuston opened this issue Jan 30, 2023 · 3 comments · Fixed by #766
Labels
good first issue Good for newcomers

Comments

@zuston
Copy link
Member

zuston commented Jan 30, 2023 •
edited
Loading

Code of Conduct

Search before asking

  • I have searched in the issues and found no similar issues.

What would you like to be improved?

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.

The detailed fix could refer to https://juejin.cn/post/7094561581631012878

How should we improve?

No response

Are you willing to submit PR?

  • Yes I am willing to submit a PR!
@zuston
Copy link
Member Author

zuston commented Jan 30, 2023

cc @kaijchen @advancedxy @jerqi

@zuston zuston added the good first issue Good for newcomers label Jan 30, 2023
@advancedxy
Copy link
Contributor

Ouch, does putIfAbsent has similar issue under JDK 8?

If putIfAbsent doesn't have this problem, I believe we should prefer putIfAbsent for simple cases to improve readability.

Otherwise, I'm opt for this solution.

BTW, https://cwiki.apache.org/confluence/display/HADOOP/Hadoop+Java+Versions JDK 1.8 seems to be lasting for a long time since only hadoop 3.3 supports JDK11 in runtime.

@advancedxy advancedxy mentioned this issue Jan 30, 2023
Closed
3 tasks
@zuston
Copy link
Member Author

zuston commented Jan 31, 2023

Ouch, does putIfAbsent has similar issue under JDK 8?

From its implementation, it also has similar problems.

BTW, https://cwiki.apache.org/confluence/display/HADOOP/Hadoop+Java+Versions JDK 1.8 seems to be lasting for a long time since only hadoop 3.3 supports JDK11 in runtime.

Yes.

@turboFei turboFei mentioned this issue Mar 25, 2023
Merged
turboFei added a commit to turboFei/incubator-uniffle that referenced this issue Mar 27, 2023
@turboFei
[apache#519] Speed up ConcurrentHashMap#computeIfAbsent
4ec3a20
@turboFei turboFei mentioned this issue Mar 27, 2023
Merged
zuston pushed a commit that referenced this issue Mar 27, 2023
[#519] Speed up ConcurrentHashMap#computeIfAbsent (#766)
ece8d07 
### What changes were proposed in this pull request?

Speed up ConcurrentHashMap#computeIfAbsent by checking key existence.

### Why are the changes needed?

Fix: #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.
xianjingfeng pushed a commit to xianjingfeng/incubator-uniffle that referenced this issue Apr 5, 2023
[apache#519] Speed up ConcurrentHashMap#computeIfAbsent (apache#766)
0c1e8c5 
### 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.
rickyma added a commit to rickyma/incubator-uniffle that referenced this issue May 17, 2024
@rickyma
[apache#519][FOLLOWUP] Speed up ConcurrentHashMap#computeIfAbsent
5804c30
@rickyma rickyma mentioned this issue May 17, 2024
Merged
rickyma added a commit to rickyma/incubator-uniffle that referenced this issue May 17, 2024
@rickyma
[apache#519][FOLLOWUP] Speed up ConcurrentHashMap#computeIfAbsent
5b2cfd2
zuston pushed a commit that referenced this issue May 17, 2024
@rickyma
[#519][FOLLOWUP] improvement: Speed up ConcurrentHashMap#computeIfAbs…
c3cbdec 
…ent (#1719)

### What changes were proposed in this pull request?

Speed up ConcurrentHashMap#computeIfAbsent by checking key existence.

### Why are the changes needed?

A followup pr for #519.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Existing UTs.
@SteNicholas SteNicholas mentioned this issue Jan 9, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[#519] Speed up ConcurrentHashMap#computeIfAbsent turboFei/incubator-uniffle
2 participants
@advancedxy @zuston