perf(octicons): cache embedded data URIs on demand#2851
Open
dexhunter wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes the pkg/octicons package by adding an on-demand, race-safe cache for successfully read embedded Octicon PNG data URIs, reducing repeated base64 encoding work during repeated tool/server registrations while preserving existing behavior for misses and returning fresh icon slices.
Changes:
- Introduced an RWMutex-protected
(name, theme) -> data URIcache for successful embedded icon reads, leaving missing lookups uncached. - Updated
DataURIto use the cache while keeping the underlying embedded read logic in a dedicated helper. - Added tests for all embedded icons, cache semantics (misses not cached), mutation isolation, and concurrent first-use; added cold/warm benchmarks.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/octicons/octicons.go | Adds a race-safe on-demand cache and routes DataURI through it. |
| pkg/octicons/octicons_test.go | Adds test coverage for cache behavior, embedded inventory coverage, concurrency, and slice mutation isolation. |
| pkg/octicons/octicons_benchmark_test.go | Adds benchmarks for cold/warm cache behavior and icon registration allocation profiles. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cache successful embedded Octicon data-URI reads on first use so repeated stateless server and tool registration no longer rereads and base64-encodes the same PNGs. Missing lookups remain uncached, and every
Iconscall still returns a fresh mutable slice.Why
Fixes #2850
Repeated all-tool registration currently allocates a median
193,085 B/opand527 allocs/opfor the 31 required icons after warmup. The same process may rebuild this inventory for multiple stateless HTTP requests.What changed
(name, theme)DataURIreads.required_icons.txt.Performance
For the default inventory, warm allocation bytes fall by
97.69%and allocation count falls by94.12%. The disclosed cold cost is+6,616 Band+4 allocationswhile the cache is populated. No icon is read or encoded ininit().Supplementary autoresearch: 20-step public dashboard.
MCP impact
Prompts tested (tool changes only)
Security / limits
Only embedded PNG data is cached. Missing lookups are not retained. With all current variants used, retained data-URI payload is
46,840bytes plus bounded map overhead.Tool renaming
Lint & tests
./script/lint./script/testgo test -race ./...git diff --checkDocs