From b754fdb3e1262692ceeef216c9ed0ee1c73133af Mon Sep 17 00:00:00 2001
From: Agniva De Sarker <agnivade@yahoo.co.in>
Date: Sat, 28 Mar 2020 09:33:38 +0530
Subject: [PATCH] MM-23460: Reuse a single HTTP client across all push
 notifications (#14122)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Before this, for every single request we would create a new http.Client
from scratch. This was costly because every new client has internal
TCP connections which it reuses.

Therefore, originally, beyond a throughput of 800-1200 rps with a
concurrency of 50, beyond which it would run out of file descriptors.

Now it can support upto 8000-10000 rps.
An improvement by a factor of 10.

Throughput numbers:
BenchmarkPushNotification
    BenchmarkPushNotification: notification_push_test.go:1478: throughput: 11445.091601 reqs/s
    BenchmarkPushNotification: notification_push_test.go:1478: throughput: 9190.761518 reqs/s
    BenchmarkPushNotification: notification_push_test.go:1478: throughput: 8334.394764 reqs/s
BenchmarkPushNotification-8   	     219	   5999304 ns/op	 3369224 B/op	   42127 allocs/op
    BenchmarkPushNotification: notification_push_test.go:1478: throughput: 9590.399780 reqs/s
    BenchmarkPushNotification: notification_push_test.go:1478: throughput: 9347.137463 reqs/s
    BenchmarkPushNotification: notification_push_test.go:1478: throughput: 9053.784250 reqs/s
BenchmarkPushNotification-8   	     223	   5522636 ns/op	 3368470 B/op	   42034 allocs/op
    BenchmarkPushNotification: notification_push_test.go:1478: throughput: 10250.783365 reqs/s
    BenchmarkPushNotification: notification_push_test.go:1478: throughput: 9123.292899 reqs/s
    BenchmarkPushNotification: notification_push_test.go:1478: throughput: 9173.553180 reqs/s

This is the aggregate count using benchstat.
The number is for a single run of sending 50 requests.

name                time/op
PushNotification-8  5.61ms ± 7%

name                alloc/op
PushNotification-8  3.36MB ± 1%

name                allocs/op
PushNotification-8   42.0k ± 1%

Tested on a machine with ulimit -n = 4096.
---
 app/notification_push.go      | 2 +-
 app/notification_push_test.go | 8 +++++---
 app/server.go                 | 4 +++-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/app/notification_push.go b/app/notification_push.go
index 3dd056ce1a..fae062847d 100644
--- a/app/notification_push.go
+++ b/app/notification_push.go
@@ -333,7 +333,7 @@ func (a *App) sendToPushProxy(msg model.PushNotification, session *model.Session
 		return err
 	}
 
-	resp, err := a.HTTPService().MakeClient(true).Do(request)
+	resp, err := a.Srv().pushNotificationClient.Do(request)
 	if err != nil {
 		return err
 	}
diff --git a/app/notification_push_test.go b/app/notification_push_test.go
index 83132cb118..3ccec8f118 100644
--- a/app/notification_push_test.go
+++ b/app/notification_push_test.go
@@ -1349,6 +1349,7 @@ func TestAllPushNotifications(t *testing.T) {
 	assert.Equal(t, 6, numUpdateBadges)
 }
 
+// Run it with | grep -v '{"level"' to prevent spamming the console.
 func BenchmarkPushNotification(b *testing.B) {
 	th := SetupWithStoreMock(b)
 	defer th.TearDown()
@@ -1432,9 +1433,10 @@ func BenchmarkPushNotification(b *testing.B) {
 	b.ResetTimer()
 	// We have an inner loop which ranges the testdata slice
 	// and we just repeat that.
-	// TODO: replace 10 by b.N
 	then := time.Now()
-	for i := 0; i < 10; i++ {
+	cnt := 0
+	for i := 0; i < b.N; i++ {
+		cnt++
 		var wg sync.WaitGroup
 		for j, data := range testData {
 			wg.Add(1)
@@ -1473,7 +1475,7 @@ func BenchmarkPushNotification(b *testing.B) {
 		}
 		wg.Wait()
 	}
-	b.Logf("time taken: %v", time.Since(then))
+	b.Logf("throughput: %f reqs/s", float64(len(testData)*cnt)/time.Since(then).Seconds())
 	b.StopTimer()
 	time.Sleep(2 * time.Second)
 }
diff --git a/app/server.go b/app/server.go
index 743e088d4f..8ffd881293 100644
--- a/app/server.go
+++ b/app/server.go
@@ -118,7 +118,8 @@ type Server struct {
 
 	phase2PermissionsMigrationComplete bool
 
-	HTTPService httpservice.HTTPService
+	HTTPService            httpservice.HTTPService
+	pushNotificationClient *http.Client // TODO: move this to it's own package
 
 	ImageProxy *imageproxy.ImageProxy
 
@@ -204,6 +205,7 @@ func NewServer(options ...Option) (*Server, error) {
 	})
 
 	s.HTTPService = httpservice.MakeHTTPService(s)
+	s.pushNotificationClient = s.HTTPService.MakeClient(true)
 
 	s.ImageProxy = imageproxy.MakeImageProxy(s, s.HTTPService, s.Log)