Repository: syncthing/syncthing (language: go)

Issue:
Enabling rate limit drastically reduces network performance
Hi,

I was wondering about this:

SyncThing without limitation reaches a transfer speed of 119MiB/s

SyncThing with limitation of 112MiB/s reaches only 58MiB/s as max.

![50 unlimited](https://user-images.githubusercontent.com/116442584/198989448-f631f34a-d3cc-4711-9d29-f400bce0d9fd.jpg)

![51 limited](https://user-images.githubusercontent.com/116442584/198989465-c8dc1fd8-11f5-4a0f-ad00-ad6ed311bab1.jpg)



Discussion / hints:
So for what it's worth this doesn't reproduce easily for me, with two devices on the same machine talking over localhost. That just means the limiting isn't conceptually broken, I guess there may be timing effects or fragmentation or something that makes it perform less well over a real network with latency etc. If I remember correctly we will tend to do smaller writes when limiting is in effect, to smooth out the rate (when the limit is much lower than in these examples) and that could be an issue.


<img width="1173" alt="Screenshot 2022-10-31 at 12 36 37" src="https://user-images.githubusercontent.com/125426/198999689-681f12c7-83d6-4dcf-bbd4-ae1ff1adf376.png">


<img width="1179" alt="Screenshot 2022-10-31 at 12 37 55" src="https://user-images.githubusercontent.com/125426/198999729-a516a0f5-f84d-41a7-b9ea-e106c455a9a4.png">



I've personally observed that if upload rate is limited with the limit maxed out, then download rate will drop too even though there should still be enough download bandwidth available. Judging by the screenshots, this doesn't seem to be the case here, but I'm putting this information just in case.

Fix patch:
diff --git a/lib/connections/limiter.go b/lib/connections/limiter.go
index 3f3e84bd71b..9f279ba6edc 100644
--- a/lib/connections/limiter.go
+++ b/lib/connections/limiter.go
@@ -37,8 +37,7 @@ type waiter interface {
 }
 
 const (
-	limiterBurstSize   = 4 * 128 << 10
-	maxSingleWriteSize = 8 << 10
+	limiterBurstSize = 4 * 128 << 10
 )
 
 func newLimiter(myId protocol.DeviceID, cfg config.Wrapper) *limiter {
@@ -251,10 +250,20 @@ func (w *limitedWriter) Write(buf []byte) (int, error) {
 	}
 
 	// This does (potentially) multiple smaller writes in order to be less
-	// bursty with large writes and slow rates.
+	// bursty with large writes and slow rates. At the same time we don't
+	// want to do hilarious amounts of tiny writes when the rate is high, so
+	// try to be a bit adaptable. We range from the minimum write size of 1
+	// KiB up to the limiter burst size, aiming for about a write every
+	// 10ms.
+	singleWriteSize := int(w.waiter.Limit() / 100)          // 10ms worth of data
+	singleWriteSize = ((singleWriteSize / 1024) + 1) * 1024 // round up to the next kibibyte
+	if singleWriteSize > limiterBurstSize {
+		singleWriteSize = limiterBurstSize
+	}
+
 	written := 0
 	for written < len(buf) {
-		toWrite := maxSingleWriteSize
+		toWrite := singleWriteSize
 		if toWrite > len(buf)-written {
 			toWrite = len(buf) - written
 		}
@@ -294,7 +303,7 @@ func (w waiterHolder) take(tokens int) {
 	// into the lower level reads so we might get a large amount of data and
 	// end up in the loop further down.
 
-	if tokens < limiterBurstSize {
+	if tokens <= limiterBurstSize {
 		// Fast path. We won't get an error from WaitN as we don't pass a
 		// context with a deadline.
 		_ = w.waiter.WaitN(context.TODO(), tokens)


Test patch:
diff --git a/lib/connections/limiter_test.go b/lib/connections/limiter_test.go
index caf6bf201ef..2379ec65c27 100644
--- a/lib/connections/limiter_test.go
+++ b/lib/connections/limiter_test.go
@@ -218,7 +218,7 @@ func TestLimitedWriterWrite(t *testing.T) {
 
 	// A buffer with random data that is larger than the write size and not
 	// a precise multiple either.
-	src := make([]byte, int(12.5*maxSingleWriteSize))
+	src := make([]byte, int(12.5*8192))
 	if _, err := crand.Reader.Read(src); err != nil {
 		t.Fatal(err)
 	}
@@ -242,9 +242,14 @@ func TestLimitedWriterWrite(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// Verify there were lots of writes and that the end result is identical.
-	if cw.writeCount != 13 {
-		t.Error("expected lots of smaller writes, but not too many")
+	// Verify there were lots of writes (we expect one kilobyte write size
+	// for the very low rate in this test) and that the end result is
+	// identical.
+	if cw.writeCount < 10*8 {
+		t.Error("expected lots of smaller writes")
+	}
+	if cw.writeCount > 15*8 {
+		t.Error("expected fewer larger writes")
 	}
 	if !bytes.Equal(src, dst.Bytes()) {
 		t.Error("results should be equal")
@@ -319,8 +324,11 @@ func TestLimitedWriterWrite(t *testing.T) {
 	}
 
 	// Verify there were lots of writes and that the end result is identical.
-	if cw.writeCount != 13 {
-		t.Error("expected just the one write")
+	if cw.writeCount < 10*8 {
+		t.Error("expected lots of smaller writes")
+	}
+	if cw.writeCount > 15*8 {
+		t.Error("expected fewer larger writes")
 	}
 	if !bytes.Equal(src, dst.Bytes()) {
 		t.Error("results should be equal")