[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #1119: [CELEBORN-61][CELEBORN-62][FOLLOW_UP] Fix some issues for slow start

[GitBox <gi...@apache.org>]

AngersZhuuuu commented on code in PR #1119:
URL: https://github.com/apache/incubator-celeborn/pull/1119#discussion_r1058029003


##########
client/src/main/java/org/apache/celeborn/client/ShuffleClientImpl.java:
##########
@@ -1267,30 +1275,48 @@ public void setupMetaServiceRef(RpcEndpointRef endpointRef) {
     driverRssMetaService = endpointRef;
   }
 
+  /**
+   * If `pushDataSlowStart` is enabled, will increase `currentMaxReqsInFlight` gradually to meet the
+   * max push speed.
+   *
+   * <p>1. slow start period: every RTT period, `currentMaxReqsInFlight` is multiplied. 2.congestion
+   * avoidance: every RTT period, `currentMaxReqsInFlight` plus 1.
+   *
+   * <p>Note that here we define one RTT period: one batch(currentMaxReqsInFlight) of push data
+   * requests.
+   */
   private void slowStart() {
-    synchronized (currentMaxReqsInFlight) {
-      if (currentMaxReqsInFlight > maxInFlight) {
-        // Congestion avoidance
-        congestionAvoidanceFlag++;
-        if (congestionAvoidanceFlag >= currentMaxReqsInFlight) {
-          currentMaxReqsInFlight++;
-          congestionAvoidanceFlag = 0;
+    if (conf.pushDataSlowStart()) {
+      synchronized (congestionAvoidanceFlag) {
+        int pre = currentMaxReqsInFlight.get();
+        if (pre > maxInFlight) {
+          // Congestion avoidance
+          congestionAvoidanceFlag++;
+          if (congestionAvoidanceFlag >= pre) {
+            currentMaxReqsInFlight.set(pre + 1);
+            congestionAvoidanceFlag = 0;
+          }
+        } else {
+          // Slow start
+          currentMaxReqsInFlight.set(pre + 1);
         }
-      } else {
-        // Slow start
-        currentMaxReqsInFlight++;
       }
     }
   }
 
   private void congestionControl() {
-    synchronized (currentMaxReqsInFlight) {
-      if (currentMaxReqsInFlight <= 1) {
-        currentMaxReqsInFlight = 1;
-      } else {
-        currentMaxReqsInFlight = currentMaxReqsInFlight / 2;
-      }
-      maxInFlight = currentMaxReqsInFlight;
+    maxInFlight =
+        currentMaxReqsInFlight.getAndUpdate(
+            pre -> {
+              if (pre <= 1) {
+                return 1;
+              } else {
+                return pre / 2;
+              }
+            });
+
+    synchronized (congestionAvoidanceFlag) {
+      congestionAvoidanceFlag = 0;

Review Comment:
   Nice catch!



##########
client/src/main/java/org/apache/celeborn/client/ShuffleClientImpl.java:
##########
@@ -1267,30 +1275,48 @@ public void setupMetaServiceRef(RpcEndpointRef endpointRef) {
     driverRssMetaService = endpointRef;
   }
 
+  /**
+   * If `pushDataSlowStart` is enabled, will increase `currentMaxReqsInFlight` gradually to meet the
+   * max push speed.
+   *
+   * <p>1. slow start period: every RTT period, `currentMaxReqsInFlight` is multiplied. 2.congestion
+   * avoidance: every RTT period, `currentMaxReqsInFlight` plus 1.

Review Comment:
   ```
   * <p>1. slow start period: every RTT period, `currentMaxReqsInFlight` is multiplied. 
   * <p>2. congestion avoidance: every RTT period, `currentMaxReqsInFlight` plus 1.
   ```



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