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/22 13:28:35 UTC
[GitHub] [incubator-uniffle] summaryzb opened a new pull request, #67: [Bugfix]Fix memory leak & Improve buffer management
summaryzb opened a new pull request, #67:
URL: https://github.com/apache/incubator-uniffle/pull/67
### What changes were proposed in this pull request?
the behavior of buffer management and memory allocate
1.when adding a huge record to a partition where the buffer is not initialized, we only allocate `bufferSegmentSize` memory, however the release the real huge size memory after the buffer is sent
2.byte array will be fully used as possible to save memory
### Why are the changes needed?
fix memory leak & improve buffer management
### Does this PR introduce _any_ user-facing change?
no
### How was this patch tested?
pass testing
--
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] colinmjj commented on pull request #67: [Bugfix]Fix memory leak & Improve buffer management
Posted by GitBox <gi...@apache.org>.
colinmjj commented on PR #67:
URL: https://github.com/apache/incubator-uniffle/pull/67#issuecomment-1193480618
@summaryzb can you describe more detail about how to save memory for buffer management?
--
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 #67: [Bugfix]Fix memory leak & Improve buffer management
Posted by GitBox <gi...@apache.org>.
jerqi commented on PR #67:
URL: https://github.com/apache/incubator-uniffle/pull/67#issuecomment-1206152789
@summaryzb Do you want to continue pr?
--
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] summaryzb commented on pull request #67: [Bugfix]Fix memory leak & Improve buffer management
Posted by GitBox <gi...@apache.org>.
summaryzb commented on PR #67:
URL: https://github.com/apache/incubator-uniffle/pull/67#issuecomment-1208075717
> @summaryzb Do you want to continue pr?
yeah I'll move on this pr, sorry for reply not in time
--
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] summaryzb commented on pull request #67: [Bugfix]Fix memory leak & Improve buffer management
Posted by GitBox <gi...@apache.org>.
summaryzb commented on PR #67:
URL: https://github.com/apache/incubator-uniffle/pull/67#issuecomment-1208084133
> @summaryzb can you describe more detail about how to save memory for buffer management? And I prefer to split this PR into 2 PRs for 'Bug fix' and 'Improvement'
follow this suggesting, 'Bug fix' and 'Improvement' is on the way
--
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] summaryzb commented on pull request #67: [Bugfix]Fix memory leak & Improve buffer management
Posted by GitBox <gi...@apache.org>.
summaryzb commented on PR #67:
URL: https://github.com/apache/incubator-uniffle/pull/67#issuecomment-1208159954
fix memory leak:https://github.com/apache/incubator-uniffle/pull/145
--
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] summaryzb commented on pull request #67: [Bugfix]Fix memory leak & Improve buffer management
Posted by GitBox <gi...@apache.org>.
summaryzb commented on PR #67:
URL: https://github.com/apache/incubator-uniffle/pull/67#issuecomment-1208073769
> @summaryzb can you describe more detail about how to save memory for buffer management? And I prefer to split this PR into 2 PRs for 'Bug fix' and 'Improvement'
previously, we add `WrappedBuffer` to `buffers` in `WriterBuffer` class, but the byte[] in `WrappedBuffer` is not fully used.
for example, the length of every record is 2k, every time we add a record, we create a buffer with 3k length by default and wrap the previous buffer as `WrappedBuffer` which resulting in wasting 1k memory in every `WrappedBuffer`.
--
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 #67: [Bugfix]Fix memory leak & Improve buffer management
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #67:
URL: https://github.com/apache/incubator-uniffle/pull/67#issuecomment-1195115269
# [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/67?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 [#67](https://codecov.io/gh/apache/incubator-uniffle/pull/67?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (93c5ea1) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/174e6bb7c65556de51fd74dacca6d44469a36375?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (174e6bb) will **decrease** coverage by `0.94%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #67 +/- ##
============================================
- Coverage 56.41% 55.47% -0.95%
+ Complexity 1173 1098 -75
============================================
Files 149 140 -9
Lines 7968 7593 -375
Branches 761 731 -30
============================================
- Hits 4495 4212 -283
+ Misses 3231 3147 -84
+ Partials 242 234 -8
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/67?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...storage/handler/impl/DataSkippableReadHandler.java](https://codecov.io/gh/apache/incubator-uniffle/pull/67/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-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2hhbmRsZXIvaW1wbC9EYXRhU2tpcHBhYmxlUmVhZEhhbmRsZXIuamF2YQ==) | `81.25% <0.00%> (-3.13%)` | :arrow_down: |
| [...org/apache/uniffle/server/ShuffleFlushManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/67/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-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlRmx1c2hNYW5hZ2VyLmphdmE=) | `76.70% <0.00%> (-1.71%)` | :arrow_down: |
| [.../apache/uniffle/coordinator/ClientConfManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/67/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: |
| [...pache/spark/shuffle/writer/WriteBufferManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/67/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-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvV3JpdGVCdWZmZXJNYW5hZ2VyLmphdmE=) | | |
| [.../org/apache/spark/shuffle/writer/WriterBuffer.java](https://codecov.io/gh/apache/incubator-uniffle/pull/67/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-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvV3JpdGVyQnVmZmVyLmphdmE=) | | |
| [...org/apache/spark/shuffle/RssSparkShuffleUtils.java](https://codecov.io/gh/apache/incubator-uniffle/pull/67/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) | | |
| [...ava/org/apache/spark/shuffle/RssShuffleHandle.java](https://codecov.io/gh/apache/incubator-uniffle/pull/67/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=) | | |
| [...e/spark/shuffle/reader/RssShuffleDataIterator.java](https://codecov.io/gh/apache/incubator-uniffle/pull/67/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/67/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==) | | |
| [...org/apache/spark/shuffle/writer/AddBlockEvent.java](https://codecov.io/gh/apache/incubator-uniffle/pull/67/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-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvQWRkQmxvY2tFdmVudC5qYXZh) | | |
| ... and [2 more](https://codecov.io/gh/apache/incubator-uniffle/pull/67/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] summaryzb closed pull request #67: [Bugfix]Fix memory leak & Improve buffer management
Posted by GitBox <gi...@apache.org>.
summaryzb closed pull request #67: [Bugfix]Fix memory leak & Improve buffer management
URL: https://github.com/apache/incubator-uniffle/pull/67
--
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] colinmjj commented on a diff in pull request #67: [Bugfix]Fix memory leak & Improve buffer management
Posted by GitBox <gi...@apache.org>.
colinmjj commented on code in PR #67:
URL: https://github.com/apache/incubator-uniffle/pull/67#discussion_r928360881
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -117,29 +117,16 @@ public List<ShuffleBlockInfo> addRecord(int partitionId, Object key, Object valu
return null;
}
List<ShuffleBlockInfo> result = Lists.newArrayList();
- if (buffers.containsKey(partitionId)) {
- WriterBuffer wb = buffers.get(partitionId);
- if (wb.askForMemory(serializedDataLength)) {
- if (serializedDataLength > bufferSegmentSize) {
- requestMemory(serializedDataLength);
- } else {
- requestMemory(bufferSegmentSize);
- }
- }
- wb.addRecord(serializedData, serializedDataLength);
- if (wb.getMemoryUsed() > bufferSize) {
- result.add(createShuffleBlock(partitionId, wb));
- copyTime += wb.getCopyTime();
- buffers.remove(partitionId);
- LOG.debug("Single buffer is full for shuffleId[" + shuffleId
- + "] partition[" + partitionId + "] with memoryUsed[" + wb.getMemoryUsed()
- + "], dataLength[" + wb.getDataLength() + "]");
- }
- } else {
- requestMemory(bufferSegmentSize);
- WriterBuffer wb = new WriterBuffer(bufferSegmentSize);
- wb.addRecord(serializedData, serializedDataLength);
Review Comment:
For memory leak, how about:
```
int bufferSize = bufferSegmentSize
if (serializedDataLength > bufferSize) {
bufferSize = serializedDataLength
}
requestMemory(bufferSize);
WriterBuffer wb = new WriterBuffer(bufferSize);
..........
..........
```
--
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 #67: [Bugfix]Fix memory leak & Improve buffer management
Posted by GitBox <gi...@apache.org>.
jerqi commented on PR #67:
URL: https://github.com/apache/incubator-uniffle/pull/67#issuecomment-1193512798
If this is a bug fix, Could we add some cases ?
--
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] summaryzb commented on pull request #67: [Bugfix]Fix memory leak & Improve buffer management
Posted by GitBox <gi...@apache.org>.
summaryzb commented on PR #67:
URL: https://github.com/apache/incubator-uniffle/pull/67#issuecomment-1208082657
> If this is a bug fix, Could we add some cases ?
follow this suggesting
--
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] summaryzb commented on pull request #67: [Bugfix]Fix memory leak & Improve buffer management
Posted by GitBox <gi...@apache.org>.
summaryzb commented on PR #67:
URL: https://github.com/apache/incubator-uniffle/pull/67#issuecomment-1212867211
Improvement of writer buffer to save memory:https://github.com/apache/incubator-uniffle/pull/157
--
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] summaryzb commented on pull request #67: [Bugfix]Fix memory leak & Improve buffer management
Posted by GitBox <gi...@apache.org>.
summaryzb commented on PR #67:
URL: https://github.com/apache/incubator-uniffle/pull/67#issuecomment-1208161695
> fix memory leak:#145
I'll post the improvement pr after this pr merged
--
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] summaryzb commented on a diff in pull request #67: [Bugfix]Fix memory leak & Improve buffer management
Posted by GitBox <gi...@apache.org>.
summaryzb commented on code in PR #67:
URL: https://github.com/apache/incubator-uniffle/pull/67#discussion_r940163020
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -117,29 +117,16 @@ public List<ShuffleBlockInfo> addRecord(int partitionId, Object key, Object valu
return null;
}
List<ShuffleBlockInfo> result = Lists.newArrayList();
- if (buffers.containsKey(partitionId)) {
- WriterBuffer wb = buffers.get(partitionId);
- if (wb.askForMemory(serializedDataLength)) {
- if (serializedDataLength > bufferSegmentSize) {
- requestMemory(serializedDataLength);
- } else {
- requestMemory(bufferSegmentSize);
- }
- }
- wb.addRecord(serializedData, serializedDataLength);
- if (wb.getMemoryUsed() > bufferSize) {
- result.add(createShuffleBlock(partitionId, wb));
- copyTime += wb.getCopyTime();
- buffers.remove(partitionId);
- LOG.debug("Single buffer is full for shuffleId[" + shuffleId
- + "] partition[" + partitionId + "] with memoryUsed[" + wb.getMemoryUsed()
- + "], dataLength[" + wb.getDataLength() + "]");
- }
- } else {
- requestMemory(bufferSegmentSize);
- WriterBuffer wb = new WriterBuffer(bufferSegmentSize);
- wb.addRecord(serializedData, serializedDataLength);
Review Comment:
it's almost the same as my implementation, however if we use `serializedDataLength` as the construct parameter of `WriterBuffer`, the following record added to this partition will compare length with the first record `serializedDataLength`, that's not a elegant way
--
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