f5b996b · crush · git.iko.soy

git.iko.soy/crush

on main help sign in

perf(config): make model selection and config reload fast

f5b996bKieran Klukas4 months ago

internal/config/load.go+50−5

compare with main

unifiedsplit

@@ -572,11 +572,33 @@ func (c *Config) setDefaults(workingDir, dataDir string) {

572 c.Options.InitializeAs = cmp.Or(c.Options.InitializeAs, defaultInitializeAs)

573}

574

575+// powernapDefaults caches the powernap default LSP server catalog. The

576+// catalog is static and immutable for the life of the process, but

577+// building it (NewManager + LoadDefaults) is expensive and was previously

578+// repeated on every config reload. We load it once and only ever read from

579+// it via GetServer, so a shared instance is safe.

580+var (

581+ powernapDefaultsOnce sync.Once

582+ powernapDefaults *powernapConfig.Manager

583+)

584+

585+func lspDefaultsManager() *powernapConfig.Manager {

586+ powernapDefaultsOnce.Do(func() {

587+ m := powernapConfig.NewManager()

588+ // LoadDefaults only fails on malformed embedded defaults, which

589+ // would be a build-time bug; treat the manager as usable either

590+ // way so a transient error never wedges config loading.

591+ _ = m.LoadDefaults()

592+ powernapDefaults = m

593+ })

594+ return powernapDefaults

595+}

596+

575// applyLSPDefaults applies default values from powernap to LSP configurations

597// applyLSPDefaults applies default values from powernap to LSP configurations

576func (c *Config) applyLSPDefaults() {

598func (c *Config) applyLSPDefaults() {

577− // Get powernap's default configuration

599+ // Reuse the process-wide default catalog; building it per reload was a

578− configManager := powernapConfig.NewManager()

600+ // significant chunk of reload latency.

579− configManager.LoadDefaults()

601+ configManager := lspDefaultsManager()

580

602

581 // Apply defaults to each LSP configuration

603 // Apply defaults to each LSP configuration

582 for name, cfg := range c.LSP {

604 for name, cfg := range c.LSP {

@@ -900,10 +922,18 @@ func hasAWSCredentials(env env.Env) bool {

900 return true

922 return true

901 }

923 }

902

924

925+ // File-based credential discovery requires filesystem stats, so do it

926+ // last and skip it under test. Checking testing.Testing() before the

927+ // os.Stat call (rather than after, in the && tail) ensures the syscall

928+ // is never issued during tests, where it otherwise ran unconditionally

929+ // and only had its result discarded.

930+ if testing.Testing() {

931+ return false

932+ }

903− if _, err := os.Stat(filepath.Join(home.Dir(), ".aws/credentials")); err == nil && !testing.Testing() {

933+ if _, err := os.Stat(filepath.Join(home.Dir(), ".aws/credentials")); err == nil {

904 return true

934 return true

905 }

935 }

906− if _, err := os.Stat(filepath.Join(home.Dir(), ".aws/login")); err == nil && !testing.Testing() {

936+ if _, err := os.Stat(filepath.Join(home.Dir(), ".aws/login")); err == nil {

907 return true

937 return true

908 }

938 }

909

939

@@ -1058,7 +1088,22 @@ func isInsideWorktree() bool {

1058// repositories, missing git binary, plain directories, or any other

1088// repositories, missing git binary, plain directories, or any other

1059// failure mode). Linked worktrees and submodules each report their own

1089// failure mode). Linked worktrees and submodules each report their own

1060// top-level, which is what callers want when bounding lookups.

1090// top-level, which is what callers want when bounding lookups.

1091+// worktreeRootCache memoizes the git worktree root per directory. The root

1092+// is stable for the life of the process, so we avoid re-shelling out to

1093+// "git rev-parse" on every config reload. Keyed by the requested dir; the

1094+// value is the resolved root ("" when dir is not in a git worktree).

1095+var worktreeRootCache sync.Map // map[string]string

1096+

1061func worktreeRoot(dir string) string {

1097func worktreeRoot(dir string) string {

1098+ if cached, ok := worktreeRootCache.Load(dir); ok {

1099+ return cached.(string)

1100+ }

1101+ root := computeWorktreeRoot(dir)

1102+ worktreeRootCache.Store(dir, root)

1103+ return root

1104+}

1105+

1106+func computeWorktreeRoot(dir string) string {

1062 cmd := exec.CommandContext(

1107 cmd := exec.CommandContext(

1063 context.Background(),

1108 context.Background(),

1064 "git", "rev-parse", "--show-toplevel",

1109 "git", "rev-parse", "--show-toplevel",

internal/config/store.go+62−8

@@ -221,6 +221,16 @@ func (s *ConfigStore) SetConfigField(scope Scope, key string, value any) error {

221// The write is protected by an in-process mutex and a cross-process flock

222// to prevent races between concurrent writers in different processes.

223func (s *ConfigStore) SetConfigFields(scope Scope, kv map[string]any) error {

224+ return s.setConfigFields(scope, kv, true)

225+}

226+

227+// setConfigFields performs the write and, when reload is true, refreshes

228+// in-memory state from disk. Callers that have already updated the relevant

229+// in-memory state themselves can pass reload=false to skip the expensive

230+// full reparse (which rebuilds the provider catalog and agents). The disk

231+// is the source of truth for other processes either way; skipping reload

232+// only avoids re-deriving state this process already holds.

233+func (s *ConfigStore) setConfigFields(scope Scope, kv map[string]any, reload bool) error {

224 // Sort keys for deterministic output regardless of map iteration

234 // Sort keys for deterministic output regardless of map iteration

225 // order. This also ensures consistent results when callers pass

235 // order. This also ensures consistent results when callers pass

226 // overlapping JSONPath keys (e.g. "a" and "a.b").

236 // overlapping JSONPath keys (e.g. "a" and "a.b").

@@ -245,6 +255,16 @@ func (s *ConfigStore) SetConfigFields(scope Scope, kv map[string]any) error {

245 return err

255 return err

246 }

256 }

247

257

258+ if !reload {

259+ // The caller has already updated in-memory state; just refresh

260+ // the staleness snapshot so the file watcher does not treat our

261+ // own write as an external change.

262+ if path, perr := s.configPath(scope); perr == nil {

263+ s.captureStalenessSnapshot(append(slices.Clone(s.loadedPaths), path))

264+ }

265+ return nil

266+ }

267+

248 // Auto-reload to keep in-memory state fresh after config edits.

268 // Auto-reload to keep in-memory state fresh after config edits.

249 // We use context.Background() since this is an internal operation that

269 // We use context.Background() since this is an internal operation that

250 // shouldn't be cancelled by user context.

270 // shouldn't be cancelled by user context.

@@ -281,14 +301,30 @@ func (s *ConfigStore) RemoveConfigField(scope Scope, key string) error {

281}

301}

282

302

283// UpdatePreferredModel updates the preferred model for the given type and

303// UpdatePreferredModel updates the preferred model for the given type and

284−// persists it to the config file at the given scope.

304+// persists it to the config file at the given scope. The selected model and

305+// the recent-models list are written together in a single config write.

306+//

307+// The in-memory config is updated directly here, so the write skips the

308+// full disk reparse/reload. The reload would otherwise rebuild the entire

309+// provider catalog and agents on every model switch, which dominates the

310+// latency of selecting a model. Agents are refreshed separately by the

311+// caller (see UpdateAgentModel).

285func (s *ConfigStore) UpdatePreferredModel(scope Scope, modelType SelectedModelType, model SelectedModel) error {

312func (s *ConfigStore) UpdatePreferredModel(scope Scope, modelType SelectedModelType, model SelectedModel) error {

286 s.config.Models[modelType] = model

313 s.config.Models[modelType] = model

314+

315+ fields := map[string]any{

287− if err := s.SetConfigField(scope, fmt.Sprintf("models.%s", modelType), model); err != nil {

316+ fmt.Sprintf("models.%s", modelType): model,

288− return fmt.Errorf("failed to update preferred model: %w", err)

289 }

317 }

290− if err := s.recordRecentModel(scope, modelType, model); err != nil {

318+

291− return err

319+ // Compute the updated recent-models list in memory. When it changes,

320+ // fold the write into the same batch so we avoid a second write.

321+ if updated, changed := s.nextRecentModels(modelType, model); changed {

322+ s.config.RecentModels[modelType] = updated

323+ fields[fmt.Sprintf("recent_models.%s", modelType)] = updated

324+ }

325+

326+ if err := s.setConfigFields(scope, fields, false); err != nil {

327+ return fmt.Errorf("failed to update preferred model: %w", err)

292 }

328 }

293 return nil

329 return nil

294}

330}

@@ -504,10 +540,15 @@ func (s *ConfigStore) loadTokenFromDisk(scope Scope, providerID string) (*oauth.

504 return &token, nil

540 return &token, nil

505}

541}

506

542

507−// recordRecentModel records a model in the recent models list.

543+// nextRecentModels computes the recent-models list for the given type

544+// after recording the supplied model at the front, without persisting

545+// anything. It returns the new slice and whether it differs from the

546+// current list. It also lazily initializes the RecentModels map. Callers

547+// that want to persist the change can fold the result into a batched

548+// config write; recordRecentModel wraps it for the standalone case.

508−func (s *ConfigStore) recordRecentModel(scope Scope, modelType SelectedModelType, model SelectedModel) error {

549+func (s *ConfigStore) nextRecentModels(modelType SelectedModelType, model SelectedModel) ([]SelectedModel, bool) {

509 if model.Provider == "" || model.Model == "" {

550 if model.Provider == "" || model.Model == "" {

510− return nil

551+ return nil, false

511 }

552 }

512

553

513 if s.config.RecentModels == nil {

554 if s.config.RecentModels == nil {

@@ -534,6 +575,19 @@ func (s *ConfigStore) recordRecentModel(scope Scope, modelType SelectedModelType

534 }

575 }

535

576

536 if slices.EqualFunc(current, updated, eq) {

577 if slices.EqualFunc(current, updated, eq) {

578+ return current, false

579+ }

580+

581+ return updated, true

582+}

583+

584+// recordRecentModel records a model in the recent models list and persists

585+// the change. It is retained for callers that update recent models in

586+// isolation; UpdatePreferredModel batches the same change into a single

587+// config write instead.

588+func (s *ConfigStore) recordRecentModel(scope Scope, modelType SelectedModelType, model SelectedModel) error {

589+ updated, changed := s.nextRecentModels(modelType, model)

590+ if !changed {

537 return nil

591 return nil

538 }

592 }

539

593

internal/config/update_model_bench_test.gonew file+108

@@ -0,0 +1,108 @@

1+package config

3+import (

4+ "context"

5+ "os"

6+ "path/filepath"

7+ "testing"

8+)

10+// BenchmarkUpdatePreferredModel measures the full write cost of a single

11+// model selection, which is what the user experiences when picking a model

12+// in the TUI. It guards against regressing back to a full config reload on

13+// every selection.

14+func BenchmarkUpdatePreferredModel(b *testing.B) {

15+ dir := b.TempDir()

16+ configPath := filepath.Join(dir, "crush.json")

17+

18+ b.Setenv("CRUSH_GLOBAL_CONFIG", dir)

19+ b.Setenv("CRUSH_GLOBAL_DATA", dir)

20+ resetProviderState()

21+ b.Cleanup(resetProviderState)

22+

23+ cfg := `{

24+ "models": {

25+ "large": {"provider": "openai", "model": "gpt-4"},

26+ "small": {"provider": "openai", "model": "gpt-4o-mini"}

27+ },

28+ "providers": {

29+ "openai": {

30+ "api_key": "test-key",

31+ "models": [

32+ {"id": "gpt-4", "name": "GPT-4"},

33+ {"id": "gpt-4o", "name": "GPT-4o"},

34+ {"id": "gpt-4o-mini", "name": "GPT-4o mini"}

35+ ]

36+ },

37+ "anthropic": {

38+ "api_key": "test-key-2",

39+ "models": [

40+ {"id": "claude-3", "name": "Claude 3"},

41+ {"id": "claude-3-5", "name": "Claude 3.5"}

42+ ]

43+ }

44+ }

45+ }`

46+ if err := os.WriteFile(configPath, []byte(cfg), 0o600); err != nil {

47+ b.Fatal(err)

48+ }

49+

50+ store, err := Load(dir, dir, false)

51+ if err != nil {

52+ b.Fatal(err)

53+ }

54+ store.globalDataPath = configPath

55+

56+ models := []SelectedModel{

57+ {Provider: "openai", Model: "gpt-4"},

58+ {Provider: "anthropic", Model: "claude-3"},

59+ {Provider: "openai", Model: "gpt-4o"},

60+ {Provider: "anthropic", Model: "claude-3-5"},

61+ }

62+

63+ b.ReportAllocs()

64+ i := 0

65+ for b.Loop() {

66+ m := models[i%len(models)]

67+ if err := store.UpdatePreferredModel(ScopeGlobal, SelectedModelTypeLarge, m); err != nil {

68+ b.Fatal(err)

69+ }

70+ i++

71+ }

72+}

73+

74+// BenchmarkReloadFromDisk isolates the full reload cost for comparison with

75+// BenchmarkUpdatePreferredModel.

76+func BenchmarkReloadFromDisk(b *testing.B) {

77+ dir := b.TempDir()

78+ configPath := filepath.Join(dir, "crush.json")

79+

80+ b.Setenv("CRUSH_GLOBAL_CONFIG", dir)

81+ b.Setenv("CRUSH_GLOBAL_DATA", dir)

82+ resetProviderState()

83+ b.Cleanup(resetProviderState)

84+

85+ cfg := `{

86+ "models": {"large": {"provider": "openai", "model": "gpt-4"}},

87+ "providers": {

88+ "openai": {"api_key": "test-key", "models": [{"id": "gpt-4", "name": "GPT-4"}]}

89+ }

90+ }`

91+ if err := os.WriteFile(configPath, []byte(cfg), 0o600); err != nil {

92+ b.Fatal(err)

93+ }

94+

95+ store, err := Load(dir, dir, false)

96+ if err != nil {

97+ b.Fatal(err)

98+ }

99+ store.globalDataPath = configPath

100+

101+ ctx := context.Background()

102+ b.ReportAllocs()

103+ for b.Loop() {

104+ if err := store.ReloadFromDisk(ctx); err != nil {

105+ b.Fatal(err)

106+ }

107+ }

108+}

internal/shell/expand.go+19−2

@@ -59,6 +59,15 @@ var NoUnset atomic.Bool

59// prefix of its stderr. Callers that surface the error to users

60// should additionally scrub it for the original template text.

61func ExpandValue(ctx context.Context, value string, env []string) (string, error) {

62+ // Fast path: a value with no shell metacharacters expands to itself.

63+ // Parsing and running it through the interpreter would produce the

64+ // same string at significant cost. Most config values (literal API

65+ // keys, fixed URLs) hit this path, which matters because ExpandValue

66+ // runs over every provider/MCP/LSP value on each config reload.

67+ if !strings.ContainsAny(value, "$`\\'\"") {

68+ return value, nil

69+ }

70+

62 // Parse the value as a here-doc style word: no word splitting, no

71 // Parse the value as a here-doc style word: no word splitting, no

63 // globbing, but full support for $VAR, ${VAR...}, $(...), and

72 // globbing, but full support for $VAR, ${VAR...}, $(...), and

64 // quoted/escaped strings.

73 // quoted/escaped strings.

@@ -73,12 +82,16 @@ func ExpandValue(ctx context.Context, value string, env []string) (string, error

73 // verbatim, with no CRUSH/AGENT/AI_AGENT injection: callers of

82 // verbatim, with no CRUSH/AGENT/AI_AGENT injection: callers of

74 // ExpandValue control the env, and nounset must treat any name

83 // ExpandValue control the env, and nounset must treat any name

75 // not in env as unset.

84 // not in env as unset.

76− cwd, _ := os.Getwd()

85+ //

86+ // The working directory is only needed for $(...) command

87+ // substitution, so we resolve it lazily on first use. Values that

88+ // only reference variables or use quoting (the common case) avoid

89+ // the os.Getwd() syscall entirely.

77 s := &Shell{

90 s := &Shell{

78− cwd: cwd,

79 env: env,

91 env: env,

80 logger: noopLogger{},

92 logger: noopLogger{},

81 }

93 }

94+ cwdResolved := false

83 strict := NoUnset.Load()

96 strict := NoUnset.Load()

@@ -87,6 +100,10 @@ func ExpandValue(ctx context.Context, value string, env []string) (string, error

87 Env: expand.ListEnviron(env...),

100 Env: expand.ListEnviron(env...),

88 NoUnset: strict,

101 NoUnset: strict,

89 CmdSubst: func(w io.Writer, cs *syntax.CmdSubst) error {

102 CmdSubst: func(w io.Writer, cs *syntax.CmdSubst) error {

103+ if !cwdResolved {

104+ s.cwd, _ = os.Getwd()

105+ cwdResolved = true

106+ }

90 stderrBuf.Reset()

107 stderrBuf.Reset()

91 runnerOpts := []interp.RunnerOption{

108 runnerOpts := []interp.RunnerOption{

92 interp.StdIO(nil, w, &stderrBuf),

109 interp.StdIO(nil, w, &stderrBuf),