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 2022/07/29 05:11:46 UTC
[GitHub] [incubator-uniffle] kaijchen opened a new pull request, #103: [Improvement] Add RssUtils#cloneBitMap()
kaijchen opened a new pull request, #103:
URL: https://github.com/apache/incubator-uniffle/pull/103
### What changes were proposed in this pull request?
1. Add `RssUtils#cloneBitMap()`.
2. Replace `deserializeBitMap(serializeBitMap(bitmap))` by `cloneBitMap(bitmap)`.
### Why are the changes needed?
1. No need to handle `IOException`.
2. Might be more efficient.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
New unit test `RssUtilsTest#testCloneBitmap()`.
--
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 #103: [Improvement] Use OR operation instead of serialization for cloning BitMaps
Posted by GitBox <gi...@apache.org>.
jerqi merged PR #103:
URL: https://github.com/apache/incubator-uniffle/pull/103
--
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 #103: [Improvement] Add RssUtils#cloneBitMap()
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #103:
URL: https://github.com/apache/incubator-uniffle/pull/103#issuecomment-1198894348
# [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/103?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 [#103](https://codecov.io/gh/apache/incubator-uniffle/pull/103?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dba0300) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/ccb39ed95fa7cd8742ce323eb7096ddd1a09827b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ccb39ed) will **decrease** coverage by `0.82%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #103 +/- ##
============================================
- Coverage 56.33% 55.50% -0.83%
+ Complexity 1176 1103 -73
============================================
Files 149 140 -9
Lines 7992 7612 -380
Branches 766 735 -31
============================================
- Hits 4502 4225 -277
+ Misses 3246 3153 -93
+ Partials 244 234 -10
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/103?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...che/uniffle/client/impl/ShuffleReadClientImpl.java](https://codecov.io/gh/apache/incubator-uniffle/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NsaWVudC9pbXBsL1NodWZmbGVSZWFkQ2xpZW50SW1wbC5qYXZh) | `89.69% <100.00%> (+3.28%)` | :arrow_up: |
| [.../java/org/apache/uniffle/common/util/RssUtils.java](https://codecov.io/gh/apache/incubator-uniffle/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi91dGlsL1Jzc1V0aWxzLmphdmE=) | `66.19% <100.00%> (+0.72%)` | :arrow_up: |
| [.../org/apache/uniffle/server/ShuffleTaskManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlVGFza01hbmFnZXIuamF2YQ==) | `64.10% <100.00%> (-0.31%)` | :arrow_down: |
| [.../apache/uniffle/coordinator/ClientConfManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvQ2xpZW50Q29uZk1hbmFnZXIuamF2YQ==) | `91.54% <0.00%> (-1.41%)` | :arrow_down: |
| [.../java/org/apache/spark/shuffle/RssSparkConfig.java](https://codecov.io/gh/apache/incubator-uniffle/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS9Sc3NTcGFya0NvbmZpZy5qYXZh) | | |
| [...e/spark/shuffle/reader/RssShuffleDataIterator.java](https://codecov.io/gh/apache/incubator-uniffle/pull/103/diff?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) | | |
| [...che/spark/shuffle/writer/BufferManagerOptions.java](https://codecov.io/gh/apache/incubator-uniffle/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvQnVmZmVyTWFuYWdlck9wdGlvbnMuamF2YQ==) | | |
| [...k/shuffle/writer/WrappedByteArrayOutputStream.java](https://codecov.io/gh/apache/incubator-uniffle/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvV3JhcHBlZEJ5dGVBcnJheU91dHB1dFN0cmVhbS5qYXZh) | | |
| [...ava/org/apache/spark/shuffle/RssShuffleHandle.java](https://codecov.io/gh/apache/incubator-uniffle/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS9Sc3NTaHVmZmxlSGFuZGxlLmphdmE=) | | |
| [...org/apache/spark/shuffle/RssSparkShuffleUtils.java](https://codecov.io/gh/apache/incubator-uniffle/pull/103/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS9Sc3NTcGFya1NodWZmbGVVdGlscy5qYXZh) | | |
| ... and [3 more](https://codecov.io/gh/apache/incubator-uniffle/pull/103/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?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] kaijchen commented on pull request #103: [Improvement] Add RssUtils#cloneBitMap()
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #103:
URL: https://github.com/apache/incubator-uniffle/pull/103#issuecomment-1199001150
Random ints
<img width="660" alt="image" src="https://user-images.githubusercontent.com/5821159/181714511-8311cf99-f874-443f-88c0-5d64cf94e029.png">
Random longs
<img width="657" alt="image" src="https://user-images.githubusercontent.com/5821159/181714626-6f9a8ced-8898-4271-9265-46eb745525c2.png">
--
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 pull request #103: [Improvement] Add RssUtils#cloneBitMap()
Posted by GitBox <gi...@apache.org>.
jerqi commented on PR #103:
URL: https://github.com/apache/incubator-uniffle/pull/103#issuecomment-1198972740
Could you use some random number to test them again? Because bitmap will compress the ordered number, the size of bitmap will be smaller, we test in more situations, the result will be more persuasive.
--
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] kaijchen commented on pull request #103: [Improvement] Add RssUtils#cloneBitMap()
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #103:
URL: https://github.com/apache/incubator-uniffle/pull/103#issuecomment-1198965147
The leap is even bigger for large BitMap.
<img width="983" alt="screenshot_ccc1ad40-80da-418d-9d17-568a6dc428c9" src="https://user-images.githubusercontent.com/5821159/181707235-06a04e52-c60b-4bc4-a3e7-98c91ec1cad2.png">
--
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 pull request #103: [Improvement] Add RssUtils#cloneBitMap()
Posted by GitBox <gi...@apache.org>.
jerqi commented on PR #103:
URL: https://github.com/apache/incubator-uniffle/pull/103#issuecomment-1198956173
> > Could we have a simple benchmark test?
>
> Performance is not the main concern here. Exception-free is more important.
>
> If you find this change is impacting performance later, you can change the method body to
>
> ```java
> try {
> return deserializeBitMap(serializeBitMap(bitmap));
> } catch(IOException e) {
> // not expected
> }
> ```
>
> Just look at the body of `serializeBitMap` and `deserializeBitMap`, it feels they are more costly and error-prone.
>
> And explicitly throwing RuntimeException is not a good practice, because they are difficult to catch (without catching real RuntimeException).
Sadly, we care the performance here. We need performance test to ensure that the pr won't bring worse performance. Bitmap method `or` will do many operations, it may cost much time, I can't ensure the performance easily.
--
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 pull request #103: [Improvement] Add RssUtils#cloneBitMap()
Posted by GitBox <gi...@apache.org>.
jerqi commented on PR #103:
URL: https://github.com/apache/incubator-uniffle/pull/103#issuecomment-1198923838
Could we have a simple benchmark test?
--
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] kaijchen commented on pull request #103: [Improvement] Add RssUtils#cloneBitMap()
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #103:
URL: https://github.com/apache/incubator-uniffle/pull/103#issuecomment-1198963610
I'll include this test in this PR.
<img width="994" alt="screenshot_c1ceb13e-0655-460a-a50f-552c793b057f" src="https://user-images.githubusercontent.com/5821159/181706321-bf1b371f-e9a5-49de-b208-62c4e546aed4.png">
--
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] kaijchen commented on pull request #103: [Improvement] Use OR operation instead of serialization for cloning BitMaps
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #103:
URL: https://github.com/apache/incubator-uniffle/pull/103#issuecomment-1199034482
Thanks @jerqi for the 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] kaijchen commented on pull request #103: [Improvement] Add RssUtils#cloneBitMap()
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #103:
URL: https://github.com/apache/incubator-uniffle/pull/103#issuecomment-1198943673
> Could we have a simple benchmark test?
Performance is not the main concern here.
Exception-free is more important.
--
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 pull request #103: [Improvement] Add RssUtils#cloneBitMap()
Posted by GitBox <gi...@apache.org>.
jerqi commented on PR #103:
URL: https://github.com/apache/incubator-uniffle/pull/103#issuecomment-1198970114
Great work!
--
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] kaijchen commented on pull request #103: [Improvement] Add RssUtils#cloneBitMap()
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #103:
URL: https://github.com/apache/incubator-uniffle/pull/103#issuecomment-1199011364
> I'll include the following test in this PR.
Removed this test because it might be flaky due to resource sharing in the CI environment.
--
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