Copilot commented on code in PR #3756:
URL: https://github.com/apache/celeborn/pull/3756#discussion_r3557475351
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/congestcontrol/CongestionController.java:
##########
@@ -203,7 +203,7 @@ public void consumeBytes(int numBytes) {
}
public long getTotalPendingBytes() {
- return MemoryManager.instance().getMemoryUsage();
+ return MemoryManager.instance().getPinnedMemory();
}
Review Comment:
`getTotalPendingBytes()` is evaluated on the congestion-control checker
thread, whose default interval is 10ms
(celeborn.worker.congestionControl.check.interval). Switching to
`MemoryManager.getPinnedMemory()` makes each check iterate all pooled Netty
allocators (`getNettyPinnedDirectMemory()`), which can add noticeable CPU/alloc
overhead at this frequency and also bypasses MemoryManager’s own pinned-memory
check throttling (`pinnedMemoryCheckInterval`). Consider caching/throttling the
pinned-memory sampling (e.g., reuse MemoryManager’s interval or add a
lightweight cached accessor) so the 10ms loop doesn’t scan allocators every
time.
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/congestcontrol/CongestionController.java:
##########
@@ -203,7 +203,7 @@ public void consumeBytes(int numBytes) {
}
public long getTotalPendingBytes() {
- return MemoryManager.instance().getMemoryUsage();
+ return MemoryManager.instance().getPinnedMemory();
}
Review Comment:
Current unit tests for `CongestionController` override
`getTotalPendingBytes()` in anonymous subclasses, so this production
implementation (and the switch from `getMemoryUsage()` to `getPinnedMemory()`)
isn’t exercised by tests. Adding a test that uses the real implementation (or
injecting/mocking the MemoryManager value) would guard against regressions in
resume-from-congestion behavior tied to pinned memory sampling.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
