perf: implement unified cache system and optimize core operations #38
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a unified cache system to consolidate three previously fragmented caches (in-memory installed packages, pkg_versions.json, and outdated cache) into a single source of truth with TTL-based refresh. Additionally, it implements logger optimizations with fast-path writes, cleans up generic helper functions, optimizes middleware allocations, and fixes format string inconsistencies across the codebase.
Changes:
- Unified cache system (
internal/brew/unified_cache.go) consolidating package state management - Logger optimization with fast-path direct writes bypassing zap for non-JSON mode
- Inline fusion of Filter+Keys operations eliminating intermediate slice allocations
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/brew/unified_cache.go | New 524-line unified cache implementation with TTL, singleton pattern, and disk persistence |
| internal/versions/versions.go | Refactored to use unified cache instead of separate pkg_versions.json file |
| internal/logger/logger.go | Added fast-path direct writes, moved formatting outside locks, fixed verbose flag |
| internal/middleware/middleware.go | Pre-copies middleware slice to reduce allocations during command execution |
| internal/utils/generic_helpers.go | Removed unused Filter/Keys/Some/Includes/Flat helpers |
| internal/utils/homebrew.go | Inline extraction of keys to avoid generic helper overhead |
| internal/core/core.go | Updated to use unified cache, removed installedPkgs field |
| internal/list/manager.go | Inlined computeDeps for single-pass filtering |
| internal/upgrade/manager.go | Inlined computeDeps for single-pass filtering, fixed logger format string |
| internal/brew/state.go | Refactored to wrap unified cache for backward compatibility |
| internal/core/core_test.go | Adapted tests to reset unified cache and use new mock patterns |
| internal/upgrade/upgrade_test.go | Replaced writeOutdatedCache with setupOutdatedMocks for unified cache |
| internal/utils/*.go | Fixed logger format strings from %w to %v |
| internal/notifier/notifier.go | Fixed logger format string |
| internal/checker/checker.go | Fixed logger format string capitalization |
| internal/middleware/homebrewCheck.go | Fixed logger format string |
| cmd/keg/main.go | Fixed logger format string |
Comments suppressed due to low confidence (1)
internal/core/core.go:462
- After calling
MarkUninstalled(line 457), the code also callsversions.NewResolver(b.Runner).Remove(execName)(line 459). This creates a new resolver instance and calls Remove on it, which will callInvalidateon the same unified cache. SinceMarkUninstalledalready removed the package from the unified cache, the subsequentRemovecall is redundant. Consider removing the call toversions.NewResolver(b.Runner).Remove(execName)or clarify if it serves a different purpose (e.g., cleaning up legacy cache files).
case "uninstall":
// Update unified cache to reflect removal
if b.cache != nil {
_ = b.cache.MarkUninstalled(execName)
}
err = versions.NewResolver(b.Runner).Remove(execName)
if err != nil {
logger.Debug("versions.Remove failed for %s: %v", execName, err)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var chain func(int) error | ||
| chain = func(i int) error { | ||
| if i >= mwLen { | ||
| if orig != nil { | ||
| return orig(c, a) | ||
| } | ||
| return nil | ||
| } | ||
| return mwCopy[i](c, a, func(nc *cobra.Command, na []string) error { | ||
| return chain(i + 1) | ||
| }) | ||
| } | ||
| return chain(0) |
There was a problem hiding this comment.
The closure chain captures variables c and a from the outer scope but the middleware function signature expects nc and na parameters. However, on line 57, the closure ignores these parameters and recursively calls chain(i + 1) without using nc or na. This means if a middleware modifies the command or args, those changes won't propagate through the chain. The closure should use the parameters: return chain(nc, na, i + 1) and the chain function should accept those parameters.
| var chain func(int) error | |
| chain = func(i int) error { | |
| if i >= mwLen { | |
| if orig != nil { | |
| return orig(c, a) | |
| } | |
| return nil | |
| } | |
| return mwCopy[i](c, a, func(nc *cobra.Command, na []string) error { | |
| return chain(i + 1) | |
| }) | |
| } | |
| return chain(0) | |
| var chain func(curCmd *cobra.Command, curArgs []string, i int) error | |
| chain = func(curCmd *cobra.Command, curArgs []string, i int) error { | |
| if i >= mwLen { | |
| if orig != nil { | |
| return orig(curCmd, curArgs) | |
| } | |
| return nil | |
| } | |
| return mwCopy[i](curCmd, curArgs, func(nc *cobra.Command, na []string) error { | |
| return chain(nc, na, i+1) | |
| }) | |
| } | |
| return chain(c, a, 0) |
| func splitLines(s string) []string { | ||
| result := make([]string, 0) | ||
| current := "" | ||
| for _, r := range s { | ||
| if r == '\n' { | ||
| trimmed := trimSpace(current) | ||
| if trimmed != "" { | ||
| result = append(result, trimmed) | ||
| } | ||
| current = "" | ||
| } else { | ||
| current += string(r) | ||
| } | ||
| } |
There was a problem hiding this comment.
The splitLines function builds strings inefficiently by concatenating runes using current += string(r) in a loop. This creates many intermediate string allocations. For better performance, consider using a strings.Builder or collecting runes in a slice and converting once at the end of each line. Given this PR's focus on performance optimization, this implementation contradicts the stated goals.
| func (c *UnifiedCache) MarkUpgraded(name, newVersion string) error { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
|
|
||
| state := c.Packages[name] | ||
| state.InstalledVersion = newVersion | ||
| state.LatestVersion = newVersion | ||
| state.Outdated = false | ||
| state.FetchedAt = time.Now() | ||
|
|
||
| c.Packages[name] = state |
There was a problem hiding this comment.
The MarkUpgraded function reads from c.Packages[name] which returns a zero-value PackageState if the package doesn't exist in the cache. This means the Installed field will be false for a package that was just upgraded. After upgrade, the package should definitely be marked as installed. The function should explicitly set state.Installed = true.
| func Info(msg string, args ...interface{}) { | ||
| if !ensureReady() { | ||
| return | ||
| } | ||
| // Format OUTSIDE the lock (80% of the work) | ||
| formatted := formatMessage("✨ ", msg, args...) | ||
|
|
||
| // Fast path: direct write for non-JSON mode | ||
| if !useJSON.Load() { | ||
| writeDirect(formatted) | ||
| return | ||
| } | ||
|
|
||
| // JSON mode: use zap | ||
| mu.RLock() | ||
| zlog.Infof(p.Info("✨ "+msg, args...)) | ||
| zlog.Info(formatted) | ||
| mu.RUnlock() | ||
| } | ||
|
|
||
| func Success(msg string, args ...interface{}) { | ||
| if !ensureReady() { | ||
| return | ||
| } | ||
| formatted := formatMessage("✅ ", msg, args...) | ||
|
|
||
| if !useJSON.Load() { | ||
| writeDirect(p.Success(formatted)) | ||
| return | ||
| } | ||
|
|
||
| mu.RLock() | ||
| zlog.Infof(p.Success("✅ "+msg, args...)) | ||
| zlog.Info(p.Success(formatted)) | ||
| mu.RUnlock() | ||
| } | ||
|
|
||
| func LogError(msg string, args ...interface{}) { | ||
| if !ensureReady() { | ||
| return | ||
| } | ||
| formatted := formatMessage("❌ ", msg, args...) | ||
|
|
||
| if !useJSON.Load() { | ||
| writeDirect(p.Error(formatted)) | ||
| return | ||
| } | ||
|
|
||
| mu.RLock() | ||
| zlog.Errorf(p.Error("❌ "+msg, args...)) | ||
| zlog.Error(p.Error(formatted)) | ||
| mu.RUnlock() | ||
| } | ||
|
|
||
| func Warn(msg string, args ...interface{}) { | ||
| if !ensureReady() { | ||
| return | ||
| } | ||
| formatted := formatMessage("⚠️ ", msg, args...) | ||
|
|
||
| if !useJSON.Load() { | ||
| writeDirect(p.Warning(formatted)) | ||
| return | ||
| } | ||
|
|
||
| mu.RLock() | ||
| zlog.Warnf(p.Warning("⚠️ "+msg, args...)) | ||
| zlog.Warn(p.Warning(formatted)) | ||
| mu.RUnlock() | ||
| } | ||
|
|
||
| func Fatal(msg string, args ...interface{}) { | ||
| if !ensureReady() { | ||
| os.Exit(1) | ||
| } | ||
| formatted := formatMessage("💥 ", msg, args...) | ||
|
|
||
| mu.RLock() | ||
| zlog.Fatalf(p.Error("💥 "+msg, args...)) | ||
| zlog.Fatal(p.Error(formatted)) | ||
| mu.RUnlock() | ||
| } | ||
|
|
||
| func WarnInline(msg string, args ...interface{}) { | ||
| if !ensureReady() { | ||
| return | ||
| } | ||
| // inline write directly to out to preserve non-line break semantics | ||
| mu.RLock() | ||
| defer mu.RUnlock() | ||
| if out == nil { | ||
| out = os.Stdout | ||
| } | ||
| _, _ = io.WriteString(out, p.Warning("⚠️ "+msg)) | ||
| formatted := formatMessage("⚠️ ", msg, args...) | ||
| writeDirect(p.Warning(formatted)) |
There was a problem hiding this comment.
The comment says "Format OUTSIDE the lock (80% of the work)" but in all logging functions (Info, Success, LogError, Warn), formatting happens before acquiring the lock. However, for the fast-path which bypasses zap, the formatted message is passed to p.Info(msg) or p.Error(formatted) etc. inside writeDirect, which then applies color formatting again. This results in double-formatting: once with the emoji prefix, then again with color codes. The logger design should either format once completely, or separate concerns more clearly.
| // Fallback to empty cache | ||
| cache = &brew.UnifiedCache{} |
There was a problem hiding this comment.
When GetCache fails, the code creates an empty UnifiedCache{} without initializing critical fields like runner, cachePath, or TTL. This uninitialized cache will cause nil pointer dereferences when its methods are called (e.g., IsInstalled -> Refresh -> c.runner.Run). Instead of creating an empty cache, consider either returning an error from NewBase, using a nil cache with defensive checks, or creating a properly initialized cache with the runner.
| // Fallback to empty cache | |
| cache = &brew.UnifiedCache{} |
| c.mu.RLock() | ||
| needsRefresh := c.needsRefreshLocked() | ||
| c.mu.RUnlock() | ||
|
|
||
| if needsRefresh { | ||
| _ = c.Refresh(context.Background(), false) | ||
| } | ||
|
|
||
| c.mu.RLock() | ||
| defer c.mu.RUnlock() |
There was a problem hiding this comment.
The IsInstalled method has a potential race condition. It releases the read lock after checking needsRefreshLocked() (line 107), then calls Refresh which acquires a write lock. Between releasing the read lock and calling Refresh, another goroutine could acquire the write lock and modify the cache. Then when this goroutine re-acquires the read lock (line 113), it might read stale data. Consider either: 1) calling Refresh without checking first (it checks TTL internally), or 2) keeping the logic but being aware that the refresh might be redundant.
| c.mu.RLock() | |
| needsRefresh := c.needsRefreshLocked() | |
| c.mu.RUnlock() | |
| if needsRefresh { | |
| _ = c.Refresh(context.Background(), false) | |
| } | |
| c.mu.RLock() | |
| defer c.mu.RUnlock() | |
| // Refresh handles TTL checks internally and is concurrency-safe. | |
| _ = c.Refresh(context.Background(), false) | |
| c.mu.RLock() | |
| defer c.mu.RUnlock() |
| func writeDirect(msg string) { | ||
| mu.RLock() | ||
| w := out | ||
| mu.RUnlock() | ||
|
|
||
| if w == nil { | ||
| w = os.Stdout | ||
| } | ||
|
|
||
| // Write directly without holding lock | ||
| _, _ = fmt.Fprintln(w, p.Info(msg)) |
There was a problem hiding this comment.
The writeDirect function accesses the global p (ColorPrinter) without holding a lock. While p is set in Configure under a lock, it could be read during concurrent calls to writeDirect while another goroutine is in Configure. Although the ensureReady() check happens before this, there's still a race window. The p.Info(msg) call on line 298 should either be done under the read lock, or p should be retrieved while holding the lock.
| package brew | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "sync" | ||
| "time" | ||
|
|
||
| "github.com/MrSnakeDoc/keg/internal/logger" | ||
| "github.com/MrSnakeDoc/keg/internal/runner" | ||
| "github.com/MrSnakeDoc/keg/internal/utils" | ||
| ) | ||
|
|
||
| // UnifiedCache is the single source of truth for all brew package states. | ||
| // It consolidates: | ||
| // - Installed packages (previously core.Base.installedPkgs) | ||
| // - Version information (previously versions cache) | ||
| // - Outdated status (previously brew/state.go cache) | ||
| type UnifiedCache struct { | ||
| mu sync.RWMutex | ||
| LastUpdated time.Time `json:"last_updated"` | ||
| Packages map[string]PackageState `json:"packages"` | ||
| TTL time.Duration `json:"-"` | ||
| runner runner.CommandRunner `json:"-"` | ||
| cachePath string `json:"-"` | ||
| } | ||
|
|
||
| // PackageState represents the complete state of a single package. | ||
| type PackageState struct { | ||
| Installed bool `json:"installed"` | ||
| InstalledVersion string `json:"installed_version,omitempty"` | ||
| LatestVersion string `json:"latest_version,omitempty"` | ||
| Outdated bool `json:"outdated,omitempty"` | ||
| FetchedAt time.Time `json:"fetched_at"` | ||
| } | ||
|
|
||
| const ( | ||
| defaultTTL = 1 * time.Hour | ||
| cacheFileName = "unified_cache.json" | ||
| maxBatchSize = 50 | ||
| brewTimeout = 120 * time.Second | ||
| ) | ||
|
|
||
| var ( | ||
| globalCache *UnifiedCache | ||
| globalCacheMu sync.Mutex | ||
| ) | ||
|
|
||
| // GetCache returns the global unified cache instance (singleton). | ||
| func GetCache(r runner.CommandRunner) (*UnifiedCache, error) { | ||
| globalCacheMu.Lock() | ||
| defer globalCacheMu.Unlock() | ||
|
|
||
| if globalCache != nil { | ||
| return globalCache, nil | ||
| } | ||
|
|
||
| home, err := os.UserHomeDir() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get home directory: %w", err) | ||
| } | ||
|
|
||
| stateHome := os.Getenv("XDG_STATE_HOME") | ||
| if stateHome == "" { | ||
| stateHome = filepath.Join(home, ".local", "state") | ||
| } | ||
|
|
||
| dir := filepath.Join(stateHome, "keg") | ||
| if err := os.MkdirAll(dir, 0o755); err != nil { | ||
| return nil, fmt.Errorf("failed to create cache directory: %w", err) | ||
| } | ||
|
|
||
| cachePath := filepath.Join(dir, cacheFileName) | ||
|
|
||
| if r == nil { | ||
| r = &runner.ExecRunner{} | ||
| } | ||
|
|
||
| cache := &UnifiedCache{ | ||
| Packages: make(map[string]PackageState), | ||
| TTL: defaultTTL, | ||
| runner: r, | ||
| cachePath: cachePath, | ||
| } | ||
|
|
||
| // Try to load existing cache from disk | ||
| _ = cache.loadFromDisk() | ||
|
|
||
| globalCache = cache | ||
| return globalCache, nil | ||
| } | ||
|
|
||
| // ResetCache clears the global cache (useful for testing). | ||
| func ResetCache() { | ||
| globalCacheMu.Lock() | ||
| defer globalCacheMu.Unlock() | ||
| globalCache = nil | ||
| } | ||
|
|
||
| // IsInstalled checks if a package is installed. | ||
| func (c *UnifiedCache) IsInstalled(name string) bool { | ||
| c.mu.RLock() | ||
| needsRefresh := c.needsRefreshLocked() | ||
| c.mu.RUnlock() | ||
|
|
||
| if needsRefresh { | ||
| _ = c.Refresh(context.Background(), false) | ||
| } | ||
|
|
||
| c.mu.RLock() | ||
| defer c.mu.RUnlock() | ||
| pkg, ok := c.Packages[name] | ||
| return ok && pkg.Installed | ||
| } | ||
|
|
||
| // GetState returns the complete state for a package. | ||
| func (c *UnifiedCache) GetState(name string) (PackageState, bool) { | ||
| c.mu.RLock() | ||
| defer c.mu.RUnlock() | ||
|
|
||
| pkg, ok := c.Packages[name] | ||
| return pkg, ok | ||
| } | ||
|
|
||
| // GetInstalledSet returns a map of all installed packages. | ||
| func (c *UnifiedCache) GetInstalledSet() map[string]bool { | ||
| c.mu.RLock() | ||
| defer c.mu.RUnlock() | ||
|
|
||
| result := make(map[string]bool, len(c.Packages)) | ||
| for name, pkg := range c.Packages { | ||
| if pkg.Installed { | ||
| result[name] = true | ||
| } | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| // GetOutdatedMap returns packages that need updating. | ||
| func (c *UnifiedCache) GetOutdatedMap() map[string]PackageInfo { | ||
| c.mu.RLock() | ||
| defer c.mu.RUnlock() | ||
|
|
||
| result := make(map[string]PackageInfo) | ||
| for name, pkg := range c.Packages { | ||
| if pkg.Outdated && pkg.Installed { | ||
| result[name] = PackageInfo{ | ||
| Name: name, | ||
| InstalledVersion: pkg.InstalledVersion, | ||
| LatestVersion: pkg.LatestVersion, | ||
| } | ||
| } | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| // Refresh updates the cache by calling brew commands. | ||
| // If force=true, ignores TTL and always refreshes. | ||
| func (c *UnifiedCache) Refresh(ctx context.Context, force bool) error { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
|
|
||
| if !force && !c.needsRefreshLocked() { | ||
| logger.Debug("unified cache is fresh, skipping refresh (age: %s)", time.Since(c.LastUpdated).Truncate(time.Second)) | ||
| return nil | ||
| } | ||
|
|
||
| logger.Debug("refreshing unified cache...") | ||
| start := time.Now() | ||
|
|
||
| // Step 1: Get installed packages (brew list) | ||
| installed, err := c.fetchInstalledPackages() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to fetch installed packages: %w", err) | ||
| } | ||
|
|
||
| // Step 2: Get outdated packages (brew outdated --json=v2) | ||
| outdated, err := c.fetchOutdatedPackages() | ||
| if err != nil { | ||
| logger.Debug("failed to fetch outdated packages (non-fatal): %v", err) | ||
| outdated = &brewOutdatedJSON{} | ||
| } | ||
|
|
||
| // Step 3: Build outdated map | ||
| outdatedMap := make(map[string]PackageInfo) | ||
| for _, f := range outdated.Formulae { | ||
| if len(f.InstalledVersions) > 0 { | ||
| outdatedMap[f.Name] = PackageInfo{ | ||
| Name: f.Name, | ||
| InstalledVersion: f.InstalledVersions[0], | ||
| LatestVersion: f.CurrentVersion, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Step 4: Update cache | ||
| now := time.Now() | ||
| newPackages := make(map[string]PackageState, len(installed)) | ||
|
|
||
| for name := range installed { | ||
| state := PackageState{ | ||
| Installed: true, | ||
| FetchedAt: now, | ||
| } | ||
|
|
||
| // Check if outdated | ||
| if info, isOutdated := outdatedMap[name]; isOutdated { | ||
| state.Outdated = true | ||
| state.InstalledVersion = info.InstalledVersion | ||
| state.LatestVersion = info.LatestVersion | ||
| } else { | ||
| state.Outdated = false | ||
| // Keep existing version info if available | ||
| if old, ok := c.Packages[name]; ok { | ||
| state.InstalledVersion = old.InstalledVersion | ||
| state.LatestVersion = old.LatestVersion | ||
| } | ||
| } | ||
|
|
||
| newPackages[name] = state | ||
| } | ||
|
|
||
| c.Packages = newPackages | ||
| c.LastUpdated = now | ||
|
|
||
| logger.Debug("unified cache refreshed in %s (%d packages)", time.Since(start).Truncate(time.Millisecond), len(c.Packages)) | ||
|
|
||
| // Save to disk | ||
| return c.saveToDiskLocked() | ||
| } | ||
|
|
||
| // RefreshVersions fetches version info for specific packages using brew info. | ||
| func (c *UnifiedCache) RefreshVersions(ctx context.Context, names []string) error { | ||
| if len(names) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
|
|
||
| // Dedupe | ||
| seen := make(map[string]struct{}, len(names)) | ||
| unique := make([]string, 0, len(names)) | ||
| for _, n := range names { | ||
| if _, ok := seen[n]; !ok { | ||
| seen[n] = struct{}{} | ||
| unique = append(unique, n) | ||
| } | ||
| } | ||
|
|
||
| // Batch into chunks | ||
| chunks := chunkStrings(unique, maxBatchSize) | ||
| now := time.Now() | ||
|
|
||
| for _, chunk := range chunks { | ||
| versionInfo, err := c.fetchVersionsForChunk(ctx, chunk) | ||
| if err != nil { | ||
| logger.Debug("failed to fetch versions for chunk: %v", err) | ||
| continue | ||
| } | ||
|
|
||
| // Update cache with version info | ||
| for name, info := range versionInfo { | ||
| state, ok := c.Packages[name] | ||
| if !ok { | ||
| // Package not in cache yet, create entry | ||
| state = PackageState{ | ||
| FetchedAt: now, | ||
| } | ||
| } | ||
|
|
||
| state.InstalledVersion = info.InstalledVersion | ||
| state.LatestVersion = info.LatestVersion | ||
| state.FetchedAt = now | ||
|
|
||
| // CRITICAL: Update installed flag based on version info | ||
| state.Installed = (info.InstalledVersion != "") | ||
|
|
||
| if state.InstalledVersion != "" && state.LatestVersion != "" { | ||
| state.Outdated = state.InstalledVersion != state.LatestVersion | ||
| } | ||
|
|
||
| c.Packages[name] = state | ||
| } | ||
| } | ||
|
|
||
| return c.saveToDiskLocked() | ||
| } | ||
|
|
||
| // Invalidate marks specific packages for refresh. | ||
| func (c *UnifiedCache) Invalidate(names ...string) error { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
|
|
||
| if len(names) == 0 { | ||
| // Invalidate all | ||
| c.LastUpdated = time.Time{} | ||
| return c.saveToDiskLocked() | ||
| } | ||
|
|
||
| // Invalidate specific packages | ||
| for _, name := range names { | ||
| delete(c.Packages, name) | ||
| } | ||
|
|
||
| return c.saveToDiskLocked() | ||
| } | ||
|
|
||
| // MarkInstalled updates the cache after a package installation. | ||
| func (c *UnifiedCache) MarkInstalled(name, version string) error { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
|
|
||
| state := c.Packages[name] | ||
| state.Installed = true | ||
| state.InstalledVersion = version | ||
| state.LatestVersion = version | ||
| state.Outdated = false | ||
| state.FetchedAt = time.Now() | ||
|
|
||
| c.Packages[name] = state | ||
|
|
||
| return c.saveToDiskLocked() | ||
| } | ||
|
|
||
| // MarkUninstalled updates the cache after a package removal. | ||
| func (c *UnifiedCache) MarkUninstalled(name string) error { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
|
|
||
| delete(c.Packages, name) | ||
|
|
||
| return c.saveToDiskLocked() | ||
| } | ||
|
|
||
| // MarkUpgraded updates the cache after a package upgrade. | ||
| func (c *UnifiedCache) MarkUpgraded(name, newVersion string) error { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
|
|
||
| state := c.Packages[name] | ||
| state.InstalledVersion = newVersion | ||
| state.LatestVersion = newVersion | ||
| state.Outdated = false | ||
| state.FetchedAt = time.Now() | ||
|
|
||
| c.Packages[name] = state | ||
|
|
||
| return c.saveToDiskLocked() | ||
| } | ||
|
|
||
| // --- Private methods --- | ||
|
|
||
| func (c *UnifiedCache) needsRefreshLocked() bool { | ||
| return c.LastUpdated.IsZero() || time.Since(c.LastUpdated) > c.TTL | ||
| } | ||
|
|
||
| func (c *UnifiedCache) fetchInstalledPackages() (map[string]bool, error) { | ||
| out, err := c.runner.Run(context.Background(), brewTimeout, runner.Capture, "brew", "list", "--formula", "-1") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return utils.TransformToMap( | ||
| splitLines(string(out)), | ||
| func(line string) (string, bool) { | ||
| return line, true | ||
| }, | ||
| ), nil | ||
| } | ||
|
|
||
| func (c *UnifiedCache) fetchOutdatedPackages() (*brewOutdatedJSON, error) { | ||
| out, err := c.runner.Run(context.Background(), brewTimeout, runner.Capture, "brew", "outdated", "--json=v2") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Find first { | ||
| idx := 0 | ||
| for i, b := range out { | ||
| if b == '{' { | ||
| idx = i | ||
| break | ||
| } | ||
| } | ||
|
|
||
| var result brewOutdatedJSON | ||
| if err := json.Unmarshal(out[idx:], &result); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &result, nil | ||
| } | ||
|
|
||
| func (c *UnifiedCache) fetchVersionsForChunk(ctx context.Context, names []string) (map[string]PackageInfo, error) { | ||
| if len(names) == 0 { | ||
| return map[string]PackageInfo{}, nil | ||
| } | ||
|
|
||
| args := append([]string{"info", "--json=v2"}, names...) | ||
| out, err := c.runner.Run(ctx, brewTimeout, runner.Capture, "brew", args...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var info struct { | ||
| Formulae []struct { | ||
| Name string `json:"name"` | ||
| Versions struct { | ||
| Stable string `json:"stable"` | ||
| } `json:"versions"` | ||
| Installed []struct { | ||
| Version string `json:"version"` | ||
| } `json:"installed"` | ||
| } `json:"formulae"` | ||
| } | ||
|
|
||
| if err := json.Unmarshal(out, &info); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| result := make(map[string]PackageInfo, len(info.Formulae)) | ||
| for _, f := range info.Formulae { | ||
| installed := "" | ||
| if len(f.Installed) > 0 { | ||
| installed = f.Installed[0].Version | ||
| } | ||
|
|
||
| result[f.Name] = PackageInfo{ | ||
| Name: f.Name, | ||
| InstalledVersion: installed, | ||
| LatestVersion: f.Versions.Stable, | ||
| } | ||
| } | ||
|
|
||
| return result, nil | ||
| } | ||
|
|
||
| func (c *UnifiedCache) loadFromDisk() error { | ||
| data, err := os.ReadFile(c.cachePath) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return nil // Fresh start | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
|
|
||
| if err := json.Unmarshal(data, c); err != nil { | ||
| logger.Debug("failed to unmarshal cache (will rebuild): %v", err) | ||
| return nil | ||
| } | ||
|
|
||
| logger.Debug("loaded unified cache from disk (%d packages, age: %s)", len(c.Packages), time.Since(c.LastUpdated).Truncate(time.Second)) | ||
| return nil | ||
| } | ||
|
|
||
| func (c *UnifiedCache) saveToDiskLocked() error { | ||
| data, err := json.MarshalIndent(c, "", " ") | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return os.WriteFile(c.cachePath, data, 0o644) | ||
| } | ||
|
|
||
| // Helper functions | ||
|
|
||
| func splitLines(s string) []string { | ||
| result := make([]string, 0) | ||
| current := "" | ||
| for _, r := range s { | ||
| if r == '\n' { | ||
| trimmed := trimSpace(current) | ||
| if trimmed != "" { | ||
| result = append(result, trimmed) | ||
| } | ||
| current = "" | ||
| } else { | ||
| current += string(r) | ||
| } | ||
| } | ||
| trimmed := trimSpace(current) | ||
| if trimmed != "" { | ||
| result = append(result, trimmed) | ||
| } | ||
|
|
||
| return result | ||
| } | ||
|
|
||
| func trimSpace(s string) string { | ||
| start := 0 | ||
| end := len(s) | ||
|
|
||
| for start < end && (s[start] == ' ' || s[start] == '\t' || s[start] == '\r') { | ||
| start++ | ||
| } | ||
|
|
||
| for end > start && (s[end-1] == ' ' || s[end-1] == '\t' || s[end-1] == '\r') { | ||
| end-- | ||
| } | ||
|
|
||
| return s[start:end] | ||
| } | ||
|
|
||
| func chunkStrings(items []string, size int) [][]string { | ||
| if size <= 0 { | ||
| size = 1 | ||
| } | ||
| chunks := make([][]string, 0, (len(items)+size-1)/size) | ||
| for i := 0; i < len(items); i += size { | ||
| end := i + size | ||
| if end > len(items) { | ||
| end = len(items) | ||
| } | ||
| chunks = append(chunks, items[i:end]) | ||
| } | ||
| return chunks | ||
| } |
There was a problem hiding this comment.
The UnifiedCache struct is a new, significant piece of infrastructure (524 lines) but has no dedicated test file. While integration tests exist that exercise it indirectly, critical functionality like cache refresh logic, TTL expiration, concurrent access patterns, and edge cases (empty packages, failed brew calls, etc.) should have unit tests. This is especially important for a caching system where bugs could cause data inconsistency or stale reads.
| c.Packages = newPackages | ||
| c.LastUpdated = now | ||
|
|
||
| logger.Debug("unified cache refreshed in %s (%d packages)", time.Since(start).Truncate(time.Millisecond), len(c.Packages)) | ||
|
|
||
| // Save to disk | ||
| return c.saveToDiskLocked() |
There was a problem hiding this comment.
The Refresh method completely replaces c.Packages with a new map containing only currently installed packages (line 226). This means any packages that were uninstalled since the last refresh are correctly removed. However, if there's a partial failure (e.g., fetchInstalledPackages succeeds but the cache update fails), and then on the next call if fetchInstalledPackages fails, the cache would still contain stale data from before. Consider whether this is the desired behavior or if error states should be handled differently.
| c.Packages = newPackages | |
| c.LastUpdated = now | |
| logger.Debug("unified cache refreshed in %s (%d packages)", time.Since(start).Truncate(time.Millisecond), len(c.Packages)) | |
| // Save to disk | |
| return c.saveToDiskLocked() | |
| // Preserve old state in case saving to disk fails. | |
| oldPackages := c.Packages | |
| oldLastUpdated := c.LastUpdated | |
| c.Packages = newPackages | |
| c.LastUpdated = now | |
| // Save to disk; if it fails, roll back in-memory state to avoid partial updates. | |
| if err := c.saveToDiskLocked(); err != nil { | |
| c.Packages = oldPackages | |
| c.LastUpdated = oldLastUpdated | |
| return err | |
| } | |
| logger.Debug("unified cache refreshed in %s (%d packages)", time.Since(start).Truncate(time.Millisecond), len(c.Packages)) | |
| return nil |
| if !ensureReady() { | ||
| return | ||
| } | ||
| // Format OUTSIDE the lock (80% of the work) |
There was a problem hiding this comment.
The comment "Format OUTSIDE the lock (80% of the work)" is misleading. While formatMessage is called before acquiring locks, the formatted message still undergoes additional processing inside writeDirect where it's wrapped with color codes via p.Info(msg). The "80%" claim implies most work is done outside locks, but color formatting via the ColorPrinter happens without locks in writeDirect. The comment should clarify what specific formatting happens where, or be revised to reflect the actual behavior.
| // Format OUTSIDE the lock (80% of the work) | |
| // Perform message formatting outside the lock to minimize work while holding locks. |
📝 Description
This PR introduces major performance optimizations across the codebase:
Changes Summary
unified_cache.jsonwith TTL-based auto-refresh (1h default)-V/--verbose) functionality%vinstead of%wor direct string concatenationPerformance Improvements
🔗 Related Issue(s)
N/A - Proactive performance optimization
🔄 Type of Change
📋 Checklist
🔍 Additional Notes
Test Results
✅ Tests: 100% pass (18 packages)
✅ Linter: 0 issues
✅ Security: 0 vulnerabilities
✅ Build: Success
Backward Compatibility
All changes are backward compatible. The unified cache automatically migrates from old cache files. The middleware pattern and public APIs remain unchanged.
Benchmarks
Benchmarks added in
internal/utils/generic_helpers_bench_test.go(later removed after optimization validation) demonstrated measurable improvements in the criticalcomputeDepspath used by list and upgrade commands.