[GitHub] [incubator-celeborn] waitinfuture commented on a diff in pull request #1428: [CELEBORN-524][PERF] ChannelsLimiter onTrim add interval

["waitinfuture (via GitHub)" <gi...@apache.org>]

waitinfuture commented on code in PR #1428:
URL: https://github.com/apache/incubator-celeborn/pull/1428#discussion_r1169725312


##########
common/src/main/java/org/apache/celeborn/common/network/server/ChannelsLimiter.java:
##########
@@ -126,8 +142,10 @@ public void onResume(String moduleName) {
   }
 
   @Override
-  public void onTrim() {
-    trimCache();
+  public void onTrim(MemoryManager.TrimCallback callback) {
+    if (trimInProcess.compareAndSet(false, true)) {
+      trimCache();
+    }

Review Comment:
   should we wait until ```needTrimChannels``` equals 0 at the end of ```onTrim```? We can delete ```trimmedChannels``` and instead to decrement ```needTrimChannels``` in ```userEventTriggered```.



##########
common/src/main/java/org/apache/celeborn/common/network/server/memory/MemoryManager.java:
##########
@@ -323,7 +331,12 @@ public MemoryManagerStat currentMemoryAction() {
   }
 
   public void trimAllListeners() {
-    memoryPressureListeners.forEach(MemoryPressureListener::onTrim);
+    // First trim data from disk buffer to flusher
+    // Second trim flusher flush data
+    // Finally trim empty netty buffer cache
+    otherListeners.forEach(

Review Comment:
   I thinks it's better to sequentially call each listener's ```onTrim``` instead of using a callback



##########
common/src/main/java/org/apache/celeborn/common/network/server/ChannelsLimiter.java:
##########
@@ -102,6 +110,14 @@ public void handlerRemoved(ChannelHandlerContext ctx) throws Exception {
   public void userEventTriggered(ChannelHandlerContext ctx, Object evt) {
     if (evt instanceof TrimCache) {
       ((PooledByteBufAllocator) ctx.alloc()).trimCurrentThreadCache();
+      if (trimInProcess.get()) {

Review Comment:
   We don't need ```trimInProcess``` then



-- 
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