Optimize locking behavior of the profiling handler. (#980)

The profiling handler is in the path of our most performance critical components (especially the activator). Taking a write-lock on each request is probably a bad idea.

Replaced the mutex with an atomic flag. Lost reads are not critical in this code path and that should be the quickest solution in terms of avoiding contention.

profiling/server.go

@@ -21,7 +21,7 @@ import (
 	"net/http"
 	"net/http/pprof"
 	"strconv"
-	"sync"
+	"sync/atomic"
 
 	"go.uber.org/zap"
 	corev1 "k8s.io/api/core/v1"
@@ -39,8 +39,7 @@ const (
 // Handler holds the main HTTP handler and a flag indicating
 // whether the handler is active
 type Handler struct {
-	enabled    bool
-	enabledMux sync.Mutex
+	enabled int32
 	handler http.Handler
 	log     *zap.SugaredLogger
 }
@@ -58,18 +57,15 @@ func NewHandler(logger *zap.SugaredLogger, enableProfiling bool) *Handler {
 	mux.HandleFunc(pprofPrefix+"trace", pprof.Trace)
 
 	logger.Infof("Profiling enabled: %t", enableProfiling)
-
 	return &Handler{
-		enabled: enableProfiling,
+		enabled: boolToInt32(enableProfiling),
 		handler: mux,
 		log:     logger,
 	}
 }
 
 func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	h.enabledMux.Lock()
-	defer h.enabledMux.Unlock()
-	if h.enabled {
+	if atomic.LoadInt32(&h.enabled) == 1 {
 		h.handler.ServeHTTP(w, r)
 	} else {
 		http.NotFoundHandler().ServeHTTP(w, r)
@@ -96,11 +92,11 @@ func (h *Handler) UpdateFromConfigMap(configMap *corev1.ConfigMap) {
 		h.log.Errorw("Failed to update the profiling flag", zap.Error(err))
 		return
 	}
-	h.enabledMux.Lock()
-	defer h.enabledMux.Unlock()
-	if h.enabled != enabled {
-		h.enabled = enabled
-		h.log.Infof("Profiling enabled: %t", h.enabled)
+
+	new := boolToInt32(enabled)
+	old := atomic.SwapInt32(&h.enabled, new)
+	if old != new {
+		h.log.Infof("Profiling enabled: %t", enabled)
 	}
 }
 
@@ -111,3 +107,10 @@ func NewServer(handler http.Handler) *http.Server {
 		Handler: handler,
 	}
 }
+
+func boolToInt32(b bool) int32 {
+	if b {
+		return 1
+	}
+	return 0
+}

profiling/server_test.go

@@ -19,6 +19,7 @@ package profiling
 import (
 	"net/http"
 	"net/http/httptest"
+	"sync/atomic"
 	"testing"
 
 	"go.uber.org/zap"
@@ -95,7 +96,7 @@ func TestUpdateFromConfigMap(t *testing.T) {
 				t.Errorf("StatusCode: %v, want: %v", rr.Code, tt.wantStatusCode)
 			}
 
-			if handler.enabled != tt.wantEnabled {
+			if atomic.LoadInt32(&handler.enabled) != boolToInt32(tt.wantEnabled) {
 				t.Fatalf("Test: %q; want %v, but got %v", tt.name, tt.wantEnabled, handler.enabled)
 			}
 		})