You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "rickyma (via GitHub)" <gi...@apache.org> on 2024/03/24 09:37:41 UTC
[PR] [#1600] feat(netty): Support for allocating direct memory when reading local files and indexes [incubator-uniffle]
rickyma opened a new pull request, #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601
### What changes were proposed in this pull request?
1. Set `readCapacity` dynamically according to `nettyServerEnabled`.
2. Allocating direct memory when enabling Netty when reading local files and indexes.
### Why are the changes needed?
For https://github.com/apache/incubator-uniffle/issues/1600.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Tested in our env.
--
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] [#1600] feat(netty): Support for allocating direct memory when reading local data files and index files [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601#issuecomment-2017142903
> > But I think the key point is the concurrency of tasks rather than task num.
>
> `Task Num` here means concurrent running tasks. I think it's the same meaning. I'll change the title in the report table.
The peak IO speed and memory capacity/ total shuffle data size also could be included in description.
--
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] [#1600] feat(netty): Support for allocating direct memory when reading local data files and index files [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601#discussion_r1537026995
##########
common/src/main/java/org/apache/uniffle/common/netty/buffer/FileSegmentManagedBuffer.java:
##########
@@ -61,7 +63,8 @@ public ByteBuffer nioByteBuffer() {
FileChannel channel = null;
try {
channel = new RandomAccessFile(file, "r").getChannel();
- ByteBuffer buf = ByteBuffer.allocate(length);
+ ByteBuffer buf =
+ preferDirect ? ByteBuffer.allocateDirect(length) : ByteBuffer.allocate(length);
Review Comment:
Do we need to use the same memory allocator with netty server?
--
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] [#1600] feat(netty): Support for allocating direct memory when reading local data files and index files [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601#issuecomment-2017146042
> > > But I think the key point is the concurrency of tasks rather than task num.
> >
> >
> > `Task Num` here means concurrent running tasks. I think it's the same meaning. I'll change the title in the report table.
>
> The peak IO speed and memory capacity/ total shuffle data size also could be included in description.
I'll submit the full version of the performance report in another PR.
And I found that, the greater the concurrency, the more unstable gRPC becomes. I roughly understand the specific reasons, but it's not easy to solve, nor is it necessary, because we already have Netty. After I upload the Netty report, I think we can set the default serverType to Netty instead of gRPC.
--
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] [#1600] feat(netty): Support for allocating direct memory when reading local data files and index files [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601#issuecomment-2017118681
> The read capability is used for limiting the read concurrency to avoid too much GC. Maybe you can try to remove the read capability.
I don't understand what you mean.
--
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] [#1600] feat(netty): Support for allocating direct memory when reading local data files and index files [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601#issuecomment-2021114097
Closed. I don't think the method `nioByteBuffer` in `FileSegmentManagedBuffer` will be used when enabling Netty. So the PR makes no sense.
--
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] [#1600] feat(netty): Support for allocating direct memory when reading local data files and index files [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on code in PR #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601#discussion_r1537032759
##########
common/src/main/java/org/apache/uniffle/common/netty/buffer/FileSegmentManagedBuffer.java:
##########
@@ -61,7 +63,8 @@ public ByteBuffer nioByteBuffer() {
FileChannel channel = null;
try {
channel = new RandomAccessFile(file, "r").getChannel();
- ByteBuffer buf = ByteBuffer.allocate(length);
+ ByteBuffer buf =
+ preferDirect ? ByteBuffer.allocateDirect(length) : ByteBuffer.allocate(length);
Review Comment:
Here we need a `ByteBuffer` instead of `ByteBuf`, so I think we don't need to use the same allocator with Netty server.
--
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] [#1600] feat(netty): Support for allocating direct memory when reading local data files and index files [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma closed pull request #1601: [#1600] feat(netty): Support for allocating direct memory when reading local data files and index files
URL: https://github.com/apache/incubator-uniffle/pull/1601
--
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] [#1600] feat(netty): Support for allocating direct memory when reading local data files and index files [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601#issuecomment-2017152995
> Total Shuffle Read Size: 6.4TiB
> Peak IO: Can reach up to 3.5GB/s
> Shuffle Server capacity: 140g
Some added. But I think these metrics not necessary in this PR.
This is just for comparison, to see how much improvement Netty can bring over gRPC after merging this PR. As long as we keep the configurations of Netty and gRPC consistent (using the control variates method), we can make the comparison. As for the specific amount of memory and type of disk they use, I don't think it matters much, as long as they use the same ones.
--
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] [#1600] feat(netty): Support for allocating direct memory when reading local data files and index files [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601#issuecomment-2017173499
> After I upload the Netty report, I think we can set the default serverType to Netty instead of gRPC.
I'll submit the report in this week or maybe next week.
--
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] [#1600] feat(netty): Support for allocating direct memory when reading local data files and index files [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601#issuecomment-2017140173
> Task Num
`Task Num` here means concurrent running tasks. I think it's the same meaning. I'll change the title in the report table.
--
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] [#1600] feat(netty): Support for allocating direct memory when reading local data files and index files [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601#issuecomment-2017156865
> > Total Shuffle Read Size: 6.4TiB
> > Peak IO: Can reach up to 3.5GB/s
> > Shuffle Server capacity: 140g
>
> Some added. But I think these metrics not necessary in this PR. This is just for comparison, to see how much improvement Netty can bring over gRPC after merging this PR. As long as we keep the configurations of Netty and gRPC consistent (using the control variates method), we can make the comparison. As for the specific amount of memory and type of disk they use, I don't think it matters much, as long as they use the same ones.
But I don't see the same netty based server comparison report before/after 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
Re: [PR] [#1600] feat(netty): Support for allocating direct memory when reading local data files and index files [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601#issuecomment-2017138758
Thanks for providing the benchmark report! @rickyma But I think the key point is the concurrency of tasks rather than task num.
--
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] [#1600] feat(netty): Support for allocating direct memory when reading local files and indexes [incubator-uniffle]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601#issuecomment-2016752835
## [Codecov](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1601?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `33.33333%` with `10 lines` in your changes are missing coverage. Please review.
> Project coverage is 54.00%. Comparing base [(`32d533d`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/32d533d226346600f9ed945c81bc19bf0c7c2994?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`dab882b`)](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1601?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1601?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../common/netty/buffer/FileSegmentManagedBuffer.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1601?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9uZXR0eS9idWZmZXIvRmlsZVNlZ21lbnRNYW5hZ2VkQnVmZmVyLmphdmE=) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1601?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../org/apache/uniffle/server/ShuffleTaskManager.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1601?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlVGFza01hbmFnZXIuamF2YQ==) | 33.33% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1601?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...he/uniffle/server/buffer/ShuffleBufferManager.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1601?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9idWZmZXIvU2h1ZmZsZUJ1ZmZlck1hbmFnZXIuamF2YQ==) | 66.66% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1601?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #1601 +/- ##
============================================
- Coverage 54.01% 54.00% -0.01%
+ Complexity 2863 2292 -571
============================================
Files 438 360 -78
Lines 24850 16101 -8749
Branches 2114 1471 -643
============================================
- Hits 13423 8696 -4727
+ Misses 10586 6898 -3688
+ Partials 841 507 -334
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1601?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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] [#1600] feat(netty): Support for allocating direct memory when reading local data files and index files [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601#issuecomment-2017169753
> But I don't see the same netty based server comparison report before/after this PR.
As outlined in the title, this PR is solely intended to provide the functionality of reading files through allocating direct memory in Netty mode. There shouldn't be any need for performance report discussions. I posted a simple report just for everyone to more intuitively see the improvement brought about by 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
Re: [PR] [#1600] feat(netty): Support for allocating direct memory when reading local files and indexes [incubator-uniffle]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601#issuecomment-2016758272
## Test Results
2 340 files ±0 2 340 suites ±0 4h 31m 39s :stopwatch: -22s
908 tests ±0 907 :white_check_mark: ±0 1 :zzz: ±0 0 :x: ±0
10 541 runs ±0 10 527 :white_check_mark: ±0 14 :zzz: ±0 0 :x: ±0
Results for commit dab882bf. ± Comparison against base commit 32d533d2.
[test-results]:data:application/gzip;base64,H4sIAB37/2UC/03MTQ6DIBCG4asY1l0AHRR6mWbkJyFVaRBWTe9eNEhZfg/D+yHOL3Ynj4Hfgd4Gsmef+m1yxOTDVoSNXKlC5T0dF4rKaz33rPVJ059e/n38auDQLwVoAxtjiFVi3o4mowJYnVe0GJ+a1Spc0FXP3Ud1WFefyiAGZyn57Dhaq5kRjhmgQgIiAFgrRwTNNEjy/QFieBAbEgEAAA==
--
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] [#1600] feat(netty): Support for allocating direct memory when reading local data files and index files [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1601:
URL: https://github.com/apache/incubator-uniffle/pull/1601#issuecomment-2017079149
The read capability is used for limiting the read concurrency to avoid too much GC. Maybe you can try to remove the read capability.
--
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