perf(menu bar): cache rasterized custom layouts - #3110
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a46d6d730
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rendered.attributedTitle.draw( | ||
| with: titleRect, | ||
| options: [.usesLineFragmentOrigin, .usesFontLeading]) | ||
| image.isTemplate = false |
There was a problem hiding this comment.
When inactive-display contrast is disabled and a custom layout contains a normal template icon, forcing the combined bitmap to be non-template prevents AppKit from dimming it on inactive displays. MenuBarLayoutRenderedTitle explicitly surfaces the leading icon separately because only template status images receive this tinting; after this change the entire custom layout remains at full contrast on every monitor, making the inactive-display contrast preference ineffective for these layouts. Preserve the native/template path when inactive tinting is requested, or otherwise account for each replicant's active state.
Useful? React with 👍 / 👎.
| self.rasterizedMenuBarLayoutCache.signatures[buttonID] = signature | ||
| self.rasterizedMenuBarLayoutCache.images[buttonID] = image |
There was a problem hiding this comment.
When a provider is disabled or icons are switched into merged mode, removeProviderStatusItem destroys its status item but leaves these ObjectIdentifier entries behind; switching back creates a new button and inserts another image. Repeated configuration changes therefore grow both dictionaries for the lifetime of the controller, and the existing memory-pressure trim also does not clear this new image cache. Remove the button's entries during status-item teardown and include this cache in rebuildable-cache trimming.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs changes before merge. Reviewed August 20, 2026, 12:27 PM ET / 16:27 UTC. ClawSweeper reviewWhat this changesThe PR caches each custom menu-bar layout as a raster image per status-bar button to avoid repeated AppKit title layout and redraw work. Merge readinessKeep this PR open for a focused repair: the raster path changes the established inactive-display contrast behavior and retains images for destroyed status items. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherCodexBar renders provider usage and menu-bar settings into macOS status-bar buttons. The proposed path replaces AppKit’s attributed-title rendering with a cached bitmap image and explicit status-item width. flowchart LR
A[Provider usage and settings] --> B[Menu-bar layout renderer]
B --> C[Rendered title and icon]
C --> D{Test or production}
D -->|Tests| E[Native attributed title]
D -->|Production| F[Cached raster image]
F --> G[Status-bar button on displays]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep raster caching only where it preserves display-state semantics, evict entries during item teardown and memory-pressure relief, and add direct coverage for the bitmap/cache path. Do we have a high-confidence way to reproduce the issue? Not applicable as a bug report; this is a performance optimization, and the body provides detailed after-fix live measurements for the submitted path. Is this the best way to solve the issue? No: raster caching is plausible, but the current implementation must retain the existing per-display contrast semantics and bounded cache ownership. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8709988272b8. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
Thanks — the measurements are compelling and
Happy to land this once it draws via |
2 participants
Summary
This draft pre-renders a custom menu-bar layout into a cached
NSImageand leaves theNSStatusBarButtonattributed title empty. The rendered text, icon, accessibilitylabel, item width, provider overrides, spacing, and layout settings are preserved.
The cache is scoped per status-bar button and invalidates when rendered content, the
leading icon, or the configured gap changes. Recreating the status items clears it.
Why
On a three-display M4 MacBook Pro, AppKit repeatedly replicated and snapshotted a
custom attributed status-item title across displays and Spaces. Samples were dominated
by
NSStatusItem._updateReplicants,_redrawReplicantSnapshot, and Core Animationtransaction commits even while the displayed value had not changed.
Environment: M4 MacBook Pro, macOS 27.0 (26A5406e), built-in display plus 144 Hz and
180 Hz external displays. Layout used for both measurements: percentage + pace +
compact runs-out (
82% +11% 2d 2h).sampletraceDuring the original system-wide incident, the correlated processes reached roughly
78% CPU for
MenuBarAgentand 108% forWindowServer; those figures include the widermenu-bar/display workload and are not claimed as CodexBar-only CPU.
Verification
swift test --filter 'MenuBarLayoutRendererTests|StatusItemLayoutPaceSignatureTests|StatusItemIconObservationSignatureTests'mainprovider/account configuration
82% +11% 2d 2hReview note
This is deliberately a draft. Existing tests retain the attributed-title path so their
renderer characterization remains unchanged; the production bitmap path would benefit
from a dedicated injectable policy or direct bitmap/cache tests before merge. I would
appreciate maintainer guidance on the preferred shape for that coverage.