From 0695a4bc5174c70d19f33be68d2852a5a3f78a92 Mon Sep 17 00:00:00 2001 From: Samuel Reed Date: Tue, 18 Aug 2026 16:55:24 -0400 Subject: [PATCH 1/2] perf: memoize split-button SF Symbol resolution splitActionSystemImage(for:) decided whether a name was a real SF Symbol by allocating an NSImage and throwing it away. splitActionButtonIcon calls it once per split button during every tab bar body evaluation, so the tab bar paid a symbol-catalog lookup per button per frame. A profile of a host app with animating tab titles put 219ms of a 20-second sample inside this path, 3.3% of the process's total CPU, with 83ms of it in -[_NSSimpleLRUCache objectForKey:creationBlock:]. The answer is a pure function of the name and cannot change while the process runs, so it is now memoized. Measured at 2000 lookups: ~27us each uncached, ~0.1us each cached. Claude-Session: https://claude.ai/code/session_01Bj791kgph6Xk9CJf9c41Db --- CHANGELOG.md | 8 ++ .../Bonsplit/Internal/Views/TabBarView.swift | 63 ++++++++++++++ .../SplitActionSystemImageCacheTests.swift | 84 +++++++++++++++++++ 3 files changed, 155 insertions(+) create mode 100644 Tests/BonsplitTests/SplitActionSystemImageCacheTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 288aee74..f046f370 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 modifier-hold) with its width reserved so the tab never resizes. Pinned terminal (and other non-browser) tabs keep their titled layout. +### Fixed +- Split-button icons no longer re-resolve their SF Symbol on every tab bar body + evaluation. `TabBarStyling.splitActionSystemImage(for:)` tested whether a name + was a real symbol by allocating an `NSImage` and discarding it, which cost + ~27µs per call and ran once per button per evaluation. Results are now + memoized by name, which the symbol catalog makes safe: the answer cannot + change while the process runs. + ## [1.1.1] - 2025-01-29 ### Fixed diff --git a/Sources/Bonsplit/Internal/Views/TabBarView.swift b/Sources/Bonsplit/Internal/Views/TabBarView.swift index 709ddd7e..ddef81b3 100644 --- a/Sources/Bonsplit/Internal/Views/TabBarView.swift +++ b/Sources/Bonsplit/Internal/Views/TabBarView.swift @@ -170,7 +170,23 @@ enum TabBarStyling { ) } + /// Resolves a configured split-button icon name to the glyph the tab bar + /// should draw. + /// + /// Memoized, because `splitActionButtonIcon` calls this during every tab + /// bar body evaluation, once per split button, and resolving is far more + /// expensive than it looks. See ``resolveSplitActionSystemImage(for:)``. static func splitActionSystemImage(for name: String) -> SplitActionSystemImage { + SplitActionSystemImageCache.shared.image(for: name) + } + + /// The uncached resolution. + /// + /// The only way to ask AppKit whether a name is a real SF Symbol is to load + /// the symbol image and throw it away, which walks the symbol catalog and + /// builds a representation. The answer cannot change while the process + /// runs, so it is worth remembering. + static func resolveSplitActionSystemImage(for name: String) -> SplitActionSystemImage { if NSImage(systemSymbolName: name, accessibilityDescription: nil) != nil { return SplitActionSystemImage(name: name, rotationDegrees: 0, pointSize: 12) } @@ -1757,6 +1773,53 @@ private struct TabBarLayerBackedColor: NSViewRepresentable { } } +/// Remembers ``TabBarStyling/splitActionSystemImage(for:)`` results by name. +/// +/// Names come from host configuration, so the live set is a handful of entries +/// and never turns over. The bound exists so a pathological host cannot grow +/// this without limit. +final class SplitActionSystemImageCache { + /// The instance the tab bar uses. Tests build their own so they never race + /// each other through shared state. + static let shared = SplitActionSystemImageCache() + + private static let capacity = 256 + + private let lock = NSLock() + private var resolved: [String: TabBarStyling.SplitActionSystemImage] = [:] + private var resolutions = 0 + + init() {} + + func image(for name: String) -> TabBarStyling.SplitActionSystemImage { + lock.lock() + if let hit = resolved[name] { + lock.unlock() + return hit + } + lock.unlock() + + // Resolved outside the lock so a symbol lookup never blocks another + // caller. Two threads racing on the same name both resolve and agree. + let image = TabBarStyling.resolveSplitActionSystemImage(for: name) + + lock.lock() + resolutions += 1 + if resolved.count < Self.capacity { + resolved[name] = image + } + lock.unlock() + return image + } + + /// How many times the uncached resolution has run. Tests only. + var resolutionCount: Int { + lock.lock() + defer { lock.unlock() } + return resolutions + } +} + private final class SplitActionButtonImageCache { static let shared = SplitActionButtonImageCache() diff --git a/Tests/BonsplitTests/SplitActionSystemImageCacheTests.swift b/Tests/BonsplitTests/SplitActionSystemImageCacheTests.swift new file mode 100644 index 00000000..e38379cb --- /dev/null +++ b/Tests/BonsplitTests/SplitActionSystemImageCacheTests.swift @@ -0,0 +1,84 @@ +import AppKit +@testable import Bonsplit +import Testing + +/// `splitActionButtonIcon` resolves every split button's icon during every tab +/// bar body evaluation, and resolving asks AppKit whether a name is a real SF +/// Symbol by loading the symbol image and discarding it. Under an animating tab +/// title the tab bar re-evaluates ~20 times a second, so the resolution has to +/// be memoized rather than repeated. +@Suite("Split action symbol resolution") +struct SplitActionSystemImageCacheTests { + private let names = ["plus", "square.split.2x1", "ellipsis.vertical", "not.a.real.symbol.name"] + + @Test func repeatedLookupsResolveEachNameOnce() { + let cache = SplitActionSystemImageCache() + + for _ in 0..<500 { + for name in names { + _ = cache.image(for: name) + } + } + + #expect(cache.resolutionCount == names.count) + } + + @Test func memoizingDoesNotChangeWhatIsResolved() { + let cache = SplitActionSystemImageCache() + + // A real symbol keeps its own name and is drawn upright. + let real = cache.image(for: "plus") + #expect(real == TabBarStyling.resolveSplitActionSystemImage(for: "plus")) + #expect(real.name == "plus") + #expect(real.rotationDegrees == 0) + + // `ellipsis.vertical` is not a symbol; it is a rotated `ellipsis`. + let rotated = cache.image(for: "ellipsis.vertical") + #expect(rotated == TabBarStyling.resolveSplitActionSystemImage(for: "ellipsis.vertical")) + #expect(rotated.name == "ellipsis") + #expect(rotated.rotationDegrees == 90) + + // Anything else falls back rather than drawing nothing. + let unknown = cache.image(for: "not.a.real.symbol.name") + #expect(unknown == TabBarStyling.resolveSplitActionSystemImage(for: "not.a.real.symbol.name")) + #expect(unknown.name == "questionmark.circle") + } + + @Test func theTabBarGoesThroughTheCache() { + // Two calls with no cache in between would resolve twice; the point of + // the accessor is that the second one costs a dictionary lookup. + let before = SplitActionSystemImageCache.shared.resolutionCount + for _ in 0..<200 { + _ = TabBarStyling.splitActionSystemImage(for: "square.split.2x1") + } + let resolutions = SplitActionSystemImageCache.shared.resolutionCount - before + + #expect(resolutions <= 1) + } + + /// A repeat lookup must not reach AppKit at all. Timed rather than counted + /// so the assertion still means something if the cache is later replaced by + /// a different mechanism. The bound is loose on purpose: the measured gap is + /// two to three orders of magnitude. + @Test func cachedLookupsAreOrdersOfMagnitudeCheaper() { + let iterations = 2_000 + let name = "square.split.2x1" + let cache = SplitActionSystemImageCache() + + let uncachedStart = ContinuousClock.now + for _ in 0.. Date: Tue, 18 Aug 2026 17:05:38 -0400 Subject: [PATCH 2/2] test: make the cache-bypass assertion actually detect a bypass resolutions <= 1 passed at zero, which is what an accessor that skipped the cache and called the resolver directly would produce. Now it asserts exactly one resolution for a name no other test touches, so 0 catches a bypass and 200 catches no caching at all. Verified by pointing splitActionSystemImage(for:) straight at the resolver: the test fails with resolutions 0. Addresses CodeRabbit review on #221. Claude-Session: https://claude.ai/code/session_01Bj791kgph6Xk9CJf9c41Db --- .../SplitActionSystemImageCacheTests.swift | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Tests/BonsplitTests/SplitActionSystemImageCacheTests.swift b/Tests/BonsplitTests/SplitActionSystemImageCacheTests.swift index e38379cb..580a40fe 100644 --- a/Tests/BonsplitTests/SplitActionSystemImageCacheTests.swift +++ b/Tests/BonsplitTests/SplitActionSystemImageCacheTests.swift @@ -45,15 +45,18 @@ struct SplitActionSystemImageCacheTests { } @Test func theTabBarGoesThroughTheCache() { - // Two calls with no cache in between would resolve twice; the point of - // the accessor is that the second one costs a dictionary lookup. + // A name no other test in this file touches, so the shared cache has + // never seen it and the count below is exact. 200 says the accessor + // resolves every time; 0 says it went around the cache entirely. + let name = "rectangle.split.3x1" + let before = SplitActionSystemImageCache.shared.resolutionCount for _ in 0..<200 { - _ = TabBarStyling.splitActionSystemImage(for: "square.split.2x1") + _ = TabBarStyling.splitActionSystemImage(for: name) } let resolutions = SplitActionSystemImageCache.shared.resolutionCount - before - #expect(resolutions <= 1) + #expect(resolutions == 1) } /// A repeat lookup must not reach AppKit at all. Timed rather than counted