git: Cache Git version

In various places we need to detect the Git version such that we can
decide whether a given feature is supported. Detecting the Git version
requires us to run git-version(1) every time, which can become quite
expensive when executed often. This is exacerbated under load when
spawning lots of commands due to the spawn tokens we generate, which
means that it may take multiple seconds to detect the Git version.

We can improve the situation quite easily by caching the Git version: if
the Git binary has not been changed since we have last detected the Git
version, then we don't need to rerun git-version(1). This reduces the
number of Git commands we need to spawn in some locations and thus also
reduces the contention around command spawn tokens.

Changelog: performance

2 files changed, 110 insertions, 2 deletions

diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index 263174335..45ca9cbec 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -8,6 +8,7 @@ import (
 	"os/exec"
 	"path/filepath"
 	"strings"
+	"sync"
 
 	"github.com/prometheus/client_golang/prometheus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/cgroups"
@@ -88,6 +89,10 @@ type ExecCommandFactory struct {
 	cgroupsManager        cgroups.Manager
 	invalidCommandsMetric *prometheus.CounterVec
 	skipHooks             bool
+
+	cachedGitVersionLock sync.RWMutex
+	cachedGitVersion     Version
+	cachedGitStat        *os.FileInfo
 }
 
 // NewExecCommandFactory returns a new instance of initialized ExecCommandFactory. The returned
@@ -196,8 +201,43 @@ func (cf *ExecCommandFactory) validateHooks() error {
 	return nil
 }
 
-// GitVersion returns the Git version in use.
+func statDiffers(a, b os.FileInfo) bool {
+	return a.Size() != b.Size() || a.ModTime() != b.ModTime() || a.Mode() != b.Mode()
+}
+
+// GitVersion returns the Git version in use. The version is cached as long as the binary remains
+// unchanged as determined by stat(3P).
 func (cf *ExecCommandFactory) GitVersion(ctx context.Context) (Version, error) {
+	stat, err := os.Stat(cf.GetExecutionEnvironment(ctx).BinaryPath)
+	if err != nil {
+		return Version{}, fmt.Errorf("cannot stat Git binary: %w", err)
+	}
+
+	cf.cachedGitVersionLock.RLock()
+	upToDate := cf.cachedGitStat != nil && !statDiffers(stat, *cf.cachedGitStat)
+	cachedGitVersion := cf.cachedGitVersion
+	cf.cachedGitVersionLock.RUnlock()
+
+	if upToDate {
+		return cachedGitVersion, nil
+	}
+
+	cf.cachedGitVersionLock.Lock()
+	defer cf.cachedGitVersionLock.Unlock()
+
+	// We cannot reuse the stat(3P) information from above given that it wasn't acquired under
+	// the write-lock. As such, it may have been invalidated by a concurrent thread which has
+	// already updated the Git version information.
+	stat, err = os.Stat(cf.GetExecutionEnvironment(ctx).BinaryPath)
+	if err != nil {
+		return Version{}, fmt.Errorf("cannot stat Git binary: %w", err)
+	}
+
+	// There is a race here: if the Git executable has changed between calling stat(3P) on the
+	// binary and executing it, then we may report the wrong Git version. This race is inherent
+	// though: it can also happen after `GitVersion()` was called, so it doesn't really help to
+	// retry version detection here. Instead, we just live with this raciness -- the next call
+	// to `GitVersion()` would detect the version being out-of-date anyway and thus correct it.
 	cmd, err := cf.NewWithoutRepo(ctx, SubCmd{
 		Name: "version",
 	})
@@ -205,7 +245,15 @@ func (cf *ExecCommandFactory) GitVersion(ctx context.Context) (Version, error) {
 		return Version{}, fmt.Errorf("spawning version command: %w", err)
 	}
 
-	return parseVersionFromCommand(cmd)
+	gitVersion, err := parseVersionFromCommand(cmd)
+	if err != nil {
+		return Version{}, err
+	}
+
+	cf.cachedGitVersion = gitVersion
+	cf.cachedGitStat = &stat
+
+	return gitVersion, nil
 }
 
 // newCommand creates a new command.Command for the given git command. If a repo is given, then the
diff --git a/internal/git/version_test.go b/internal/git/version_test.go
index 74a113148..51ea27c67 100644
--- a/internal/git/version_test.go
+++ b/internal/git/version_test.go
@@ -2,6 +2,7 @@ package git
 
 import (
 	"fmt"
+	"os"
 	"path/filepath"
 	"testing"
 
@@ -69,6 +70,65 @@ func TestExecCommandFactory_GitVersion(t *testing.T) {
 			require.Equal(t, tc.expectedVersion, actualVersion)
 		})
 	}
+
+	t.Run("caching", func(t *testing.T) {
+		gitPath := filepath.Join(testhelper.TempDir(t), "git")
+		testhelper.WriteExecutable(t, gitPath, []byte(
+			`#!/usr/bin/env bash
+			echo 'git version 1.2.3'
+		`))
+
+		stat, err := os.Stat(gitPath)
+		require.NoError(t, err)
+
+		gitCmdFactory, cleanup, err := NewExecCommandFactory(config.Cfg{
+			Git: config.Git{
+				BinPath: gitPath,
+			},
+		}, WithSkipHooks())
+		require.NoError(t, err)
+		defer cleanup()
+
+		version, err := gitCmdFactory.GitVersion(ctx)
+		require.NoError(t, err)
+		require.Equal(t, "1.2.3", version.String())
+		require.Equal(t, stat, *gitCmdFactory.cachedGitStat)
+
+		// We rewrite the file with the same content length and modification time such that
+		// its file information doesn't change. As a result, information returned by
+		// stat(3P) shouldn't differ and we should continue to see the cached version. This
+		// is a known insufficiency, but it is extremely unlikely to ever happen in
+		// production when the real Git binary changes.
+		require.NoError(t, os.Remove(gitPath))
+		testhelper.WriteExecutable(t, gitPath, []byte(
+			`#!/usr/bin/env bash
+			echo 'git version 9.8.7'
+		`))
+		require.NoError(t, os.Chtimes(gitPath, stat.ModTime(), stat.ModTime()))
+
+		// Given that we continue to use the cached version we shouldn't see any
+		// change here.
+		version, err = gitCmdFactory.GitVersion(ctx)
+		require.NoError(t, err)
+		require.Equal(t, "1.2.3", version.String())
+		require.Equal(t, stat, *gitCmdFactory.cachedGitStat)
+
+		// If we really replace the Git binary with something else, then we should
+		// see a changed version.
+		require.NoError(t, os.Remove(gitPath))
+		testhelper.WriteExecutable(t, gitPath, []byte(
+			`#!/usr/bin/env bash
+			echo 'git version 2.34.1'
+		`))
+
+		stat, err = os.Stat(gitPath)
+		require.NoError(t, err)
+
+		version, err = gitCmdFactory.GitVersion(ctx)
+		require.NoError(t, err)
+		require.Equal(t, "2.34.1", version.String())
+		require.Equal(t, stat, *gitCmdFactory.cachedGitStat)
+	})
 }
 
 func TestVersion_LessThan(t *testing.T) {