You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "leixm (via GitHub)" <gi...@apache.org> on 2024/12/20 10:12:16 UTC
[PR] [CELEBORN-1792] Congestion and MemoryManager should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
leixm opened a new pull request, #3018:
URL: https://github.com/apache/celeborn/pull/3018
### What changes were proposed in this pull request?
Congestion and MemoryManager should use pinnedDirectMemory instead of usedDirectMemory
### Why are the changes needed?
In our production environment, after worker pausing, the usedDirectMemory keep high and does not decrease. The worker node is permanently blacklisted and cannot be used.
This problem has been bothering us for a long time. When the thred cache is turned off, in fact, **after ctx.channel().config().setAutoRead(false), the netty framework will still hold some ByteBufs**. This part of ByteBuf allocated from chunk in PoolArena which can not be released.
In netty, if a chunk is 16M and 8k of this chunk has been allocated, then the pinnedMemory is 8k and the activeMemory is 16M. The remaining (16M-8k) memory can be allocated, but not yet allocated, netty allocates and releases memory in chunk units, so the 8k that has been allocated will result in 16M that cannot be returned to the operating system.
Here are some scenes from our production/test environment:
We config 10gb off-heap memory for worker, other configs as below:
```
celeborn.network.memory.allocator.allowCache false
celeborn.worker.monitor.memory.check.interval 100ms
celeborn.worker.monitor.memory.report.interval 10s
celeborn.worker.directMemoryRatioToPauseReceive 0.75
celeborn.worker.directMemoryRatioToPauseReplicate 0.85
celeborn.worker.directMemoryRatioToResume 0.5
```
When receiving high traffic, the worker's usedDirectMemory increases. After triggering trim and pause, usedDirectMemory still does not reach the resume threshold, and worker was excluded.
So we checked the heap snapshot of the abnormal worker, we can see that there are a large number of DirectByteBuffers in the heap memory. These DirectByteBuffers are all 4mb in size, which is exactly the size of chunksize. According to the path to gc root, DirectByteBuffer is held by PoolChunk, and these 4m only have 160k pinnedBytes.
There are many ByteBufs that are not released
The stack shows that these ByteBufs are allocated by netty
We tried to reproduce this situation in the test environment. When the same problem occurred, we added a restful api of the worker to force the worker to resume. After the resume, the worker returned to normal, and PushDataHandler handled many delayed requests.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing UTs.
--
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-1792] Congestion and MemoryManager should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on PR #3018:
URL: https://github.com/apache/celeborn/pull/3018#issuecomment-2568718896
Don't forget to update the default value of this config "celeborn.worker.directMemoryRatioToResume" to 0.3.
--
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-1792] MemoryManager resume should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "leixm (via GitHub)" <gi...@apache.org>.
leixm commented on code in PR #3018:
URL: https://github.com/apache/celeborn/pull/3018#discussion_r1923655109
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -93,6 +96,9 @@ public class MemoryManager {
private long memoryFileStorageThreshold;
private final LongAdder memoryFileStorageCounter = new LongAdder();
private final StorageManager storageManager;
+ private boolean pinnedMemoryCheckEnabled;
+ private long pinnedMemoryCheckInterval;
Review Comment:
Here is another PR introducing pinnedMemory metrics https://github.com/apache/celeborn/pull/3019
--
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-1792] Congestion and MemoryManager should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "leixm (via GitHub)" <gi...@apache.org>.
leixm commented on PR #3018:
URL: https://github.com/apache/celeborn/pull/3018#issuecomment-2556688899
Before optimization
After optimization
--
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-1792] MemoryManager resume should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "leixm (via GitHub)" <gi...@apache.org>.
leixm commented on code in PR #3018:
URL: https://github.com/apache/celeborn/pull/3018#discussion_r1923656702
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -93,6 +96,9 @@ public class MemoryManager {
private long memoryFileStorageThreshold;
private final LongAdder memoryFileStorageCounter = new LongAdder();
private final StorageManager storageManager;
+ private boolean pinnedMemoryCheckEnabled;
+ private long pinnedMemoryCheckInterval;
Review Comment:
getPinnedMemory is not called very frequently. It is called once every pinnedMemoryCheckInterval. The default is 10 seconds.
--
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-1792] MemoryManager resume should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "leixm (via GitHub)" <gi...@apache.org>.
leixm commented on PR #3018:
URL: https://github.com/apache/celeborn/pull/3018#issuecomment-2588848832
Seems flaky test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1792] MemoryManager resume should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong closed pull request #3018: [CELEBORN-1792] MemoryManager resume should use pinnedDirectMemory instead of usedDirectMemory
URL: https://github.com/apache/celeborn/pull/3018
--
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-1792] Congestion and MemoryManager should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on PR #3018:
URL: https://github.com/apache/celeborn/pull/3018#issuecomment-2568645836
I discussed this with FMX offline, and we believe it might be better to use pinnedDirectMemory for worker resumption while keeping usedDirectMemory for trimming. @leixm @AngersZhuuuu WDYT?
--
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-1792] MemoryManager resume should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "leixm (via GitHub)" <gi...@apache.org>.
leixm commented on PR #3018:
URL: https://github.com/apache/celeborn/pull/3018#issuecomment-2588846877
Seems MasterClientSuiteJ is a flaky test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1792] MemoryManager resume should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #3018:
URL: https://github.com/apache/celeborn/pull/3018#discussion_r1923213383
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -315,69 +325,68 @@ public ServingState currentServingState() {
}
@VisibleForTesting
- protected void switchServingState() {
+ public void switchServingState() {
ServingState lastState = servingState;
servingState = currentServingState();
- if (lastState == servingState) {
- if (servingState != ServingState.NONE_PAUSED) {
+ logger.info("Serving state transformed from {} to {}", lastState, servingState);
+ switch (servingState) {
+ case PUSH_PAUSED:
+ if (canResumeByPinnedMemory()) {
+ resumeByPinnedMemory(servingState);
+ } else {
+ pausePushDataCounter.increment();
+ if (lastState == ServingState.PUSH_AND_REPLICATE_PAUSED) {
+ logger.info("Trigger action: RESUME REPLICATE");
+ resumeReplicate();
+ } else {
+ logger.info("Trigger action: PAUSE PUSH");
+ pausePushDataStartTime = System.currentTimeMillis();
+ memoryPressureListeners.forEach(
+ memoryPressureListener ->
+ memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
+ }
+ }
logger.debug("Trigger action: TRIM");
- trimCounter += 1;
- // force to append pause spent time even we are in pause state
+ trimAllListeners();
if (trimCounter >= forceAppendPauseSpentTimeThreshold) {
Review Comment:
lost `trimCounter+=1`
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -315,69 +325,68 @@ public ServingState currentServingState() {
}
@VisibleForTesting
- protected void switchServingState() {
+ public void switchServingState() {
ServingState lastState = servingState;
servingState = currentServingState();
- if (lastState == servingState) {
- if (servingState != ServingState.NONE_PAUSED) {
+ logger.info("Serving state transformed from {} to {}", lastState, servingState);
+ switch (servingState) {
+ case PUSH_PAUSED:
+ if (canResumeByPinnedMemory()) {
+ resumeByPinnedMemory(servingState);
+ } else {
+ pausePushDataCounter.increment();
+ if (lastState == ServingState.PUSH_AND_REPLICATE_PAUSED) {
+ logger.info("Trigger action: RESUME REPLICATE");
+ resumeReplicate();
+ } else {
+ logger.info("Trigger action: PAUSE PUSH");
+ pausePushDataStartTime = System.currentTimeMillis();
+ memoryPressureListeners.forEach(
+ memoryPressureListener ->
+ memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
+ }
+ }
logger.debug("Trigger action: TRIM");
- trimCounter += 1;
- // force to append pause spent time even we are in pause state
+ trimAllListeners();
if (trimCounter >= forceAppendPauseSpentTimeThreshold) {
logger.debug(
"Trigger action: TRIM for {} times, force to append pause spent time.", trimCounter);
appendPauseSpentTime(servingState);
}
- trimAllListeners();
- }
- return;
- }
- logger.info("Serving state transformed from {} to {}", lastState, servingState);
- switch (servingState) {
- case PUSH_PAUSED:
- pausePushDataCounter.increment();
- if (lastState == ServingState.PUSH_AND_REPLICATE_PAUSED) {
- logger.info("Trigger action: RESUME REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onResume(TransportModuleConstants.REPLICATE_MODULE));
- } else if (lastState == ServingState.NONE_PAUSED) {
- logger.info("Trigger action: PAUSE PUSH");
- pausePushDataStartTime = System.currentTimeMillis();
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- }
- trimAllListeners();
break;
case PUSH_AND_REPLICATE_PAUSED:
- pausePushDataAndReplicateCounter.increment();
- if (lastState == ServingState.NONE_PAUSED) {
+ if (canResumeByPinnedMemory()) {
+ resumeByPinnedMemory(servingState);
+ } else {
+ pausePushDataAndReplicateCounter.increment();
logger.info("Trigger action: PAUSE PUSH");
pausePushDataAndReplicateStartTime = System.currentTimeMillis();
memoryPressureListeners.forEach(
memoryPressureListener ->
memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
+ logger.info("Trigger action: PAUSE REPLICATE");
+ memoryPressureListeners.forEach(
+ memoryPressureListener ->
+ memoryPressureListener.onPause(TransportModuleConstants.REPLICATE_MODULE));
}
- logger.info("Trigger action: PAUSE REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.REPLICATE_MODULE));
+ logger.debug("Trigger action: TRIM");
trimAllListeners();
Review Comment:
ditto
--
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-1792] MemoryManager resume should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on PR #3018:
URL: https://github.com/apache/celeborn/pull/3018#issuecomment-2606405070
Thanks, merge to main(v0.6.0)
--
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-1792] MemoryManager resume should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #3018:
URL: https://github.com/apache/celeborn/pull/3018#discussion_r1912705474
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -3858,7 +3858,7 @@ object CelebornConf extends Logging {
.doc("If direct memory usage is less than this limit, worker will resume.")
.version("0.2.0")
.doubleConf
- .createWithDefault(0.7)
+ .createWithDefault(0.3)
Review Comment:
Maybe we can add a new conf for pinnedMemoryToResume and keep exist conf for directMemoryRatioToResume
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -293,6 +297,18 @@ public boolean shouldEvict(boolean aggressiveMemoryFileEvictEnabled, double evic
public ServingState currentServingState() {
long memoryUsage = getMemoryUsage();
+ long allocatedMemory;
+ if (networkMemoryAllocatorPooled) {
+ allocatedMemory = getAllocatedMemory();
+ } else {
+ allocatedMemory = memoryUsage;
+ }
+ // trigger resume
+ // CELEBORN-1792: resume should use pinnedDirectMemory instead of usedDirectMemory
Review Comment:
Although we needn't change to pause state, it would be better to call trim when netty direct memory used above pausePushDataThreshold/pauseReplicateThreshold, WDYT?
--
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-1792] MemoryManager resume should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #3018:
URL: https://github.com/apache/celeborn/pull/3018#discussion_r1920949498
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -557,6 +580,47 @@ public static void reset() {
_INSTANCE = null;
}
+ private void resumeByPinnedMemory(ServingState servingState) {
+ switch (servingState) {
+ case PUSH_AND_REPLICATE_PAUSED:
+ logger.info(
+ "Serving State is PUSH_AND_REPLICATE_PAUSED, but resume by lower pinned memory {}",
+ getNettyPinnedDirectMemory());
+ resumeReplicate();
+ resumePush();
+ case PUSH_PAUSED:
+ logger.info(
+ "Serving State is PUSH_PAUSED, but resume by lower pinned memory {}",
+ getNettyPinnedDirectMemory());
+ resumePush();
+ }
+ }
+
+ private boolean canResumeByPinnedMemory() {
+ if (pinnedMemoryCheckEnabled
+ && System.currentTimeMillis() >= pinnedMemoryNextCheckTime
+ && getAllocatedMemory() / (double) (maxDirectMemory) < pinnedMemoryResumeRatio) {
+ pinnedMemoryNextCheckTime += pinnedMemoryCheckInterval;
Review Comment:
`pinnedMemoryNextCheckTime` compute seems incorrect, We can Use `System.currentTimeMillis()` as last checkTime, then we can use `System.currentTimeMillis()-lastCheckTime >= pinnedMemoryCheckInterval` to check whether need resume.
--
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-1792] MemoryManager resume should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "onyourhead (via GitHub)" <gi...@apache.org>.
onyourhead commented on PR #3018:
URL: https://github.com/apache/celeborn/pull/3018#issuecomment-2647779769
@leixm I have a question. After the resumeByPinnedMemory method is called, all Channels are resumed. However, 10 milliseconds later, the memoryUsage may still be greater than the pausePushDataThreshold, and the switchServingState method will pause all Channels again. Is this what will happen? It seems that we only get a very short resume.
--
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-1792] Congestion and MemoryManager should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on PR #3018:
URL: https://github.com/apache/celeborn/pull/3018#issuecomment-2557970317
I have a question. Will the worker OOM when the pinned memory is high? In our previous implementations, the direct memory counter will count all direct memory allocated, whether or not the allocator can allocate.
Can you do a pressure test for this scenario?
--
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-1792] Congestion and MemoryManager should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "leixm (via GitHub)" <gi...@apache.org>.
leixm commented on PR #3018:
URL: https://github.com/apache/celeborn/pull/3018#issuecomment-2568656344
> I discussed this with FMX offline, and we believe it might be better to use pinnedDirectMemory for worker resumption while keeping usedDirectMemory for trimming. @leixm @AngersZhuuuu WDYT?
It's okay for me. @AngersZhuuuu WDYT?
--
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-1792] MemoryManager resume should use pinnedDirectMemory instead of usedDirectMemory [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #3018:
URL: https://github.com/apache/celeborn/pull/3018#discussion_r1923004789
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -282,7 +292,7 @@ private MemoryManager(CelebornConf conf, StorageManager storageManager, Abstract
Utils.bytesToString(readBufferThreshold),
Utils.bytesToString(readBufferTarget),
Utils.bytesToString(memoryFileStorageThreshold),
- resumeRatio);
+ directMemoryResumeRatio);
Review Comment:
You can add pinned memory resume ratio here. It is an important parameter for memory manager.
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -93,6 +96,9 @@ public class MemoryManager {
private long memoryFileStorageThreshold;
private final LongAdder memoryFileStorageCounter = new LongAdder();
private final StorageManager storageManager;
+ private boolean pinnedMemoryCheckEnabled;
+ private long pinnedMemoryCheckInterval;
+ private long pinnedMemoryLastCheckTime = 0L;
Review Comment:
The default value for this is 0.
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -93,6 +96,9 @@ public class MemoryManager {
private long memoryFileStorageThreshold;
private final LongAdder memoryFileStorageCounter = new LongAdder();
private final StorageManager storageManager;
+ private boolean pinnedMemoryCheckEnabled;
+ private long pinnedMemoryCheckInterval;
Review Comment:
To avoid frequently calling a pinned memory counter, I think you can cache the last pinned memory value and refresh it periodically. And exporting the pinned memory value to the metrics.
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -436,6 +445,16 @@ public long getMemoryUsage() {
return getNettyUsedDirectMemory() + sortMemoryCounter.get();
}
+ public long getAllocatedMemory() {
Review Comment:
This method should be renamed to getPinnedMemory. The allocated memory is the netty memory counter.
--
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