Repository navigation
perf(icons): chrome glyphs come from one shared bitmap per glyph, size and scale (REOPEN-F7 part A2) - #32481
Merged
Conversation
…-F7 part A2, red) Whole stall stacks after a reopen (feat 52a6fbb, 10 warm realistic runs, cmux18 job cea96828b32f4e6d7f183e8d): AppKit's first display pass of the restored window (37-43 ms) rasterized chrome glyphs one view at a time (-[_NSSimpleImageView updateLayer] -> NSImage CGImageForProposedRect -> NSCGImageSnapshotRep / NSImageSymbolRepProvider -> CGBitmapAllocateData). NSImage.icon returns a new drawing-handler image per call, and AppKit keeps one raster per image instance, so N views with the same glyph rasterize it N times. IconRasterSharingTests: 8 identical glyphs (a pack icon at 1x and 2x, an SF Symbol) give one bitmap per key; another size is another key; every caller still owns its image (size, description). Fails here: each image rasterizes its own bitmap.
…ide and scale (REOPEN-F7 part A2) NSImage.icon returned a new drawing-handler NSImage per call. AppKit keeps one raster per image instance, so N views showing the same glyph rasterized it N times on the main thread; after a reopen that work was part of the first window's 37-43 ms display pass (whole stall stacks, cmux18 job cea96828: NSCGImageSnapshotRep, NSImageSymbolRepProvider, CGBitmapAllocateData). IconRasters (CmuxNextIcons) draws each glyph (name, style or SF Symbol) once per side and scale (1x, 2x), from any thread, in black on clear, and keeps the bitmap. NSImage.icon returns a new template NSImage per call whose representations wrap the shared bitmaps, so a caller that sets its own image's size or description changes no other. The drawing code is the same as before (pack layers y-down, or the SF Symbol fitted and centered), so an icon at its own size draws the same pixels. Red: IconRasterSharingTests fails on the parent. Green: the same suite here.
…ory pressure (REOPEN-F7 part A2) Lead (STATUS 7066): the process-wide raster cache keeps at most 512 bitmaps and 8 MB, the least recently used dropped first, and drops every bitmap on a memory pressure warning (its own dispatch source on a utility queue). One unfair lock, held only to look up or store; drawing runs outside it; no main-thread hop. IconRastersBoundTests: the count limit drops the least recently used, the byte limit holds, a drop keeps nothing.
…art A2) Review (subagent) SHIP at dd6e954 with: - P2: three fallback glyphs were asked for smaller than the view draws them, now a bitmap stretched up: the media row's artwork (20 -> 40 pt), the import row's browser icon (16 -> 30 pt), an extension button with no icon (toolbar size -> the button's side). Each now asks for the drawn size. - The cache is fileprivate; NSImage.rasterIcon is its only door. - The pack's family overlay is part of the key, so a glyph drawn before IconPack.useOverlay is not reused after it. - The test also checks that the images' own representations are the shared bitmap. - ChiefPetRail's comment named a drawing handler the source no longer has.
…teful_singletons +1, expiry 2026-11-16) Lead decision (STATUS 7066 answer): one reviewed, dated exception for the process-wide raster cache. Follow-up issue #32444 moves the callers to an injected icon provider and removes it.
…n allSatisfy did not compile there) Fleet compile 88f96c33 at 5b9405a: IconRasterSharingTests.swift:37 'call can throw, but it is not marked with try' inside the #expect macro expansion.
lawrencecchen
temporarily deployed
to
ci-routing-agent
November 2, 2026 17:29 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
Default changesThis pull request changes 2 defaults at
Signed off on this commit by Merge receipt: merged as Regenerate the snapshot with Shipped in:
|
Contributor
Author
|
Review (subagent, correctness first): SHIP at afa4fd3 I read the whole change (82eddd7..afa4fd3) and did not build it locally. The fleet compile (6053116c), the green test run (94e76694) and the red run on the base (3c8c89ab: 8 bitmaps for 8 glyphs) are the run evidence. Each pack drawing and each SF Symbol draws once for each side and scale (1x, 2x). The output matches the old drawing-handler path in pixels: y-down pack layers, a fitted and centered symbol, black ink, and premultiplied sRGB, so template tint is unchanged. The key is complete: name, style, symbol, overlay, side and scale. The cache is thread-safe: lookup and store are under the lock, and drawing is outside it. The cache is bounded with LRU at 512 bitmaps / 8 MB. The eviction guard keeps the new entry and stops the loop. A memory warning empties the cache. The 3 fallback views that drew a glyph larger than its size now ask for the drawn size, so no glyph is stretched up from a bitmap. Since 0a428af there are 2 commits: one only moves a test key path out of |
Collaborator
|
Review (next-review patch-id; carried): SHIP at dc820c3 The independent SHIP at afa4fd3 carries to this head: the PR's own change ( |
Collaborator
|
Review (next-review patch-id; carried): SHIP at 57569f6 The independent SHIP at afa4fd3 carries to this head: the PR's own change ( |
lawrencecchen
added
the
ready-to-land
lawrencecchen
added a commit
that referenced
this pull request
Nov 3, 2026
… shows (#33112) * test(sidebar): a hidden icon button makes no image until it shows (REOPEN-F7, red) A reopened window's sidebar makes 5 icon buttons per workspace row, hidden until hover, and each made its icon image at init: about 4 ms of the profiled window build before the first frame (profile 8, SidebarIconButton.renderIcon). These tests fail at the base: a hidden button, or one in a hidden row, already has its image. * perf(sidebar): an icon button makes its image when it first shows, not at init (REOPEN-F7) Each workspace row makes 5 icon buttons (close, audio, pin, tree fold, more) that stay hidden until the pointer is on the row; each made its template image (an NSImage with a bitmap per scale) at init. Profile 8 (cmux19, job 72f71795): SidebarIconButton.renderIcon about 4 ms and the buttons' own layout 2 ms of the window build before the first frame. renderIcon now returns while the button is hidden or in a hidden row. It runs when the button shows (viewDidUnhide, also sent for an ancestor), joins a window, lays out, or is asked its intrinsic size. An icon, label or symbol change on a hidden button draws when it shows. The look does not change: a shown button has its image before it draws. The shared glyph bitmaps (#32481) stay as they are. Sidebar-Look: no visual change * test(sidebar): the lazy-image test reads the icon, not AppKit's 1 x 1 placeholder The green step 6bc3ff4c failed: a hidden button's image read as a 1 x 1 NSImage, the same object in every test. AppKit answers a button that has no image of its own with a shared placeholder, so image == nil never held. The test now counts an image as the icon only when it is at least 2 points wide. Sidebar-Look: no visual change * fix(sidebar): a button asked its size makes its icon, read from its own state (green step 1844f51d) aShownButtonHasItsImageInAWindowAndWithoutOne failed at d567840: a button outside a window, asked its intrinsic size, kept AppKit's 1 x 1 placeholder image, because the check was image == nil and a button with no image of its own answers that placeholder. A stack or a constraint reading that size would size the button 1 x 1. The check is now renderedSize == 0 (no icon made yet). Sidebar-Look: no visual change * test(sidebar): a new workspace row's hover controls start hidden, with no image (review P3, red) Review of #33112 at bc142a2 (P3): the row's Unpin button was not hidden at init, so it made its image when the row joined the window, before the row's first layout hid it. Fails at bc142a2 on the pin. Sidebar-Look: no visual change * perf(sidebar): a workspace row's Unpin button starts hidden (review P3) It shows only on hover of an expanded pinned row (layoutPin through setHoverShown, which unhides a hidden view the same way), and the row's first layout hid it anyway. Hidden at init, it makes no image before the first frame, as the close, audio and tree fold buttons. Sidebar-Look: no visual change --------- Co-authored-by: cmux-bot[bot] <348531757+cmux-bot[bot]@users.noreply.github.com>
lawrencecchen
added a commit
that referenced
this pull request
Nov 3, 2026
… first shows (stacked on #33112) (#33113) * test(sidebar): a hidden icon button makes no image until it shows (REOPEN-F7, red) A reopened window's sidebar makes 5 icon buttons per workspace row, hidden until hover, and each made its icon image at init: about 4 ms of the profiled window build before the first frame (profile 8, SidebarIconButton.renderIcon). These tests fail at the base: a hidden button, or one in a hidden row, already has its image. * perf(sidebar): an icon button makes its image when it first shows, not at init (REOPEN-F7) Each workspace row makes 5 icon buttons (close, audio, pin, tree fold, more) that stay hidden until the pointer is on the row; each made its template image (an NSImage with a bitmap per scale) at init. Profile 8 (cmux19, job 72f71795): SidebarIconButton.renderIcon about 4 ms and the buttons' own layout 2 ms of the window build before the first frame. renderIcon now returns while the button is hidden or in a hidden row. It runs when the button shows (viewDidUnhide, also sent for an ancestor), joins a window, lays out, or is asked its intrinsic size. An icon, label or symbol change on a hidden button draws when it shows. The look does not change: a shown button has its image before it draws. The shared glyph bitmaps (#32481) stay as they are. Sidebar-Look: no visual change * test(sidebar): a hidden icon button registers no tooltip until it shows (REOPEN-F7, red) Each workspace row's hover-only buttons registered their themed tooltip (a tracker in the tooltip table, a tracking area) at init, before the first window frame: about 1 ms profiled (profile 8). Fails at the base: a hidden button already has its tracker. * perf(sidebar): an icon button registers its tooltip when it first shows (REOPEN-F7) Each workspace row's hover-only icon buttons put a tracker in the tooltip table and a tracking area on themselves at init, before the first window frame (TooltipTracker.setWhole, about 1 ms profiled in profile 8), though a hidden button cannot be hovered. applyToolTip registers the label as the themed tooltip once the button shows (unhide, also of an ancestor, or joining a window). Until then it sets only the VoiceOver help, so a hidden button still has its help. A tooltip already registered follows a label change at once. Sidebar-Look: no visual change * test(sidebar): the lazy-image test reads the icon, not AppKit's 1 x 1 placeholder The green step 6bc3ff4c failed: a hidden button's image read as a 1 x 1 NSImage, the same object in every test. AppKit answers a button that has no image of its own with a shared placeholder, so image == nil never held. The test now counts an image as the icon only when it is at least 2 points wide. Sidebar-Look: no visual change * fix(sidebar): a button asked its size makes its icon, read from its own state (green step 1844f51d) aShownButtonHasItsImageInAWindowAndWithoutOne failed at d567840: a button outside a window, asked its intrinsic size, kept AppKit's 1 x 1 placeholder image, because the check was image == nil and a button with no image of its own answers that placeholder. A stack or a constraint reading that size would size the button 1 x 1. The check is now renderedSize == 0 (no icon made yet). Sidebar-Look: no visual change * test(sidebar): a shortcut set before a button shows is kept; the tooltip reads back with no window (review P1, red) Review of #33113 at e2c9ba9: showPriority sets the Priority bell's themedToolTipShortcut on a sidebar that may have no window yet. With the deferred registration, TooltipTracker.setWhole(nil, shortcut:) returned early and the shortcut was lost (SidebarIconMeaningTests. thePriorityBellNamesItsDotAndKeepsTheShortcutApart fails too). These tests fail at e2c9ba9. The earlier tests now check the tooltip tracking area, not the tracker table: the deferred part is the hover tracking, not the text. Sidebar-Look: no visual change * fix(sidebar): keep a hidden button's tooltip text and shortcut; defer only its tracking area (review P1) Review of #33113 at e2c9ba9 (P1): showPriority sets the Priority bell's themedToolTipShortcut on a sidebar that may have no window yet. The deferred registration made TooltipTracker.setWhole(nil, shortcut:) return early, so the bell lost its shortcut, and themedToolTip read nil before the button showed (SidebarIconMeaningTests.thePriorityBell...). Now the tracker always records the text and the shortcut. Only the hover tracking area waits: NSView.defersThemedToolTipTracking (opt-in; only SidebarIconButton sets it, the 127 other themedToolTip users do not change) keeps the area off while the view is hidden or has no window, and themedToolTipCanShow() adds it when the button shows (viewDidUnhide, viewDidMoveToWindow, and now viewDidMoveToSuperview, the review's P3: a move from a hidden to a shown superview in the same window). VoiceOver's help is set with the text, as before. Sidebar-Look: no visual change * test(sidebar): a new workspace row's hover controls start hidden, with no image (review P3, red) Review of #33112 at bc142a2 (P3): the row's Unpin button was not hidden at init, so it made its image when the row joined the window, before the row's first layout hid it. Fails at bc142a2 on the pin. Sidebar-Look: no visual change * perf(sidebar): a workspace row's Unpin button starts hidden (review P3) It shows only on hover of an expanded pinned row (layoutPin through setHoverShown, which unhides a hidden view the same way), and the row's first layout hid it anyway. Hidden at init, it makes no image before the first frame, as the close, audio and tree fold buttons. Sidebar-Look: no visual change
Contributor
Author
|
Launch A/B re-run on a clean mini (CMUX20 NOISE, STATUS 10157 / 10158): no measurable first-frame gain from A2. The A/B behind the "small gain" claim (STATUS 7567) ran on cmux20-bench while a second cmux NEXT app and its daemons ran there (STATUS 8394). The same refs and bench args, re-run on cmux19 (whole host, interleaved A / A1 / A1+A2 twice, 10 warm realistic launches per step, ms from
Corrections to the description:
No revert: the change is correct and keeps its later-render benefit. Data: |
This branch was previously deployed
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
REOPEN-F7 part A2. After a reopen, AppKit's first display of the restored window spends 37-43 ms on the main thread before the first frame. Whole stall stacks (10 warm realistic reopens, fleet job cea96828b32f4e6d7f183e8d on cmux18) show chrome glyphs rasterized one view at a time there:
-[_NSSimpleImageView updateLayer]->NSImage CGImageForProposedRect->NSCGImageSnapshotRep/NSImageSymbolRepProvider->CGBitmapAllocateData.Cause:
NSImage.icon(_:size:style:)/icon(symbol:size:style:)(about 105 callers) returned a new drawing-handlerNSImageper call, and AppKit keeps one raster per image instance. N views showing the same glyph rasterized it N times.Chrome's model is one shared image cache.
IconRasters(CmuxNextIcons) draws each glyph once per key (icon name, style, SF Symbol, pack family overlay, side, scale 1x/2x), from any thread, in black on clear (template).NSImage.iconreturns a new templateNSImageper call whose representations wrap the shared bitmaps, so a caller that sets its own image's size or description changes no other. The drawing code is the same as before (pack layers y-down, or the SF Symbol fitted and centered).IconRasters.sharedisfileprivate(onlyNSImage.iconreaches it) and lands under a reviewed, dated exception,plans/cmux-next/arch-exceptions/icon-rasters.json(stateful_singletons+1, expiry 2026-11-16). Follow-up Icons: move NSImage.icon callers to an injected icon provider and remove the IconRasters singleton exception #32444 moves callers to an injected icon provider and removes it.Not in this PR:
TitlebarBandButton.renderSymbolandSidebarToggleIconmake their own symbol images per instance (not throughNSImage.icon).Testing
Regression tests (committed first):
IconRasterSharingTests: 8 identical glyphs (a pack icon at 1x and 2x, an SF Symbol) give one bitmap per key, and the images' own representations are that bitmap; another size is another key; each caller owns its image (size, description).IconRastersBoundTests: the count limit drops the least recently used; the byte limit holds; a drop keeps nothing.Fleet runs (
package-test-lane.sh suite, throughproof-step.py):IconRasterSharingTestsFAILS: 8 bitmaps for 8 identical glyphs, at 1x, at 2x and for the SF Symbol.IconRasterSharingTests,IconRastersBoundTests,BrowserChromeIconsTests,BrowserToolbarExtensionsButtonTests,SidebarItemGlyphStaysTestsall passed.Review: an independent review subagent gave SHIP at dd6e954 (3 P2 fixed since) and SHIP at 0a428af; the exception file carries that receipt.
Launch A/B (A1 + A2 together, lead order): AB_LINE
Observed / Not run
Changelog
none
Proof
No capture: the glyphs draw the same pixels; the tests pin the sharing.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Chrome glyphs now come from one shared bitmap per glyph, size, and scale, so N views showing the same icon rasterize it once instead of N times. That removes the 37–43 ms AppKit stall in the first display pass after reopening a window.
NSImage.iconstill returns a new templateNSImageper call; the image's representations wrap the shared bitmaps, so callers setting size or description change no other image.IconRasterSharingTestspins the sharing;IconRastersBoundTestspins the limits and drops.Written for commit 57569f6. Summary will update on new commits.