You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "zuston (via GitHub)" <gi...@apache.org> on 2023/03/14 11:14:29 UTC
[GitHub] [incubator-uniffle] zuston opened a new pull request, #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
zuston opened a new pull request, #717:
URL: https://github.com/apache/incubator-uniffle/pull/717
### What changes were proposed in this pull request?
Use ByteString#asReadOnlyByteBuffer to reduce the memory allocation in GRPC of `sendShuffleData`
### Why are the changes needed?
Fix: #674
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
--
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] zuston commented on pull request #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#issuecomment-1475513724
> Have you done some benchmark tests?
No. But I tried this patch to run some jobs, and from the flamegraph, the hotspot of memory allocation don't occur.
--
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] zuston commented on a diff in pull request #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#discussion_r1141698207
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriter.java:
##########
@@ -31,15 +33,26 @@ public class LocalFileWriter implements FileWriter, Closeable {
private DataOutputStream dataOutputStream;
private FileOutputStream fileOutputStream;
+ private FileChannel fileChannel;
private long nextOffset;
public LocalFileWriter(File file) throws IOException {
fileOutputStream = new FileOutputStream(file, true);
+ fileChannel = fileOutputStream.getChannel();
// init fsDataOutputStream
dataOutputStream = new DataOutputStream(new BufferedOutputStream(fileOutputStream));
Review Comment:
> Could we ensure that in the code?
Yes. I have checked.
> Or do you think is it necessary to copy bytebuffer to byte[] here?
We'd better not. I think this way is also compatible with netty.
--
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] advancedxy commented on pull request #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#issuecomment-1473206858
> PTAL @jerqi @advancedxy
Sorry for the delay. Let me take a look this weekend. It involves data copy, I need some more time to review this 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] smallzhongfeng commented on pull request #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#issuecomment-1484049646
https://github.com/apache/incubator-uniffle/pull/742 has been merged, let's go ahead.
--
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 #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#issuecomment-1469794513
## [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/717?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 [#717](https://codecov.io/gh/apache/incubator-uniffle/pull/717?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8d52991) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/1fbdfe576f0a164f173cbff9ad83dccccb909ee2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1fbdfe5) will **increase** coverage by `0.91%`.
> The diff coverage is `84.21%`.
```diff
@@ Coverage Diff @@
## master #717 +/- ##
============================================
+ Coverage 60.89% 61.80% +0.91%
+ Complexity 1822 1759 -63
============================================
Files 220 205 -15
Lines 12535 10213 -2322
Branches 1058 1022 -36
============================================
- Hits 7633 6312 -1321
+ Misses 4496 3561 -935
+ Partials 406 340 -66
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/717?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...pache/uniffle/server/ShuffleServerGrpcService.java](https://codecov.io/gh/apache/incubator-uniffle/pull/717?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlU2VydmVyR3JwY1NlcnZpY2UuamF2YQ==) | `0.80% <0.00%> (ø)` | |
| [...apache/uniffle/common/ShufflePartitionedBlock.java](https://codecov.io/gh/apache/incubator-uniffle/pull/717?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9TaHVmZmxlUGFydGl0aW9uZWRCbG9jay5qYXZh) | `71.42% <75.00%> (+0.84%)` | :arrow_up: |
| [.../uniffle/storage/handler/impl/LocalFileWriter.java](https://codecov.io/gh/apache/incubator-uniffle/pull/717?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2hhbmRsZXIvaW1wbC9Mb2NhbEZpbGVXcml0ZXIuamF2YQ==) | `88.46% <83.33%> (-1.54%)` | :arrow_down: |
| [...a/org/apache/uniffle/common/ShuffleDataResult.java](https://codecov.io/gh/apache/incubator-uniffle/pull/717?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9TaHVmZmxlRGF0YVJlc3VsdC5qYXZh) | `86.66% <87.50%> (+4.84%)` | :arrow_up: |
| [...rg/apache/uniffle/server/buffer/ShuffleBuffer.java](https://codecov.io/gh/apache/incubator-uniffle/pull/717?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9idWZmZXIvU2h1ZmZsZUJ1ZmZlci5qYXZh) | `96.63% <100.00%> (+3.25%)` | :arrow_up: |
| [...he/uniffle/server/storage/LocalStorageManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/717?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9zdG9yYWdlL0xvY2FsU3RvcmFnZU1hbmFnZXIuamF2YQ==) | `86.28% <100.00%> (-0.31%)` | :arrow_down: |
| [.../storage/handler/impl/HdfsShuffleWriteHandler.java](https://codecov.io/gh/apache/incubator-uniffle/pull/717?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2hhbmRsZXIvaW1wbC9IZGZzU2h1ZmZsZVdyaXRlSGFuZGxlci5qYXZh) | `87.09% <100.00%> (ø)` | |
| [...le/storage/handler/impl/LocalFileWriteHandler.java](https://codecov.io/gh/apache/incubator-uniffle/pull/717?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2hhbmRsZXIvaW1wbC9Mb2NhbEZpbGVXcml0ZUhhbmRsZXIuamF2YQ==) | `77.08% <100.00%> (ø)` | |
... and [61 files with indirect coverage changes](https://codecov.io/gh/apache/incubator-uniffle/pull/717/indirect-changes?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] zuston commented on pull request #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#issuecomment-1475735133
> > > Have you done some benchmark tests?
> >
> >
> > No. But I tried this patch to run some jobs, and from the flamegraph, the hotspot of memory allocation don't occur.
>
> We would better have some benchmark tests.
It's OK. Let me do some tests later.
--
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] zuston commented on pull request #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#issuecomment-1487844972
It looks `ShufflePartitionedBlock` should also be updated in netty implementation.
--
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 #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#issuecomment-1475728792
> > Have you done some benchmark tests?
>
> No. But I tried this patch to run some jobs, and from the flamegraph, the hotspot of memory allocation don't occur.
We would better have some benchmark tests.
--
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] zuston commented on pull request #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#issuecomment-1471240892
PTAL @jerqi @advancedxy
--
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] zuston commented on a diff in pull request #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#discussion_r1141562822
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriter.java:
##########
@@ -31,15 +33,26 @@ public class LocalFileWriter implements FileWriter, Closeable {
private DataOutputStream dataOutputStream;
private FileOutputStream fileOutputStream;
+ private FileChannel fileChannel;
private long nextOffset;
public LocalFileWriter(File file) throws IOException {
fileOutputStream = new FileOutputStream(file, true);
+ fileChannel = fileOutputStream.getChannel();
// init fsDataOutputStream
dataOutputStream = new DataOutputStream(new BufferedOutputStream(fileOutputStream));
Review Comment:
Emm... Actually, the `FileWriter` interface is not only for writing index data but also writing shuffle data, which is split with 2 methods, but they are exclusive. So only one of `dataOutputStream` and `fileChannel` will be used.
--
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] zuston commented on pull request #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#issuecomment-1479097009
Let's resume this PR until #742 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] advancedxy commented on a diff in pull request #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#discussion_r1142040154
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriter.java:
##########
@@ -31,15 +33,26 @@ public class LocalFileWriter implements FileWriter, Closeable {
private DataOutputStream dataOutputStream;
private FileOutputStream fileOutputStream;
+ private FileChannel fileChannel;
private long nextOffset;
public LocalFileWriter(File file) throws IOException {
fileOutputStream = new FileOutputStream(file, true);
+ fileChannel = fileOutputStream.getChannel();
// init fsDataOutputStream
dataOutputStream = new DataOutputStream(new BufferedOutputStream(fileOutputStream));
Review Comment:
> Yes. I have checked.
I mean that we should refactor the LocalFileWriter to either create an file channel or to use an BufferedOutputStream.
--
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] zuston commented on pull request #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#issuecomment-1498668329
> > It looks `ShufflePartitionedBlock` should also be updated in netty implementation.
>
> So, this pr should be delay after some other netty prs checked in?
I think yes. And I'm evaluating whether we need this 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] advancedxy commented on pull request #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#issuecomment-1498518642
> It looks `ShufflePartitionedBlock` should also be updated in netty implementation.
So, this pr should be delay after some other netty prs checked in?
--
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] advancedxy commented on a diff in pull request #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#discussion_r1141353067
##########
common/src/main/java/org/apache/uniffle/common/ShuffleDataResult.java:
##########
@@ -35,20 +36,26 @@ public ShuffleDataResult(byte[] data) {
}
public ShuffleDataResult(byte[] data, List<BufferSegment> bufferSegments) {
- this.data = data;
+ this(bufferSegments, data == null ? null : ByteBuffer.wrap(data));
+ }
+
+ public ShuffleDataResult(List<BufferSegment> bufferSegments, ByteBuffer data) {
+ this.bufferData = data;
this.bufferSegments = bufferSegments;
Review Comment:
```suggestion
this.bufferSegments = bufferSegments;
this.bufferData = data;
```
##########
common/src/main/java/org/apache/uniffle/common/ShufflePartitionedBlock.java:
##########
@@ -17,31 +17,46 @@
package org.apache.uniffle.common;
-import java.util.Arrays;
+import java.nio.ByteBuffer;
import java.util.Objects;
+import com.google.common.annotations.VisibleForTesting;
+
public class ShufflePartitionedBlock {
private int length;
private long crc;
private long blockId;
private int uncompressLength;
- private byte[] data;
private long taskAttemptId;
+ // Read only byte buffer
+ private ByteBuffer bufferData;
public ShufflePartitionedBlock(
int length,
int uncompressLength,
long crc,
long blockId,
- long taskAttemptId,
- byte[] data) {
+ ByteBuffer data,
+ long taskAttemptId) {
Review Comment:
```suggestion
long taskAttemptId,
ByteBuffer data) {
```
##########
common/src/main/java/org/apache/uniffle/common/ShufflePartitionedBlock.java:
##########
@@ -62,12 +77,12 @@ public boolean equals(Object o) {
return length == that.length
&& crc == that.crc
&& blockId == that.blockId
- && Arrays.equals(data, that.data);
+ && bufferData.equals(that.bufferData);
}
@Override
public int hashCode() {
- return Objects.hash(length, crc, blockId, Arrays.hashCode(data));
+ return Objects.hash(length, crc, blockId, bufferData.hashCode());
Review Comment:
ditto
##########
common/src/main/java/org/apache/uniffle/common/ShuffleDataResult.java:
##########
@@ -35,20 +36,26 @@ public ShuffleDataResult(byte[] data) {
}
public ShuffleDataResult(byte[] data, List<BufferSegment> bufferSegments) {
- this.data = data;
+ this(bufferSegments, data == null ? null : ByteBuffer.wrap(data));
+ }
+
+ public ShuffleDataResult(List<BufferSegment> bufferSegments, ByteBuffer data) {
+ this.bufferData = data;
this.bufferSegments = bufferSegments;
}
public byte[] getData() {
- return data;
+ return bufferData.array();
Review Comment:
please add null check here since bufferData could be null
##########
common/src/main/java/org/apache/uniffle/common/ShufflePartitionedBlock.java:
##########
@@ -62,12 +77,12 @@ public boolean equals(Object o) {
return length == that.length
&& crc == that.crc
&& blockId == that.blockId
- && Arrays.equals(data, that.data);
+ && bufferData.equals(that.bufferData);
Review Comment:
null check
##########
common/src/main/java/org/apache/uniffle/common/ShufflePartitionedBlock.java:
##########
@@ -95,11 +110,7 @@ public void setBlockId(long blockId) {
}
public byte[] getData() {
- return data;
- }
-
- public void setData(byte[] data) {
- this.data = data;
+ return bufferData.array();
Review Comment:
ditto
##########
integration-test/common/src/test/java/org/apache/uniffle/test/MultiStorageHdfsFallbackTest.java:
##########
@@ -51,8 +49,6 @@ public static void setupServers(@TempDir File tmpDir) throws Exception {
@Override
public void makeChaos() {
- assertEquals(1, cluster.getDataNodes().size());
- cluster.stopDataNode(0);
- assertEquals(0, cluster.getDataNodes().size());
+ cluster.shutdown();
Review Comment:
why this change?
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBuffer.java:
##########
@@ -155,17 +156,18 @@ public synchronized ShuffleDataResult getShuffleData(
// todo: if block was flushed, it's possible to get duplicated data
public synchronized ShuffleDataResult getShuffleData(
long lastBlockId, int readBufferSize, Roaring64NavigableMap expectedTaskIds) {
+ LOG.info("Last block id: {}", lastBlockId);
Review Comment:
is this needed?
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriter.java:
##########
@@ -31,15 +33,26 @@ public class LocalFileWriter implements FileWriter, Closeable {
private DataOutputStream dataOutputStream;
private FileOutputStream fileOutputStream;
+ private FileChannel fileChannel;
private long nextOffset;
public LocalFileWriter(File file) throws IOException {
fileOutputStream = new FileOutputStream(file, true);
+ fileChannel = fileOutputStream.getChannel();
// init fsDataOutputStream
dataOutputStream = new DataOutputStream(new BufferedOutputStream(fileOutputStream));
Review Comment:
I'm worried about this, especially the dataOutputStream is buffered.
Did a quick overview of FileChannel, I'm not sure that we can mix writing data to fileChannel and a buffered dataOutputStream
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBuffer.java:
##########
@@ -238,19 +240,12 @@ private int calculateDataLength(List<BufferSegment> bufferSegments) {
return bufferSegment.getOffset() + bufferSegment.getLength();
}
- private void updateShuffleData(List<ShufflePartitionedBlock> readBlocks, byte[] data) {
- int offset = 0;
+ private void updateShuffleData(List<ShufflePartitionedBlock> readBlocks, ByteBuffer bufferData) {
for (ShufflePartitionedBlock block : readBlocks) {
- // fill shuffle data
- try {
- System.arraycopy(block.getData(), 0, data, offset, block.getLength());
- } catch (Exception e) {
- LOG.error("Unexpected exception for System.arraycopy, length["
- + block.getLength() + "], offset["
- + offset + "], dataLength[" + data.length + "]", e);
- throw e;
- }
- offset += block.getLength();
+ ByteBuffer tmp = block.getBufferData().duplicate();
+ tmp.clear();
+ bufferData.put(tmp);
+ tmp.clear();
Review Comment:
double clear? Seems like a bug.
##########
server/src/main/java/org/apache/uniffle/server/storage/LocalStorageManager.java:
##########
@@ -125,10 +127,12 @@ public class LocalStorageManager extends SingleStorageManager {
.localStorageMedia(storageType)
.build();
successCount.incrementAndGet();
+ LOG.info("Initialized path: {}", storagePath);
} catch (Exception e) {
LOG.error("LocalStorage init failed!", e);
} finally {
countDownLatch.countDown();
+ LOG.info("Finished. {}", storagePath);
Review Comment:
is this related?
--
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 #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#issuecomment-1474061682
Have you done some benchmark tests?
--
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] advancedxy commented on a diff in pull request #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#discussion_r1141684325
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriter.java:
##########
@@ -31,15 +33,26 @@ public class LocalFileWriter implements FileWriter, Closeable {
private DataOutputStream dataOutputStream;
private FileOutputStream fileOutputStream;
+ private FileChannel fileChannel;
private long nextOffset;
public LocalFileWriter(File file) throws IOException {
fileOutputStream = new FileOutputStream(file, true);
+ fileChannel = fileOutputStream.getChannel();
// init fsDataOutputStream
dataOutputStream = new DataOutputStream(new BufferedOutputStream(fileOutputStream));
Review Comment:
> Actually, the FileWriter interface is not only for writing index data but also writing shuffle data, which is split with 2 methods, but they are exclusive.
Could we ensure that in the code? Or do you think is it necessary to copy bytebuffer to byte[] here?
--
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 #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #717:
URL: https://github.com/apache/incubator-uniffle/pull/717#issuecomment-1495426404
You can see https://github.com/apache/incubator-uniffle/issues/586
--
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
Re: [PR] [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston closed pull request #717: [#674] improvement(server): use ByteString#asReadOnlyByteBuffer to reduce the memory allocation
URL: https://github.com/apache/incubator-uniffle/pull/717
--
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