From eecb1bfe22c36451003f65d1b30dac81e2e2ccaf Mon Sep 17 00:00:00 2001 From: Carlos Alexandro Becker Date: Tue, 17 Feb 2026 13:21:58 -0300 Subject: [PATCH] perf: remove mutex from lsp manager - used the synchronized client map - lsps root markers don't need to be gitignore aware - cache unexisting lsp binaries globally closes #2223 Signed-off-by: Carlos Alexandro Becker --- internal/agent/tools/glob.go | 2 +- internal/fsext/fileutil.go | 18 +++++-- internal/fsext/fileutil_test.go | 26 +++++----- internal/lsp/client.go | 2 +- internal/lsp/manager.go | 92 +++++++++++++++++++-------------- internal/ui/model/lsp.go | 2 +- 6 files changed, 83 insertions(+), 59 deletions(-) diff --git a/internal/agent/tools/glob.go b/internal/agent/tools/glob.go index 3f56d9dcb6437d5ad6c9ca3b8416ae45c6eb7969..1117e57858e07bbcf8c401e98fd7e66be13140ff 100644 --- a/internal/agent/tools/glob.go +++ b/internal/agent/tools/glob.go @@ -81,7 +81,7 @@ func globFiles(ctx context.Context, pattern, searchPath string, limit int) ([]st slog.Warn("Ripgrep execution failed, falling back to doublestar", "error", err) } - return fsext.GlobWithDoubleStar(pattern, searchPath, limit) + return fsext.GlobGitignoreAware(pattern, searchPath, limit) } func runRipgrep(cmd *exec.Cmd, searchRoot string, limit int) ([]string, error) { diff --git a/internal/fsext/fileutil.go b/internal/fsext/fileutil.go index c091820935d9c13142b12d3bd79d8c023a42a2fd..72410a10721ae6af1965942d455a8310096399e6 100644 --- a/internal/fsext/fileutil.go +++ b/internal/fsext/fileutil.go @@ -82,7 +82,19 @@ func (w *FastGlobWalker) ShouldSkipDir(path string) bool { return w.directoryLister.shouldIgnore(path, nil, true) } -func GlobWithDoubleStar(pattern, searchPath string, limit int) ([]string, bool, error) { +// Glob globs files. +// +// Does not respect gitignore. +func Glob(pattern string, cwd string, limit int) ([]string, bool, error) { + return globWithDoubleStar(pattern, cwd, limit, false) +} + +// GlobGitignoreAware globs files respecting gitignore. +func GlobGitignoreAware(pattern string, cwd string, limit int) ([]string, bool, error) { + return globWithDoubleStar(pattern, cwd, limit, true) +} + +func globWithDoubleStar(pattern, searchPath string, limit int, gitignore bool) ([]string, bool, error) { // Normalize pattern to forward slashes on Windows so their config can use // backslashes pattern = filepath.ToSlash(pattern) @@ -101,11 +113,11 @@ func GlobWithDoubleStar(pattern, searchPath string, limit int) ([]string, bool, isDir := d.IsDir() if isDir { - if walker.ShouldSkipDir(path) { + if gitignore && walker.ShouldSkipDir(path) { return filepath.SkipDir } } else { - if walker.ShouldSkip(path) { + if gitignore && walker.ShouldSkip(path) { return nil } } diff --git a/internal/fsext/fileutil_test.go b/internal/fsext/fileutil_test.go index 3788fe5477b082dec496275a8ac028788d55fc64..e80b902c08301be3a6b02e9a8d0e9c82895410b3 100644 --- a/internal/fsext/fileutil_test.go +++ b/internal/fsext/fileutil_test.go @@ -24,7 +24,7 @@ func TestGlobWithDoubleStar(t *testing.T) { require.NoError(t, os.WriteFile(file, []byte("test content"), 0o644)) } - matches, truncated, err := GlobWithDoubleStar("**/main.go", testDir, 0) + matches, truncated, err := GlobGitignoreAware("**/main.go", testDir, 0) require.NoError(t, err) require.False(t, truncated) @@ -47,7 +47,7 @@ func TestGlobWithDoubleStar(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(srcDir, "main.go"), []byte("package main"), 0o644)) require.NoError(t, os.WriteFile(pkgFile, []byte("test"), 0o644)) - matches, truncated, err := GlobWithDoubleStar("pkg", testDir, 0) + matches, truncated, err := GlobGitignoreAware("pkg", testDir, 0) require.NoError(t, err) require.False(t, truncated) @@ -66,7 +66,7 @@ func TestGlobWithDoubleStar(t *testing.T) { require.NoError(t, os.MkdirAll(dir, 0o755)) } - matches, truncated, err := GlobWithDoubleStar("**/pkg", testDir, 0) + matches, truncated, err := GlobGitignoreAware("**/pkg", testDir, 0) require.NoError(t, err) require.False(t, truncated) @@ -95,7 +95,7 @@ func TestGlobWithDoubleStar(t *testing.T) { require.NoError(t, os.WriteFile(file, []byte("package main"), 0o644)) } - matches, truncated, err := GlobWithDoubleStar("pkg/**", testDir, 0) + matches, truncated, err := GlobGitignoreAware("pkg/**", testDir, 0) require.NoError(t, err) require.False(t, truncated) @@ -124,7 +124,7 @@ func TestGlobWithDoubleStar(t *testing.T) { require.NoError(t, os.WriteFile(file, []byte("test"), 0o644)) } - matches, truncated, err := GlobWithDoubleStar("**/*.txt", testDir, 5) + matches, truncated, err := GlobGitignoreAware("**/*.txt", testDir, 5) require.NoError(t, err) require.True(t, truncated, "Expected truncation with limit") require.Len(t, matches, 5, "Expected exactly 5 matches with limit") @@ -143,7 +143,7 @@ func TestGlobWithDoubleStar(t *testing.T) { require.NoError(t, os.WriteFile(file, []byte("test"), 0o644)) } - matches, truncated, err := GlobWithDoubleStar("a/b/c/file1.txt", testDir, 0) + matches, truncated, err := GlobGitignoreAware("a/b/c/file1.txt", testDir, 0) require.NoError(t, err) require.False(t, truncated) @@ -171,7 +171,7 @@ func TestGlobWithDoubleStar(t *testing.T) { require.NoError(t, os.Chtimes(file2, m2, m2)) require.NoError(t, os.Chtimes(file3, m3, m3)) - matches, truncated, err := GlobWithDoubleStar("*.txt", testDir, 0) + matches, truncated, err := GlobGitignoreAware("*.txt", testDir, 0) require.NoError(t, err) require.False(t, truncated) @@ -181,7 +181,7 @@ func TestGlobWithDoubleStar(t *testing.T) { t.Run("handles empty directory", func(t *testing.T) { testDir := t.TempDir() - matches, truncated, err := GlobWithDoubleStar("**", testDir, 0) + matches, truncated, err := GlobGitignoreAware("**", testDir, 0) require.NoError(t, err) require.False(t, truncated) // Even empty directories should return the directory itself @@ -191,7 +191,7 @@ func TestGlobWithDoubleStar(t *testing.T) { t.Run("handles non-existent search path", func(t *testing.T) { nonExistentDir := filepath.Join(t.TempDir(), "does", "not", "exist") - matches, truncated, err := GlobWithDoubleStar("**", nonExistentDir, 0) + matches, truncated, err := GlobGitignoreAware("**", nonExistentDir, 0) require.Error(t, err, "Should return error for non-existent search path") require.False(t, truncated) require.Empty(t, matches) @@ -219,17 +219,17 @@ func TestGlobWithDoubleStar(t *testing.T) { ignoredFileInDir := filepath.Join(testDir, "backup", "old.txt") require.NoError(t, os.WriteFile(ignoredFileInDir, []byte("old content"), 0o644)) - matches, truncated, err := GlobWithDoubleStar("*.tmp", testDir, 0) + matches, truncated, err := GlobGitignoreAware("*.tmp", testDir, 0) require.NoError(t, err) require.False(t, truncated) require.Empty(t, matches, "Expected no matches for '*.tmp' pattern (should be ignored)") - matches, truncated, err = GlobWithDoubleStar("backup", testDir, 0) + matches, truncated, err = GlobGitignoreAware("backup", testDir, 0) require.NoError(t, err) require.False(t, truncated) require.Empty(t, matches, "Expected no matches for 'backup' pattern (should be ignored)") - matches, truncated, err = GlobWithDoubleStar("*.txt", testDir, 0) + matches, truncated, err = GlobGitignoreAware("*.txt", testDir, 0) require.NoError(t, err) require.False(t, truncated) require.Equal(t, []string{goodFile}, matches) @@ -257,7 +257,7 @@ func TestGlobWithDoubleStar(t *testing.T) { require.NoError(t, os.Chtimes(middleDir, tMiddle, tMiddle)) require.NoError(t, os.Chtimes(oldestFile, tNewest, tNewest)) - matches, truncated, err := GlobWithDoubleStar("*.rs", testDir, 0) + matches, truncated, err := GlobGitignoreAware("*.rs", testDir, 0) require.NoError(t, err) require.False(t, truncated) require.Len(t, matches, 3) diff --git a/internal/lsp/client.go b/internal/lsp/client.go index c82dffabf40e99dc932e2fd326b24031d3e04ebb..3d7f07f977b1ef61bd20652d6bdc6a9b3643383a 100644 --- a/internal/lsp/client.go +++ b/internal/lsp/client.go @@ -85,7 +85,7 @@ func New( resolver: resolver, cwd: cwd, } - client.serverState.Store(StateStarting) + client.serverState.Store(StateStopped) if err := client.createPowernapClient(); err != nil { return nil, err diff --git a/internal/lsp/manager.go b/internal/lsp/manager.go index efa7596a685786e3a3c4053eb94f8858c7549e9f..381700d1299454bd241aeeaf6525686e6c44d45c 100644 --- a/internal/lsp/manager.go +++ b/internal/lsp/manager.go @@ -21,13 +21,14 @@ import ( "github.com/sourcegraph/jsonrpc2" ) +var unavailable = csync.NewMap[string, struct{}]() + // Manager handles lazy initialization of LSP clients based on file types. type Manager struct { clients *csync.Map[string, *Client] cfg *config.Config manager *powernapconfig.Manager callback func(name string, client *Client) - mu sync.Mutex } // NewManager creates a new LSP manager service. @@ -72,17 +73,12 @@ func (s *Manager) Clients() *csync.Map[string, *Client] { // SetCallback sets a callback that is invoked when a new LSP // client is successfully started. This allows the coordinator to add LSP tools. func (s *Manager) SetCallback(cb func(name string, client *Client)) { - s.mu.Lock() - defer s.mu.Unlock() s.callback = cb } // TrackConfigured will callback the user-configured LSPs, but will not create // any clients. func (s *Manager) TrackConfigured() { - s.mu.Lock() - defer s.mu.Unlock() - var wg sync.WaitGroup for name := range s.manager.GetServers() { if !s.isUserConfigured(name) { @@ -102,16 +98,10 @@ func (s *Manager) Start(ctx context.Context, path string) { return } - s.mu.Lock() - defer s.mu.Unlock() - var wg sync.WaitGroup for name, server := range s.manager.GetServers() { - if !handles(server, path, s.cfg.WorkingDir()) { - continue - } wg.Go(func() { - s.startServer(ctx, name, server) + s.startServer(ctx, name, path, server) }) } wg.Wait() @@ -149,12 +139,31 @@ var skipAutoStartCommands = map[string]bool{ "tflint": true, } -func (s *Manager) startServer(ctx context.Context, name string, server *powernapconfig.ServerConfig) { +func (s *Manager) startServer(ctx context.Context, name, filepath string, server *powernapconfig.ServerConfig) { + cfg := s.buildConfig(name, server) + if cfg.Disabled { + return + } + + if _, exists := unavailable.Get(name); exists { + return + } + + if client, ok := s.clients.Get(name); ok { + switch client.GetServerState() { + case StateReady, StateStarting, StateDisabled: + s.callback(name, client) + // already done, return + return + } + } + userConfigured := s.isUserConfigured(name) if !userConfigured { if _, err := exec.LookPath(server.Command); err != nil { slog.Debug("LSP server not installed, skipping", "name", name, "command", server.Command) + unavailable.Set(name, struct{}{}) return } if skipAutoStartCommands[server.Command] { @@ -163,34 +172,43 @@ func (s *Manager) startServer(ctx context.Context, name string, server *powernap } } - cfg := s.buildConfig(name, server) - if client, ok := s.clients.Get(name); ok { - switch client.GetServerState() { - case StateReady, StateStarting: - s.callback(name, client) - // already done, return - return - } + // this is the slowest bit, so we do it last. + if !handles(server, filepath, s.cfg.WorkingDir()) { + // nothing to do + return } - client, err := New( - ctx, - name, - cfg, - s.cfg.Resolver(), - s.cfg.WorkingDir(), - s.cfg.Options.DebugLSP, - ) + + // check again in case another goroutine started it in the meantime + var err error + client := s.clients.GetOrSet(name, func() *Client { + var cli *Client + cli, err = New( + ctx, + name, + cfg, + s.cfg.Resolver(), + s.cfg.WorkingDir(), + s.cfg.Options.DebugLSP, + ) + return cli + }) if err != nil { slog.Error("Failed to create LSP client", "name", name, "error", err) return } - s.callback(name, client) - defer func() { - s.clients.Set(name, client) s.callback(name, client) }() + switch client.GetServerState() { + case StateReady, StateStarting: + // already done, return + return + } + + client.serverState.Store(StateStarting) + s.callback(name, client) + initCtx, cancel := context.WithTimeout(ctx, time.Duration(cmp.Or(cfg.Timeout, 30))*time.Second) defer cancel() @@ -271,7 +289,7 @@ func hasRootMarkers(dir string, markers []string) bool { } for _, pattern := range markers { // Use fsext.GlobWithDoubleStar to find matches - matches, _, err := fsext.GlobWithDoubleStar(pattern, dir, 1) + matches, _, err := fsext.Glob(pattern, dir, 1) if err == nil && len(matches) > 0 { return true } @@ -291,9 +309,6 @@ func handles(server *powernapconfig.ServerConfig, filePath, workDir string) bool // in the middle of writing something. // Generally it doesn't matter when shutting down Crush, though. func (s *Manager) KillAll(context.Context) { - s.mu.Lock() - defer s.mu.Unlock() - var wg sync.WaitGroup for name, client := range s.clients.Seq2() { wg.Go(func() { @@ -308,9 +323,6 @@ func (s *Manager) KillAll(context.Context) { // StopAll stops all running LSP clients and clears the client map. func (s *Manager) StopAll(ctx context.Context) { - s.mu.Lock() - defer s.mu.Unlock() - var wg sync.WaitGroup for name, client := range s.clients.Seq2() { wg.Go(func() { diff --git a/internal/ui/model/lsp.go b/internal/ui/model/lsp.go index 87de0d39d20520b3b24f5da3861efe7d5f9fe4a5..1458d3402cbfa1536e5bef31f7d72ac5d58dddfe 100644 --- a/internal/ui/model/lsp.go +++ b/internal/ui/model/lsp.go @@ -108,7 +108,7 @@ func lspList(t *styles.Styles, lsps []LSPInfo, width, maxItems int) string { icon = t.ResourceOfflineIcon.Foreground(t.Muted.GetBackground()).String() description = t.ResourceStatus.Render("disabled") default: - icon = t.ResourceOfflineIcon.String() + continue } renderedLsps = append(renderedLsps, common.Status(t, common.StatusOpts{ Icon: icon,