From 54472b70a05cee224857275470bad101bda18d45 Mon Sep 17 00:00:00 2001
From: Erik Seliger <erikseliger@me.com>
Date: Tue, 4 Jun 2024 22:54:11 +0200
Subject: [PATCH] gitserver(fs): Remove caching of cloned state (#63066)

I thought it might be nice to save on a few IOPS from the Stat calls to check if a repo is cloned by caching it for a bit. But turns out that the mutex locking and unlocking is more expensive than the stat operation.

We can come back here later and see if we can optimize it, we never had this in the past so this will be no regression.

Some background on mutex performance: https://stackoverflow.com/questions/57562606/why-does-sync-mutex-largely-drop-performance-when-goroutine-contention-is-more-t

Closes SRC-322

Test plan:

Without the cache, the tests still pass.
---
 cmd/gitserver/internal/gitserverfs/fs.go | 51 +++---------------------
 1 file changed, 5 insertions(+), 46 deletions(-)

diff --git a/cmd/gitserver/internal/gitserverfs/fs.go b/cmd/gitserver/internal/gitserverfs/fs.go
index b44db7baf4e..d411c40c4b8 100644
--- a/cmd/gitserver/internal/gitserverfs/fs.go
+++ b/cmd/gitserver/internal/gitserverfs/fs.go
@@ -6,7 +6,6 @@ import (
 	"path/filepath"
 	"strings"
 	"sync"
-	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/sourcegraph/log"
@@ -42,11 +41,9 @@ type FS interface {
 
 func New(observationCtx *observation.Context, reposDir string) FS {
 	return &realGitserverFS{
-		logger:              observationCtx.Logger.Scoped("gitserverfs"),
-		observationCtx:      observationCtx,
-		reposDir:            reposDir,
-		clonedState:         make(map[api.RepoName]struct{}),
-		lastCloneStateReset: time.Now(),
+		logger:         observationCtx.Logger.Scoped("gitserverfs"),
+		observationCtx: observationCtx,
+		reposDir:       reposDir,
 	}
 }
 
@@ -54,10 +51,6 @@ type realGitserverFS struct {
 	reposDir       string
 	observationCtx *observation.Context
 	logger         log.Logger
-	// clonedState keeps track of the clone state of a repo.
-	clonedState         map[api.RepoName]struct{}
-	cloneStateMu        sync.Mutex
-	lastCloneStateReset time.Time
 }
 
 func (r *realGitserverFS) Initialize() error {
@@ -107,46 +100,12 @@ func (r *realGitserverFS) VisitRepos(visit func(api.RepoName, common.GitDir) (do
 	return nil
 }
 
-const cloneStateResetInterval = 5 * time.Minute
-
 func (r *realGitserverFS) RepoCloned(name api.RepoName) (bool, error) {
-	r.cloneStateMu.Lock()
-	defer r.cloneStateMu.Unlock()
-
-	// Every few minutes, we invalidate the entire cache, in case we fall into
-	// some bad state, this'll fix the state every now and then.
-	if time.Since(r.lastCloneStateReset) > cloneStateResetInterval {
-		r.clonedState = make(map[api.RepoName]struct{})
-	}
-
-	_, cloned := r.clonedState[name]
-	if cloned {
-		return true, nil
-	}
-
-	cloned, err := repoCloned(r.RepoDir(name))
-	if err != nil {
-		return false, err
-	}
-
-	if cloned {
-		r.clonedState[name] = struct{}{}
-	}
-
-	return cloned, nil
+	return repoCloned(r.RepoDir(name))
 }
 
 func (r *realGitserverFS) RemoveRepo(name api.RepoName) error {
-	err := removeRepoDirectory(r.logger, r.reposDir, r.RepoDir(name))
-
-	// Mark as not cloned anymore in cache. We even remove it from the cache
-	// when the deletion failed partially, in the next call we'll recheck if
-	// the repo is still there.
-	r.cloneStateMu.Lock()
-	delete(r.clonedState, name)
-	r.cloneStateMu.Unlock()
-
-	return err
+	return removeRepoDirectory(r.logger, r.reposDir, r.RepoDir(name))
 }
 
 // iterateGitDirs walks over the reposDir on disk and calls walkFn for each of the