⚡ perf: fix what the second audit turned up

Three of these are mine from the last round. Format.calendar snapshotted
Calendar.current for the life of the process, so an app left running
across a time zone change would keep deciding "same day" against the old
one; it holds the autoupdating calendar instead. LogBuffer encoded and
wrote inside its lock on the caller's thread, usually the main one, so a
logger on another thread waited behind a disk write. And the history
redraw cache had no expiry, which let a popover left open for an hour
flat-line the last sample across time it never fetched, and it cleared
isRefreshing while a reload was still running.

Hovering the history chart was the worst path in the app: every mouse-move
point wrote selectedDate whether or not the nearest point had changed, and
Observation publishes on assignment, so each one rebuilt every mark in the
chart.

Loading the popover's samples ran one query per window and ran whether or
not the popover was open. One query covers every window, and the cards are
its only reader.

The status item built the full attributed title for every rung of the
adaptive ladder to measure its width, then built the chosen one again to
draw it; both now share one build. Each cell image measured its lines
three times, once inside a drawing handler AppKit re-invokes on every
scale change.

AppController's observation closure captured self strongly and
re-registered itself after every change, so the registrar held the
controller, its state and its history store for good.

Claude-Session: https://claude.ai/code/session_01CGckDFXvufWVmRH4uyE4z8

Author: gaborbernat

Sources/TokenMenuBarCore/Formatting.swift

@@ -2,8 +2,9 @@ import Foundation
 
 public enum Format {
   // `Calendar.current` rebuilds the calendar on every read, and every quota window on screen asks for a reset time
-  // once a second while the popover is open.
-  public static let calendar = Calendar.current
+  // once a second while the popover is open. The autoupdating one is as cheap to hold but follows the user across a
+  // time zone change, which an app that runs for weeks will see.
+  public static let calendar = Calendar.autoupdatingCurrent
 
   public static func percent(_ value: Double, decimals: Int = 0) -> String {
     min(max(value, 0), 100).formatted(.number.precision(.fractionLength(decimals))) + "%"

Sources/TokenMenuBarCore/Logging/LogBuffer.swift

@@ -58,7 +58,6 @@ public final class LogBuffer: @unchecked Sendable {
   private var entries: [LogEntry] = []
   private var lastPrune: Date?
   private var lastPersist: Date?
-  private var dirty = false
   private let fileURL: URL?
   private let clock: Clock
   private let osLog = Logger(subsystem: "dev.tox.token-menu-bar", category: "app")
@@ -98,7 +97,10 @@ public final class LogBuffer: @unchecked Sendable {
     case .error: osLog.error("\(message, privacy: .public)")
     default: osLog.info("\(message, privacy: .public)")
     }
-    lock.withLock {
+    // Writing re-encodes the whole buffer and writes it atomically. With detailed logging on that is several lines
+    // per HTTP request, so the writes are coalesced, and the encode and write stay off the lock: another thread's
+    // logger would otherwise wait behind this one's disk write, and the caller here is usually the main thread.
+    let due: [LogEntry]? = lock.withLock {
       entries.append(LogEntry(timestamp: clock.now(), level: level, message: message))
       if entries.count > Self.capacity { entries.removeFirst(entries.count - Self.capacity) }
       let now = clock.now()
@@ -107,21 +109,23 @@ public final class LogBuffer: @unchecked Sendable {
         entries.removeAll { $0.timestamp < cutoff }
         lastPrune = now
       }
-      dirty = true
-      // Persisting re-encodes the whole buffer and writes it atomically. With detailed logging on that is several
-      // hundred entries per HTTP request, so the writes are coalesced instead.
-      if lastPersist.map({ now.timeIntervalSince($0) >= Self.flushInterval }) ?? true { persist(now: now) }
+      guard lastPersist.map({ now.timeIntervalSince($0) >= Self.flushInterval }) ?? true else { return nil }
+      lastPersist = now
+      return entries
     }
+    if let due { write(due) }
   }
 
   /// Writes anything still held in memory. Call before the app goes away or before reading the file back.
   public func flush() {
-    lock.withLock { persist(now: clock.now()) }
+    let entries: [LogEntry] = lock.withLock {
+      lastPersist = clock.now()
+      return self.entries
+    }
+    write(entries)
   }
 
-  private func persist(now: Date) {
-    lastPersist = now
-    dirty = false
+  private func write(_ entries: [LogEntry]) {
     guard let fileURL, let data = try? JSONEncoder().encode(entries) else { return }
     try? FileManager.default.createDirectory(at: fileURL.deletingLastPathComponent(), withIntermediateDirectories: true)
     try? data.write(to: fileURL, options: .atomic)
@@ -136,10 +140,8 @@ public final class LogBuffer: @unchecked Sendable {
   }
 
   public func clear() {
-    lock.withLock {
-      entries.removeAll()
-      persist(now: clock.now())
-    }
+    lock.withLock { entries.removeAll() }
+    flush()
   }
 
   public var text: String {

Sources/TokenMenuBarCore/Presenters/HistoryPresenter.swift

@@ -54,9 +54,13 @@ public final class HistoryPresenter {
   private let settings: Settings
   private let clock: Clock
   private var loadTask: Task<Void, Never>?
+  private var isLoading = false
+  /// How long the rows from the last fetch may be redrawn before they are refetched. A sample lands every five
+  /// minutes, so beyond this the chart would flat-line the last one across time it never fetched.
+  static let cacheLifetime: TimeInterval = 300
   /// The rows the last fetch returned, kept so hiding a series or stacking the chart redraws without going back to
   /// the database for the same rows.
-  private var fetched: (samples: [UsageSample], labels: [WindowKey: String])?
+  private var fetched: (samples: [UsageSample], labels: [WindowKey: String], at: Date)?
 
   public init(history: UsageHistoryStore, settings: Settings, clock: Clock = .system) {
     self.history = history
@@ -95,8 +99,10 @@ public final class HistoryPresenter {
     loadTask?.cancel()
     let now = clock.now()
     if case .loaded(let data, _, _) = state { state = .loaded(data, isRefreshing: true, error: nil) }
+    isLoading = true
     loadTask = Task { [weak self] in
       guard let self else { return }
+      defer { if !Task.isCancelled { isLoading = false } }
       do {
         let summaries = try await history.summaries()
         let earliest = try await history.earliestSample()
@@ -110,7 +116,7 @@ public final class HistoryPresenter {
         let labels = Dictionary(
           uniqueKeysWithValues: summaries.map { ($0.key, "\($0.key.provider.displayName) \($0.label)") })
         guard !Task.isCancelled else { return }
-        fetched = (samples, labels)
+        fetched = (samples, labels, now)
         let data = ChartPipeline.render(samples: samples, request: request, labels: labels, now: now)
         var sections: [ProviderID: [AnalyticsSection]] = [:]
         for provider in ProviderID.allCases {
@@ -131,11 +137,11 @@ public final class HistoryPresenter {
   /// Re-renders the chart from the rows already in hand. Hiding a series and stacking change what is drawn, not
   /// what was recorded, so neither needs the queries `reload` runs.
   public func redraw() {
-    guard let fetched else {
+    let now = clock.now()
+    guard let fetched, now.timeIntervalSince(fetched.at) < Self.cacheLifetime, !isLoading else {
       reload()
       return
     }
-    let now = clock.now()
     let data = ChartPipeline.render(
       samples: fetched.samples, request: request(now: now), labels: fetched.labels, now: now)
     state = .loaded(data, isRefreshing: false, error: nil)
@@ -182,11 +188,14 @@ public final class HistoryPresenter {
   }
 
   public func select(x date: Date?) {
+    // Hover reports every mouse-move point, and most of them land on the point already selected. Observation
+    // publishes on assignment, so writing the same date rebuilt every mark in the chart.
     guard let date, let data = state.data else {
-      selectedDate = nil
+      if selectedDate != nil { selectedDate = nil }
       return
     }
-    selectedDate = ChartPipeline.nearestDate(in: data, to: date)
+    let nearest = ChartPipeline.nearestDate(in: data, to: date)
+    if nearest != selectedDate { selectedDate = nearest }
   }
 
   public func value(for series: HistorySeries) -> String {

Sources/TokenMenuBarUI/AppController.swift

@@ -101,6 +101,7 @@ public final class AppController {
   public let environment: UIEnvironment
   public let coordinator: RefreshCoordinator
   public private(set) var statusItem: StatusItemController?
+  private var stopped = false
   public private(set) var popover: PopoverController?
   private var logWindow: LogWindowController?
   private var workspaceObservers: [Any] = []
@@ -209,11 +210,13 @@ public final class AppController {
   }
 
   func observeStatusModel() {
-    withObservationTracking {
-      _ = dependencies.state.statusLadder
+    // The tracking closure re-registers itself after every change, so capturing self strongly here would leave the
+    // registrar holding the controller, its state and its history store for good.
+    withObservationTracking { [weak self] in
+      _ = self?.dependencies.state.statusLadder
     } onChange: {
       Task { @MainActor [weak self] in
-        guard let self else { return }
+        guard let self, !stopped else { return }
         statusItem?.update(ladder: dependencies.state.statusLadder)
         observeStatusModel()
       }
@@ -227,6 +230,7 @@ public final class AppController {
     statusItem = nil
     for observer in workspaceObservers { NSWorkspace.shared.notificationCenter.removeObserver(observer) }
     workspaceObservers.removeAll()
+    stopped = true
     dependencies.log.log("stopped")
     dependencies.log.flush()
   }

Sources/TokenMenuBarUI/StatusItem/StatusItemController.swift

@@ -134,8 +134,10 @@ public final class StatusItemController {
   }
 
   public func update(ladder: [StatusItemModel]) {
+    let height = barHeight
+    let dark = isDark
     let widths = ladder.map {
-      Double(StatusItemRenderer.attributedTitle(for: $0, height: barHeight, dark: isDark).size().width)
+      Double(StatusItemRenderer.attributedTitle(for: $0, height: height, dark: dark).size().width)
     }
     self.ladder = AdaptiveWidthPlanner.ladder(ladder, widths: widths)
     restart()

Sources/TokenMenuBarUI/StatusItem/StatusItemRenderer.swift

@@ -39,7 +39,19 @@ public enum StatusItemRenderer {
     }
   }
 
+  /// The last title built, so measuring the adaptive ladder and then rendering the entry it picked share one build
+  /// instead of doing it twice for the same model.
+  private static var lastTitle: (signature: StatusRenderSignature, title: NSAttributedString)?
+
   public static func attributedTitle(for model: StatusItemModel, height: CGFloat, dark: Bool) -> NSAttributedString {
+    let signature = StatusRenderSignature(model: model, dark: dark, height: Double(height))
+    if let lastTitle, lastTitle.signature == signature { return lastTitle.title }
+    let title = build(model, height: height, dark: dark)
+    lastTitle = (signature, title)
+    return title
+  }
+
+  private static func build(_ model: StatusItemModel, height: CGFloat, dark: Bool) -> NSAttributedString {
     let title = NSMutableAttributedString()
     for (index, cell) in model.cells.enumerated() {
       if index > 0 { title.append(attachment(separatorImage(height: height))) }
@@ -71,14 +83,14 @@ public enum StatusItemRenderer {
 
   static func textImage(_ cell: StatusCell, height: CGFloat, dark: Bool) -> NSImage {
     let lines = lineStrings(cell, height: height, dark: dark)
-    let widest = lines.map { $0.size().width }.max() ?? 0
-    let width = ceil(widest) + cellPadding * 2
-    let heights = lines.map { $0.size().height }
-    let total = heights.reduce(0, +)
+    // Laying out an attributed string is the expensive part here, and AppKit calls the drawing handler again on
+    // every scale and appearance change, so measure once and carry the sizes in.
+    let sizes = lines.map { $0.size() }
+    let width = ceil(sizes.map(\.width).max() ?? 0) + cellPadding * 2
+    let total = sizes.map(\.height).reduce(0, +)
     return NSImage(size: CGSize(width: width, height: height), flipped: true) { rect in
       var lineTop = (rect.height - total) / 2
-      for line in lines {
-        let size = line.size()
+      for (line, size) in zip(lines, sizes) {
         line.draw(at: CGPoint(x: (rect.width - size.width) / 2, y: lineTop))
         lineTop += size.height
       }

Sources/TokenMenuBarUI/UIActions.swift

@@ -111,14 +111,16 @@ public final class UIEnvironment {
   }
 
   public func loadRecentSamples() async {
+    // Only the popover's cards read these, and one query covers every window: a loop was an actor hop and a
+    // prepared statement each.
+    guard state.popoverVisible else { return }
     let since = now.addingTimeInterval(-PaceEstimate.slopeWindow)
-    var loaded: [WindowKey: [UsageSample]] = [:]
-    for (provider, item) in state.providers {
-      for window in item.snapshot?.windows ?? [] {
-        let key = WindowKey(provider, window)
-        loaded[key] = (try? await history.recentSamples(key: key, since: since)) ?? []
-      }
+    let keys = state.providers.flatMap { provider, item in
+      (item.snapshot?.windows ?? []).map { WindowKey(provider, $0) }
     }
+    let rows = (try? await history.samples(keys: keys, from: since, to: .distantFuture)) ?? []
+    var loaded = Dictionary(grouping: rows, by: \.key)
+    for key in keys where loaded[key] == nil { loaded[key] = [] }
     samples = loaded
   }
 

Tests/TokenMenuBarUITests/ViewTests.swift

@@ -23,6 +23,10 @@ import Testing
   #expect(environment.settings.lastTab == .history)
   environment.tick()
   #expect(environment.now == fixedNow)
+  // The cards are the only reader, so nothing is fetched while the popover is closed.
+  await environment.loadRecentSamples()
+  #expect(environment.samples.isEmpty)
+  environment.state.popoverVisible = true
   await environment.loadRecentSamples()
   #expect(environment.samples.count == 6)
   #expect(environment.cards.count == 2)