-
Notifications
You must be signed in to change notification settings - Fork 4
β‘ Bolt: Cache color map in StatusLine to reduce allocations #83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ## 2024-05-24 - Cache static maps at struct level to avoid allocation in performance-sensitive areas | ||
| **Learning:** In performance-sensitive areas (like color mapping for the status line where multiple sections run in parallel), avoid repeated map allocations per plugin execution. Generating the color map per execution causes unnecessary GC pressure. Global variables carry risk of shared mutable state and require expensive deep copying. | ||
| **Action:** Cache static data maps at the instance level (e.g., in the `StatusLine` struct) using `sync.Once` during initialization (e.g. within an accessor method) to ensure thread-safety, prevent nil map panics, minimize allocations, and preserve backward compatibility with struct literals. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,16 @@ type StatusLine struct { | |
| isIdle bool | ||
| bashPlugins []plugin.Plugin // Cached discovered bash plugins | ||
| bashPluginsOnce sync.Once | ||
| colorsMap map[string]string | ||
| colorsOnce sync.Once | ||
| } | ||
|
|
||
| // getColors returns the color map, initializing it once if needed | ||
| func (sl *StatusLine) getColors() map[string]string { | ||
| sl.colorsOnce.Do(func() { | ||
| sl.colorsMap = colors.ColorMap() | ||
| }) | ||
| return sl.colorsMap | ||
| } | ||
|
Comment on lines
+43
to
48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sharing the same map instance across parallel plugin executions introduces a potential race condition. Since |
||
|
|
||
| // New creates a new StatusLine renderer | ||
|
|
@@ -634,7 +644,7 @@ func (sl *StatusLine) runPlugin(name string) string { | |
| ContextWindowSize: sl.input.Context.ContextWindow, | ||
| }, | ||
| Config: sl.getPluginConfig(name), | ||
| Colors: colors.ColorMap(), | ||
| Colors: sl.getColors(), | ||
| } | ||
|
|
||
| // Try native plugin first (much faster - no subprocess) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caching the color map at the
StatusLineinstance level still results in one allocation per instance. Since the color map is derived from static ANSI constants, it is a prime candidate for a package-level singleton ininternal/colors. Moving the cache there (using a package-levelsync.Once) would reduce allocations to exactly once per process lifetime, which is more efficient than the current per-instance approach and keeps theStatusLinestruct cleaner.