-
Notifications
You must be signed in to change notification settings - Fork 134
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.
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
[#493] improvement: replace putIfAbsent
with computeIfAbsent
to avoid performance loss in some critical paths
#876
Conversation
… some critical paths
@@ -241,7 +241,7 @@ protected ShuffleBlockInfo createShuffleBlock(int partitionId, WriterBuffer wb) | |||
|
|||
// it's run in single thread, and is not thread safe | |||
private int getNextSeqNo(int partitionId) { | |||
partitionToSeqNo.putIfAbsent(partitionId, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary.
@@ -544,7 +544,7 @@ public void reportShuffleResult( | |||
groupedPartitions.get(ssi).add(partitionIdx); | |||
} | |||
if (CollectionUtils.isNotEmpty(partitionToBlockIds.get(partitionIdx))) { | |||
partitionReportTracker.putIfAbsent(partitionIdx, 0); | |||
partitionReportTracker.computeIfAbsent(partitionIdx, key -> 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -630,7 +630,7 @@ public Roaring64NavigableMap getShuffleResultForMultiPart(String clientType, | |||
ShuffleServerInfo shuffleServerInfo = entry.getKey(); | |||
Set<Integer> requestPartitions = Sets.newHashSet(); | |||
for (Integer partitionId : entry.getValue()) { | |||
partitionReadSuccess.putIfAbsent(partitionId, 0); | |||
partitionReadSuccess.computeIfAbsent(partitionId, key -> 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -312,7 +312,7 @@ public static Map<Integer, Roaring64NavigableMap> generatePartitionToBitmap( | |||
Roaring64NavigableMap shuffleBitmap, int startPartition, int endPartition) { | |||
Map<Integer, Roaring64NavigableMap> result = Maps.newHashMap(); | |||
for (int partitionId = startPartition; partitionId < endPartition; partitionId++) { | |||
result.putIfAbsent(partitionId, Roaring64NavigableMap.bitmapOf()); | |||
result.computeIfAbsent(partitionId, key -> Roaring64NavigableMap.bitmapOf()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! 👍
Overall LGTM @cchung100m Left minor comments and please fix the CI failure. Thanks~ |
Codecov Report
@@ Coverage Diff @@
## master #876 +/- ##
============================================
+ Coverage 56.58% 58.71% +2.13%
- Complexity 2176 2183 +7
============================================
Files 327 307 -20
Lines 15981 13623 -2358
Branches 1263 1258 -5
============================================
- Hits 9043 7999 -1044
+ Misses 6430 5186 -1244
+ Partials 508 438 -70
... and 23 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for your contribution @cchung100m 🎉
putIfAbsent
with computeIfAbsent
to avoid performance loss in some critical paths
Hi @zuston Thanks for the invitation, I have joined the group. |
Hi @zuston
I have updated some critical paths with the replacement, however, there might be some missing pieces, any suggestions would be appreciated.
What changes were proposed in this pull request?
Replace
putIfAbsent
withcomputeIfAbsent
in some critical pathsWhy are the changes needed?
Fix: #493
Does this PR introduce any user-facing change?
No.
How was this patch tested?
current UT