Opened 4 months ago

Last modified 3 months ago

#2427 new enhancement

Maintaining bandwidth correctly: PeerState.java

Reported by: jogger Owned by: zzz
Priority: major Milestone: undecided
Component: router/transport Version: 0.9.38
Keywords: Cc:
Parent Tickets: #2412

Description

I am making this a separate ticket, as it is of fundamental importance. This is to resolve the following issues:

  • Whenever available bandwidth is checked, it must be made sure that the value returned belongs to the current time interval. getSendWindowBytesRemaining() ignores this. This is the major performance issue I2CP users complained about as they often come with large chunks of data that will then neither notify getNextVolley() nor cause an immediate send that should be allowed.
  • Maintaining bandwidth in long time intervals (currently >= 1000 ms by allocateSendingBytes()) leads to short term traffic bursts, as allowed traffic may concentrate at the end and the beginning of two consecutive time intervals, possibly causing a short term overload for outbound queues and the network connection.

The solution is easy by moving the bandwidth refill to getSendWindowBytesRemaining() and handing out bandwidth in much smaller chunks as low as 8 bytes only (unlikely, but possible on fast CPU). I simply do this before each send, provided the system clock has advanced. The maintenance of send window size stays untouched as well as the ACK logic.

Results are convincing:

  • I2CP traffic outbound (seeding torrents) clearly up
  • UDP packet pusher CPU usage clearly down
  • message delay seems down a bit
  • UDP send pauses smaller / more evenly distributed according to tcpdump

This is independent of #2412, however they make a perfect team as the improved logic in #2412 heavily relies on the correctness of the above functions.

Hope this diff is correct.

--- "PeerState orig.java"	2019-02-06 13:21:26.571744716 +0100
+++ "PeerState patch.java"	2019-02-06 14:06:03.014869734 +0100
@@ -479,7 +479,20 @@
 
     /** how many bytes can we send to the peer in the current second */
     public int getSendWindowBytesRemaining() {
-        synchronized(_outboundMessages) {
+        synchronized(_outboundMessages) { // possibly reentrant
+            long now = _context.clock().now();
+            int duration = (int)(now - _lastSendRefill);
+            if (_sendBytes > 0 && duration > 0 || duration >= 1000) {
+                _sendWindowBytesRemaining = Math.min(_sendWindowBytesRemaining + (_sendWindowBytes * duration + 500) / 1000, _sendWindowBytes);
+                _sendBps = (99 * _sendBps + _sendBytes + 50) / 100;
+             //if (isForACK) {
+            //    _sendACKBytes += size;
+            //    _sendACKBps = (int)(0.9f*(float)_sendACKBps + 0.1f*((float)_sendACKBytes * (1000f/(float)duration)));
+            //}
+               _sendBytes = 0;
+            //_sendACKBytes = 0;
+                _lastSendRefill = now;
+            }
             return _sendWindowBytesRemaining;
         }
     }
@@ -694,43 +707,29 @@
     /**
      *  Caller should synch
      */
-    private boolean allocateSendingBytes(int size, boolean isForACK, int messagePushCount) { 
-        long now = _context.clock().now();
-        long duration = now - _lastSendRefill;
-        if (duration >= 1000) {
-            _sendWindowBytesRemaining = _sendWindowBytes;
-            _sendBytes += size;
-            _sendBps = (int)(0.9f*_sendBps + 0.1f*(_sendBytes * (1000f/duration)));
-            //if (isForACK) {
-            //    _sendACKBytes += size;
-            //    _sendACKBps = (int)(0.9f*(float)_sendACKBps + 0.1f*((float)_sendACKBytes * (1000f/(float)duration)));
-            //}
-            _sendBytes = 0;
-            //_sendACKBytes = 0;
-            _lastSendRefill = now;
-        }
+    private boolean allocateSendingBytes(int size, boolean isForACK, int messagePushCount) {
         //if (true) return true;
-        if (IGNORE_CWIN || size <= _sendWindowBytesRemaining || (ALWAYS_ALLOW_FIRST_PUSH && messagePushCount == 0)) {
+        if (size <= getSendWindowBytesRemaining() || IGNORE_CWIN || isForACK || (ALWAYS_ALLOW_FIRST_PUSH && messagePushCount == 0)) {
             if ( (messagePushCount == 0) && (_concurrentMessagesActive > _concurrentMessagesAllowed) ) {
                 _consecutiveRejections++;
                 _context.statManager().addRateData("udp.rejectConcurrentActive", _concurrentMessagesActive, _consecutiveRejections);
                 return false;
-            } else if (messagePushCount == 0) {
+            }
+            if (messagePushCount == 0) {
                 _context.statManager().addRateData("udp.allowConcurrentActive", _concurrentMessagesActive, _concurrentMessagesAllowed);
                 _concurrentMessagesActive++;
                 if (_consecutiveRejections > 0) 
                     _context.statManager().addRateData("udp.rejectConcurrentSequence", _consecutiveRejections, _concurrentMessagesActive);
                 _consecutiveRejections = 0;
             }
-            _sendWindowBytesRemaining -= size; 
+            _sendWindowBytesRemaining -= size;
             _sendBytes += size;
-            _lastSendTime = now;
-            //if (isForACK) 
+            _lastSendTime = _context.clock().now();
+            //if (isForACK)
             //    _sendACKBytes += size;
             return true;
-        } else {
-            return false;
         }
+        return false;
     }
     
     /** if we need to contact them, do we need to talk to an introducer? */
@@ -1178,22 +1177,16 @@
                         _sendWindowBytes += bytesACKed; //512; // bytesACKed;
                 //}
             }
-        } else {
-            int allow = _concurrentMessagesAllowed - 1;
-            if (allow < MIN_CONCURRENT_MSGS)
-                allow = MIN_CONCURRENT_MSGS;
-            _concurrentMessagesAllowed = allow;
-        }
+        } else
+            _concurrentMessagesAllowed = Math.max(_concurrentMessagesAllowed - 1, MIN_CONCURRENT_MSGS);
+
         if (_sendWindowBytes > MAX_SEND_WINDOW_BYTES)
             _sendWindowBytes = MAX_SEND_WINDOW_BYTES;
         _lastReceiveTime = _context.clock().now();
         _lastSendFullyTime = _lastReceiveTime;
         
         //if (true) {
-            if (_sendWindowBytesRemaining + bytesACKed <= _sendWindowBytes)
-                _sendWindowBytesRemaining += bytesACKed;
-            else
-                _sendWindowBytesRemaining = _sendWindowBytes;
+            _sendWindowBytesRemaining = Math.min(_sendWindowBytesRemaining + bytesACKed, _sendWindowBytes);
         //}
         
         if (numSends < 2) {

Subtickets (add)

Attachments (1)

udp_sendBps.png (149.6 KB) - added by zab 3 months ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 3 months ago by jogger

After introducing the sliding window now there is a cosmetic glitch in the above full stats only calculation of _sendBps. Should be

_sendBps = (99 * _sendBps + _sendBytes * 1000 / duration + 50) / 100;

comment:2 Changed 3 months ago by zzz

@OP reference to "the major performance issue I2CP users complained about" ? link or ticket?

related: #2424

comment:3 Changed 3 months ago by jogger

I was referring to complaints in current and previous forums about torrent send speed. The updated bandwidth calculation deals with possibly not allowing traffic that would fit the send window.

comment:4 Changed 3 months ago by zzz

  • Parent Tickets set to 2412

This ticket is to fix a bug described in #2412 comment 21.

According to OP here, #2412 relies on this ticket, so making this one a child.

comment:5 Changed 3 months ago by zab

I haven't analyzed the patch as thoroughly as I should yet, but I did put it on my busy router and the udp.sendBps metric is way, way up. Overall traffic however is about the same, which leads me to think that metric is somehow broken by this patch.

comment:6 Changed 3 months ago by zab

I reverted the line that does the sendBps calculation to

_sendBps = (int)(0.9 * _sendBps + 0.1* _sendBytes * 1000.0 / duration);

and this restored the udp.sendBps metric to it's previous range. Whether that's correct or incorrect is a different question.

You are allocating too much bandwidth in the following logic:

if (_sendBytes > 0 && duration > 0 || duration >= 1000) {
                _sendWindowBytesRemaining = Math.min(_sendWindowBytesRemaining + (_sendWindowBytes * duration + 500) / 1000, _sendWindowBytes);

To see what I mean, imagine the initial available bytes are X. They get used up immediately, in less than 1ms (not likely but possible). 1ms elapses and you allocate another X/1000 + 0.5, and so on - total bandwidth used for the second interval will be 2X + 500.

Version 0, edited 3 months ago by zab (next)

comment:7 Changed 3 months ago by jogger

Correct sendBps calculation now found in the combined patch within #2412.

My bandwidth calculation is correct. The current logic hands out too little bw when not run exactly every 1000 ms. It might hand out too much if X is transmitted twice within a second when an interval boundary falls in between.

In my logic using a sliding window, if X is available at any given point in time, this means there has been no traffic over the past second. So X can be used up immediately, beginning a new interval with bandwidth building up to X again over the netxt 1000 ms. The 500 mentioned statistically vanish through integer truncation.

Changed 3 months ago by zab

comment:8 Changed 3 months ago by zab

The attachment udp_sendBps.png is with the patch from comment 32 from ticket 2412. Doesn't look fixed at all. Are you sure about that multiplication by 1000?

comment:9 Changed 3 months ago by jogger

absolutely sure. To see it is correct just assume duration = 1000,

Note: See TracTickets for help on using tickets.