You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "pan3793 (via GitHub)" <gi...@apache.org> on 2024/12/31 15:11:16 UTC
[PR] [CELEBORN-1815] Support UnpooledByteBufAllocator [celeborn]
pan3793 opened a new pull request, #3043:
URL: https://github.com/apache/celeborn/pull/3043
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
This PR introduces a configuration `celeborn.network.memory.allocator.pooled` to allow users to disable `PooledByteBufAllocator` globally and always use `UnpooledByteBufAllocator`.
### Why are the changes needed?
In some extreme cases, the Netty's `PooledByteBufAllocator` might have tons of 4MiB chunks but only a few sizes of the capacity are used by the real data(see https://github.com/apache/celeborn/pull/3018), for cases that stability is important than performance, it's desirable to allow users to disable the `PooledByteBufAllocator` globally.
### Does this PR introduce _any_ user-facing change?
Add a new feature, disabled by default.
### How was this patch tested?
Pass UT to ensure correctness. Performance and memory impact need to be verified in the production scale cluster.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1815] Support UnpooledByteBufAllocator [celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #3043:
URL: https://github.com/apache/celeborn/pull/3043#discussion_r1900160567
##########
client-flink/common/src/main/java/org/apache/celeborn/plugin/flink/network/FlinkTransportClientFactory.java:
##########
@@ -45,7 +45,7 @@ public FlinkTransportClientFactory(
TransportContext context, List<TransportClientBootstrap> bootstraps, int bufferSizeBytes) {
super(context, bootstraps);
bufferSuppliers = JavaUtils.newConcurrentHashMap();
- this.pooledAllocator = new UnpooledByteBufAllocator(true);
+ this.allocator = new UnpooledByteBufAllocator(true);
Review Comment:
Interesting... Flink client overrides it to forcibly use UnpooledByteBufAllocator
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1815] Support UnpooledByteBufAllocator [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #3043:
URL: https://github.com/apache/celeborn/pull/3043#discussion_r1900704030
##########
client-flink/common/src/main/java/org/apache/celeborn/plugin/flink/network/FlinkTransportClientFactory.java:
##########
@@ -45,7 +45,7 @@ public FlinkTransportClientFactory(
TransportContext context, List<TransportClientBootstrap> bootstraps, int bufferSizeBytes) {
super(context, bootstraps);
bufferSuppliers = JavaUtils.newConcurrentHashMap();
- this.pooledAllocator = new UnpooledByteBufAllocator(true);
+ this.allocator = new UnpooledByteBufAllocator(true);
Review Comment:
This is related to this PR. The Flink client's memory is quite limited.
https://github.com/apache/celeborn/pull/1324
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1815] Support UnpooledByteBufAllocator [celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 closed pull request #3043: [CELEBORN-1815] Support UnpooledByteBufAllocator
URL: https://github.com/apache/celeborn/pull/3043
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1815] Support UnpooledByteBufAllocator [celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #3043:
URL: https://github.com/apache/celeborn/pull/3043#discussion_r1900158336
##########
common/src/main/java/org/apache/celeborn/common/network/util/NettyMemoryMetrics.java:
##########
@@ -69,71 +67,87 @@ public class NettyMemoryMetrics {
}
public NettyMemoryMetrics(
- PooledByteBufAllocator pooledAllocator,
+ ByteBufAllocator allocator,
String metricPrefix,
boolean verboseMetricsEnabled,
AbstractSource source,
Map<String, String> labels) {
- this.pooledAllocator = pooledAllocator;
+ this.allocator = allocator;
this.metricPrefix = metricPrefix;
this.verboseMetricsEnabled = verboseMetricsEnabled;
this.source = source;
this.labels = labels;
- registerMetrics(this.pooledAllocator);
+ registerMetrics();
}
- private void registerMetrics(PooledByteBufAllocator allocator) {
- PooledByteBufAllocatorMetric pooledAllocatorMetric = allocator.metric();
-
+ private void registerMetrics() {
// Register general metrics.
if (source != null) {
- logger.debug("setup netty metrics");
- source.addGauge(
- MetricRegistry.name(metricPrefix, "usedHeapMemory"),
- labels,
- pooledAllocatorMetric::usedHeapMemory);
- source.addGauge(
- MetricRegistry.name(metricPrefix, "usedDirectMemory"),
- labels,
- pooledAllocatorMetric::usedDirectMemory);
- source.addGauge(
- MetricRegistry.name(metricPrefix, "numHeapArenas"),
- labels,
- pooledAllocatorMetric::numHeapArenas);
- source.addGauge(
- MetricRegistry.name(metricPrefix, "numDirectArenas"),
- labels,
- pooledAllocatorMetric::numDirectArenas);
- source.addGauge(
- MetricRegistry.name(metricPrefix, "tinyCacheSize"),
- labels,
- pooledAllocatorMetric::tinyCacheSize);
- source.addGauge(
- MetricRegistry.name(metricPrefix, "smallCacheSize"),
- labels,
- pooledAllocatorMetric::smallCacheSize);
- source.addGauge(
- MetricRegistry.name(metricPrefix, "normalCacheSize"),
- labels,
- pooledAllocatorMetric::normalCacheSize);
- source.addGauge(
- MetricRegistry.name(metricPrefix, "numThreadLocalCaches"),
- labels,
- pooledAllocatorMetric::numThreadLocalCaches);
- source.addGauge(
- MetricRegistry.name(metricPrefix, "chunkSize"), labels, pooledAllocatorMetric::chunkSize);
- if (verboseMetricsEnabled) {
- int directArenaIndex = 0;
- for (PoolArenaMetric metric : pooledAllocatorMetric.directArenas()) {
- registerArenaMetric(metric, "directArena" + directArenaIndex);
- directArenaIndex++;
- }
+ if (allocator instanceof UnpooledByteBufAllocator) {
+ logger.debug("Setup netty metrics for UnpooledByteBufAllocator");
+ ByteBufAllocatorMetric unpooledMetric = ((UnpooledByteBufAllocator) allocator).metric();
+ source.addGauge(
+ MetricRegistry.name(metricPrefix, "usedHeapMemory"),
+ labels,
+ unpooledMetric::usedHeapMemory);
+ source.addGauge(
+ MetricRegistry.name(metricPrefix, "usedDirectMemory"),
+ labels,
+ unpooledMetric::usedDirectMemory);
Review Comment:
UnpooledByteBufAllocator only supports these two metrics
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1815] Support UnpooledByteBufAllocator [celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3043:
URL: https://github.com/apache/celeborn/pull/3043#issuecomment-2567086125
## [Codecov](https://app.codecov.io/gh/apache/celeborn/pull/3043?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 `28.39506%` with `58 lines` in your changes missing coverage. Please review.
> Project coverage is 32.67%. Comparing base [(`fde6365`)](https://app.codecov.io/gh/apache/celeborn/commit/fde6365f68ce8b589bcc639e96476ed618249257?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`b3d569a`)](https://app.codecov.io/gh/apache/celeborn/commit/b3d569a3ee6685bb6ba17b088a8d8b7674531fb1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 10 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/celeborn/pull/3043?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...leborn/common/network/util/NettyMemoryMetrics.java](https://app.codecov.io/gh/apache/celeborn/pull/3043?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fceleborn%2Fcommon%2Fnetwork%2Futil%2FNettyMemoryMetrics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jZWxlYm9ybi9jb21tb24vbmV0d29yay91dGlsL05ldHR5TWVtb3J5TWV0cmljcy5qYXZh) | 0.00% | [42 Missing :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/3043?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...pache/celeborn/common/network/util/NettyUtils.java](https://app.codecov.io/gh/apache/celeborn/pull/3043?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fceleborn%2Fcommon%2Fnetwork%2Futil%2FNettyUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jZWxlYm9ybi9jb21tb24vbmV0d29yay91dGlsL05ldHR5VXRpbHMuamF2YQ==) | 40.75% | [9 Missing and 7 partials :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/3043?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 @@
## main #3043 +/- ##
==========================================
- Coverage 32.68% 32.67% -0.00%
==========================================
Files 336 336
Lines 20032 20053 +21
Branches 1792 1796 +4
==========================================
+ Hits 6546 6551 +5
- Misses 13123 13137 +14
- Partials 363 365 +2
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/celeborn/pull/3043?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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1815] Support UnpooledByteBufAllocator [celeborn]
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on code in PR #3043:
URL: https://github.com/apache/celeborn/pull/3043#discussion_r1900185543
##########
common/src/main/java/org/apache/celeborn/common/network/util/NettyUtils.java:
##########
@@ -103,53 +104,59 @@ public static String getRemoteAddress(Channel channel) {
* released by the executor thread rather than the event loop thread. Those thread-local caches
* actually delay the recycling of buffers, leading to larger memory usage.
*/
- private static PooledByteBufAllocator createPooledByteBufAllocator(
- boolean allowDirectBufs, boolean allowCache, int numCores) {
- if (numCores == 0) {
- numCores = Runtime.getRuntime().availableProcessors();
+ private static ByteBufAllocator createByteBufAllocator(
Review Comment:
The method comments should also be updated
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1815] Support UnpooledByteBufAllocator [celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #3043:
URL: https://github.com/apache/celeborn/pull/3043#discussion_r1900852331
##########
client-flink/common/src/main/java/org/apache/celeborn/plugin/flink/network/FlinkTransportClientFactory.java:
##########
@@ -45,7 +45,7 @@ public FlinkTransportClientFactory(
TransportContext context, List<TransportClientBootstrap> bootstraps, int bufferSizeBytes) {
super(context, bootstraps);
bufferSuppliers = JavaUtils.newConcurrentHashMap();
- this.pooledAllocator = new UnpooledByteBufAllocator(true);
+ this.allocator = new UnpooledByteBufAllocator(true);
Review Comment:
@FMX Thanks for the information. BTW, it's better to leave some brief comments to explain the special logic
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1815] Support UnpooledByteBufAllocator [celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #3043:
URL: https://github.com/apache/celeborn/pull/3043#issuecomment-2567734181
Thanks, merged to main(0.6)
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1815] Support UnpooledByteBufAllocator [celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #3043:
URL: https://github.com/apache/celeborn/pull/3043#discussion_r1900433659
##########
common/src/main/java/org/apache/celeborn/common/network/util/NettyUtils.java:
##########
@@ -103,53 +104,59 @@ public static String getRemoteAddress(Channel channel) {
* released by the executor thread rather than the event loop thread. Those thread-local caches
* actually delay the recycling of buffers, leading to larger memory usage.
*/
- private static PooledByteBufAllocator createPooledByteBufAllocator(
- boolean allowDirectBufs, boolean allowCache, int numCores) {
- if (numCores == 0) {
- numCores = Runtime.getRuntime().availableProcessors();
+ private static ByteBufAllocator createByteBufAllocator(
Review Comment:
update: move the existing comment to the caller side, add new comments to explain each parameters
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org