You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by GitBox <gi...@apache.org> on 2023/01/13 02:27:32 UTC
[GitHub] [incubator-uniffle] zjf2012 opened a new pull request, #480: [Improvement] It's unnecessary to use ConcurrentHashMap for "partitionToBlockIds" in RssShuffleWriter
zjf2012 opened a new pull request, #480:
URL: https://github.com/apache/incubator-uniffle/pull/480
### What changes were proposed in this pull request?
replaced some unnecessary concurrenthashmp with hashmap
### Why are the changes needed?
improve performance
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Tested with repartition workload
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #480: [ISSUE-475][Improvement] It's unnecessary to use ConcurrentHashMap for "partitionToBlockIds" in RssShuffleWriter
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #480:
URL: https://github.com/apache/incubator-uniffle/pull/480#discussion_r1069509476
##########
client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java:
##########
@@ -259,7 +259,7 @@ public SendShuffleDataResult sendShuffleData(String appId, List<ShuffleBlockInfo
}
// maintain the count of blocks that have been sent to the server
- Map<Long, AtomicInteger> blockIdsTracker = Maps.newConcurrentMap();
+ Map<Long, AtomicInteger> blockIdsTracker = Maps.newHashMap();
Review Comment:
You're right. But it's more safe to use `cocurrentHashmap`. If we modify this logic one day, we could forget to change this type to `ConcurrentHashmap`. If you still think it's meaningful to modify this type, I think we could add some comments to explain why we don't use `ConcurrentHashmap` and remind us of this point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zjf2012 commented on pull request #480: [ISSUE-475][Improvement] It's unnecessary to use ConcurrentHashMap for "partitionToBlockIds" in RssShuffleWriter
Posted by GitBox <gi...@apache.org>.
zjf2012 commented on PR #480:
URL: https://github.com/apache/incubator-uniffle/pull/480#issuecomment-1381235646
@advancedxy @jerqi please help review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #480: [ISSUE-475][Improvement] It's unnecessary to use ConcurrentHashMap for "partitionToBlockIds" in RssShuffleWriter
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #480:
URL: https://github.com/apache/incubator-uniffle/pull/480#discussion_r1068876327
##########
client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java:
##########
@@ -259,7 +259,7 @@ public SendShuffleDataResult sendShuffleData(String appId, List<ShuffleBlockInfo
}
// maintain the count of blocks that have been sent to the server
- Map<Long, AtomicInteger> blockIdsTracker = Maps.newConcurrentMap();
+ Map<Long, AtomicInteger> blockIdsTracker = Maps.newHashMap();
Review Comment:
This variable will be accessed by multiple threads in `sendShuffleDataAsync`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zjf2012 commented on a diff in pull request #480: [ISSUE-475][Improvement] It's unnecessary to use ConcurrentHashMap for "partitionToBlockIds" in RssShuffleWriter
Posted by GitBox <gi...@apache.org>.
zjf2012 commented on code in PR #480:
URL: https://github.com/apache/incubator-uniffle/pull/480#discussion_r1068872441
##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/writer/RssShuffleWriter.java:
##########
@@ -223,8 +223,7 @@ private void processShuffleBlockInfos(List<ShuffleBlockInfo> shuffleBlockInfoLis
blockIds.add(blockId);
// update [partition, blockIds], it will be sent to shuffle server
int partitionId = sbi.getPartitionId();
- partitionToBlockIds.putIfAbsent(partitionId, Sets.newConcurrentHashSet());
- partitionToBlockIds.get(partitionId).add(blockId);
+ partitionToBlockIds.computeIfAbsent(partitionId, k -> Sets.newHashSet()).add(blockId);
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zjf2012 commented on a diff in pull request #480: [ISSUE-475][Improvement] It's unnecessary to use ConcurrentHashMap for "partitionToBlockIds" in RssShuffleWriter
Posted by GitBox <gi...@apache.org>.
zjf2012 commented on code in PR #480:
URL: https://github.com/apache/incubator-uniffle/pull/480#discussion_r1068861052
##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/writer/RssShuffleWriter.java:
##########
@@ -223,8 +223,7 @@ private void processShuffleBlockInfos(List<ShuffleBlockInfo> shuffleBlockInfoLis
blockIds.add(blockId);
// update [partition, blockIds], it will be sent to shuffle server
int partitionId = sbi.getPartitionId();
- partitionToBlockIds.putIfAbsent(partitionId, Sets.newConcurrentHashSet());
- partitionToBlockIds.get(partitionId).add(blockId);
+ partitionToBlockIds.computeIfAbsent(partitionId, k -> Sets.newHashSet()).add(blockId);
Review Comment:
good catch. let me modify it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zjf2012 commented on a diff in pull request #480: [ISSUE-475][Improvement] It's unnecessary to use ConcurrentHashMap for "partitionToBlockIds" in RssShuffleWriter
Posted by GitBox <gi...@apache.org>.
zjf2012 commented on code in PR #480:
URL: https://github.com/apache/incubator-uniffle/pull/480#discussion_r1068881739
##########
client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java:
##########
@@ -259,7 +259,7 @@ public SendShuffleDataResult sendShuffleData(String appId, List<ShuffleBlockInfo
}
// maintain the count of blocks that have been sent to the server
- Map<Long, AtomicInteger> blockIdsTracker = Maps.newConcurrentMap();
+ Map<Long, AtomicInteger> blockIdsTracker = Maps.newHashMap();
Review Comment:
no, it's not shared since a new instance is created each time you call the method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zjf2012 commented on a diff in pull request #480: [ISSUE-475][Improvement] It's unnecessary to use ConcurrentHashMap for "partitionToBlockIds" in RssShuffleWriter
Posted by GitBox <gi...@apache.org>.
zjf2012 commented on code in PR #480:
URL: https://github.com/apache/incubator-uniffle/pull/480#discussion_r1070754989
##########
client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java:
##########
@@ -259,7 +259,7 @@ public SendShuffleDataResult sendShuffleData(String appId, List<ShuffleBlockInfo
}
// maintain the count of blocks that have been sent to the server
- Map<Long, AtomicInteger> blockIdsTracker = Maps.newConcurrentMap();
+ Map<Long, AtomicInteger> blockIdsTracker = Maps.newHashMap();
Review Comment:
I just changed back to HashMap with comments to show reason. And from the code logic perspective, we will unlikely to insert/delete entries after dispatching it for sendShuffleDataAsync.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zjf2012 commented on a diff in pull request #480: [ISSUE-475][Improvement] It's unnecessary to use ConcurrentHashMap for "partitionToBlockIds" in RssShuffleWriter
Posted by GitBox <gi...@apache.org>.
zjf2012 commented on code in PR #480:
URL: https://github.com/apache/incubator-uniffle/pull/480#discussion_r1068892211
##########
client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java:
##########
@@ -259,7 +259,7 @@ public SendShuffleDataResult sendShuffleData(String appId, List<ShuffleBlockInfo
}
// maintain the count of blocks that have been sent to the server
- Map<Long, AtomicInteger> blockIdsTracker = Maps.newConcurrentMap();
+ Map<Long, AtomicInteger> blockIdsTracker = Maps.newHashMap();
Review Comment:
I overlooked the CompletableFuture part inside sendShuffleDataAsync. let me rollback change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] codecov-commenter commented on pull request #480: [ISSUE-475][Improvement] It's unnecessary to use ConcurrentHashMap for "partitionToBlockIds" in RssShuffleWriter
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #480:
URL: https://github.com/apache/incubator-uniffle/pull/480#issuecomment-1381246999
# [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/480?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#480](https://codecov.io/gh/apache/incubator-uniffle/pull/480?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ce34d64) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/19a8bac456feb3b5be2000027e4719396300b4a3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (19a8bac) will **decrease** coverage by `0.66%`.
> The diff coverage is `33.33%`.
```diff
@@ Coverage Diff @@
## master #480 +/- ##
============================================
- Coverage 58.78% 58.11% -0.67%
+ Complexity 1704 1485 -219
============================================
Files 206 184 -22
Lines 11471 9629 -1842
Branches 1024 864 -160
============================================
- Hits 6743 5596 -1147
+ Misses 4317 3665 -652
+ Partials 411 368 -43
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/uniffle/client/impl/ShuffleWriteClientImpl.java](https://codecov.io/gh/apache/incubator-uniffle/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NsaWVudC9pbXBsL1NodWZmbGVXcml0ZUNsaWVudEltcGwuamF2YQ==) | `29.82% <33.33%> (ø)` | |
| [...rg/apache/uniffle/server/ShuffleServerMetrics.java](https://codecov.io/gh/apache/incubator-uniffle/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlU2VydmVyTWV0cmljcy5qYXZh) | `97.05% <0.00%> (-0.14%)` | :arrow_down: |
| [...n/java/org/apache/hadoop/mapreduce/MRIdHelper.java](https://codecov.io/gh/apache/incubator-uniffle/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL01SSWRIZWxwZXIuamF2YQ==) | | |
| [...pache/hadoop/mapreduce/task/reduce/RssFetcher.java](https://codecov.io/gh/apache/incubator-uniffle/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3Rhc2svcmVkdWNlL1Jzc0ZldGNoZXIuamF2YQ==) | | |
| [...pache/spark/shuffle/writer/WriteBufferManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvV3JpdGVCdWZmZXJNYW5hZ2VyLmphdmE=) | | |
| [...e/spark/shuffle/reader/RssShuffleDataIterator.java](https://codecov.io/gh/apache/incubator-uniffle/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS9yZWFkZXIvUnNzU2h1ZmZsZURhdGFJdGVyYXRvci5qYXZh) | | |
| [.../hadoop/mapreduce/task/reduce/RssBypassWriter.java](https://codecov.io/gh/apache/incubator-uniffle/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3Rhc2svcmVkdWNlL1Jzc0J5cGFzc1dyaXRlci5qYXZh) | | |
| [...apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java](https://codecov.io/gh/apache/incubator-uniffle/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3YyL2FwcC9Sc3NNUkFwcE1hc3Rlci5qYXZh) | | |
| [...g/apache/hadoop/mapred/SortWriteBufferManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkL1NvcnRXcml0ZUJ1ZmZlck1hbmFnZXIuamF2YQ==) | | |
| [.../java/org/apache/hadoop/mapreduce/RssMRConfig.java](https://codecov.io/gh/apache/incubator-uniffle/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL1Jzc01SQ29uZmlnLmphdmE=) | | |
| ... and [14 more](https://codecov.io/gh/apache/incubator-uniffle/pull/480?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #480: [ISSUE-475][Improvement] It's unnecessary to use ConcurrentHashMap for "partitionToBlockIds" in RssShuffleWriter
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #480:
URL: https://github.com/apache/incubator-uniffle/pull/480#discussion_r1068892517
##########
client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java:
##########
@@ -259,7 +259,7 @@ public SendShuffleDataResult sendShuffleData(String appId, List<ShuffleBlockInfo
}
// maintain the count of blocks that have been sent to the server
- Map<Long, AtomicInteger> blockIdsTracker = Maps.newConcurrentMap();
+ Map<Long, AtomicInteger> blockIdsTracker = Maps.newHashMap();
Review Comment:
> no, it's not shared since a new instance is created each time you call the method.
You can see for more details.
https://github.com/apache/incubator-uniffle/blob/f4048fc9dc11b1da642e6e97e540ea98b57d490c/client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java#L172
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zjf2012 commented on a diff in pull request #480: [ISSUE-475][Improvement] It's unnecessary to use ConcurrentHashMap for "partitionToBlockIds" in RssShuffleWriter
Posted by GitBox <gi...@apache.org>.
zjf2012 commented on code in PR #480:
URL: https://github.com/apache/incubator-uniffle/pull/480#discussion_r1068897770
##########
client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java:
##########
@@ -259,7 +259,7 @@ public SendShuffleDataResult sendShuffleData(String appId, List<ShuffleBlockInfo
}
// maintain the count of blocks that have been sent to the server
- Map<Long, AtomicInteger> blockIdsTracker = Maps.newConcurrentMap();
+ Map<Long, AtomicInteger> blockIdsTracker = Maps.newHashMap();
Review Comment:
rolled back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi merged pull request #480: [ISSUE-475][Improvement] It's unnecessary to use ConcurrentHashMap for "partitionToBlockIds" in RssShuffleWriter
Posted by GitBox <gi...@apache.org>.
jerqi merged PR #480:
URL: https://github.com/apache/incubator-uniffle/pull/480
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] zjf2012 commented on a diff in pull request #480: [ISSUE-475][Improvement] It's unnecessary to use ConcurrentHashMap for "partitionToBlockIds" in RssShuffleWriter
Posted by GitBox <gi...@apache.org>.
zjf2012 commented on code in PR #480:
URL: https://github.com/apache/incubator-uniffle/pull/480#discussion_r1069124026
##########
client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java:
##########
@@ -259,7 +259,7 @@ public SendShuffleDataResult sendShuffleData(String appId, List<ShuffleBlockInfo
}
// maintain the count of blocks that have been sent to the server
- Map<Long, AtomicInteger> blockIdsTracker = Maps.newConcurrentMap();
+ Map<Long, AtomicInteger> blockIdsTracker = Maps.newHashMap();
Review Comment:
I went through some logic and didn't find any update to "blockIdsTracker" (**correct me if I am wrong**) in main thread after "sendShuffleDataAsync" call which runs asynchronously in the threadpool, "dataTransferPool". According to [BlockingQueue](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/BlockingQueue.html ) (used internally by the thread pool), "...actions in a thread prior to placing an object into a BlockingQueue [happen-before](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/package-summary.html#MemoryVisibility) actions subsequent to the access or removal of that element from the BlockingQueue in another thread.".
So, I think we don't need cocurrentHashmap for "blockIdsTracker". And you use "AtomicInteger" as value part of "blockIdsTracker", it's enough to make the updated value visible to other threads in later code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #480: [ISSUE-475][Improvement] It's unnecessary to use ConcurrentHashMap for "partitionToBlockIds" in RssShuffleWriter
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #480:
URL: https://github.com/apache/incubator-uniffle/pull/480#discussion_r1068860255
##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/writer/RssShuffleWriter.java:
##########
@@ -223,8 +223,7 @@ private void processShuffleBlockInfos(List<ShuffleBlockInfo> shuffleBlockInfoLis
blockIds.add(blockId);
// update [partition, blockIds], it will be sent to shuffle server
int partitionId = sbi.getPartitionId();
- partitionToBlockIds.putIfAbsent(partitionId, Sets.newConcurrentHashSet());
- partitionToBlockIds.get(partitionId).add(blockId);
+ partitionToBlockIds.computeIfAbsent(partitionId, k -> Sets.newHashSet()).add(blockId);
Review Comment:
Should we modify `spark2` client, too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org