Repository navigation
Sidebar perf: kill the LazyVStack layout livelock (combined: 6019 + 6021 + 6026) #6033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
46e0cbc
f55f583
0aced72
c5cb9b6
fc42869
c8e71c1
27a8f09
83322e5
9e6cccd
b070c22
75d6004
8f992c8
250fb7b
f13128a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| import Foundation | ||
|
|
||
| /// Renders sidebar metadata-block markdown with a bounded memo cache so the | ||
| /// FIRST render of a row is already attributed. | ||
| /// | ||
| /// The previous flow parsed in the row's `onAppear` into `@State`: every first | ||
| /// appearance of every metadata block performed a guaranteed nil -> attributed | ||
| /// swap, changing the row's intrinsic height mid-scroll and re-feeding the | ||
| /// sidebar-wide layout/measurement cycle | ||
| /// (https://github.com/manaflow-ai/cmux/issues/5764, | ||
| /// https://github.com/manaflow-ai/cmux/issues/5845). Lazy rows must be | ||
| /// height-stable after they appear, and row state belongs in the initializer or | ||
| /// the model, not `onAppear`. | ||
| /// | ||
| /// Parsing inline from `body` matches the `SidebarWorkspaceDescriptionText` | ||
| /// sibling; the cache keeps repeat body evaluations cheap (agent-heavy rows | ||
| /// re-evaluate often) and is bounded so long sessions with churning metadata | ||
| /// cannot grow it without limit. | ||
| @MainActor | ||
| enum SidebarMetadataMarkdownRenderer { | ||
| private static var cache: [String: AttributedString?] = [:] | ||
| private static var insertionOrder: [String] = [] | ||
| private static let capacity = 512 | ||
| /// Only small blocks are rendered as markdown. Metadata markdown is | ||
| /// agent/control-socket supplied and uncapped at this boundary. Caching | ||
| /// large values would retain hundreds of big payloads after the workspace | ||
| /// metadata is overwritten or cleared (memory), and parsing them inline on | ||
| /// every body eval would re-run main-actor Markdown parsing under agent | ||
| /// churn (CPU). Above this size the block falls back to plain text (the row | ||
| /// renders `Text(block.markdown)` on the nil return): no parse, no | ||
| /// retention, and still height-stable because the result never changes for | ||
| /// a given block. Cached small blocks bound total retained bytes to | ||
| /// `capacity * maxCacheableBytes`. A >4 KB sidebar metadata block is already | ||
| /// pathological, so plain text is an acceptable degradation. | ||
| private static let maxCacheableBytes = 4096 | ||
|
|
||
| static func rendered(_ markdown: String) -> AttributedString? { | ||
| guard markdown.utf8.count <= maxCacheableBytes else { | ||
| return nil | ||
| } | ||
| if let hit = cache[markdown] { | ||
| return hit | ||
| } | ||
| let parsed = parse(markdown) | ||
| if insertionOrder.count >= capacity, let oldest = insertionOrder.first { | ||
| insertionOrder.removeFirst() | ||
| cache.removeValue(forKey: oldest) | ||
| } | ||
| // updateValue, not subscript assignment: with an Optional value type, | ||
| // `cache[markdown] = nil` removes the key instead of caching the failed | ||
| // parse, so unparseable blocks would re-parse on every body eval and | ||
| // append phantom keys to insertionOrder. | ||
| cache.updateValue(parsed, forKey: markdown) | ||
| insertionOrder.append(markdown) | ||
| return parsed | ||
| } | ||
|
|
||
| private static func parse(_ markdown: String) -> AttributedString? { | ||
| try? AttributedString( | ||
| markdown: markdown, | ||
| options: .init(interpretedSyntax: .full) | ||
| ) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,69 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import SwiftUI | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Lays the sidebar workspace rows out at their natural height, then stretches a | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// trailing empty drop/tap area to fill the remaining viewport — in one geometry | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// pass, with no whole-content height measurement. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// The previous approach measured the `LazyVStack`'s total height via a | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// `.background` `GeometryReader` and routed it through a `PreferenceKey` into | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// `@State` to size a fixed-height empty area. That preference write during | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// layout fed a non-converging relayout transaction | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// (https://github.com/manaflow-ai/cmux/issues/2586, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// https://github.com/manaflow-ai/cmux/issues/5764, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// https://github.com/manaflow-ai/cmux/issues/5845). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// This `Layout` takes the viewport height as an explicit input | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// (`viewportHeight`, the floored content height the call site already computes | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// from the scroll geometry) and sizes the empty area from it directly. It does | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// NOT derive the viewport from the layout proposal: a vertical `ScrollView` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// leaves the scroll-axis height unspecified, and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// `ProposedViewSize.replacingUnspecifiedDimensions()` would then fall back to a | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// 10pt placeholder, collapsing the empty area to `0` and dropping the blank | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// area below the last row out of the drop/tap target. With the explicit | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// viewport: when the rows fit, rows + empty area exactly fill the viewport (no | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// overflow, overlay scroller stays hidden — | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// https://github.com/manaflow-ai/cmux/issues/3241); when the rows overflow, the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// empty area is `0` and the document view scrolls. The rows are never measured | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// into SwiftUI state. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Expects exactly two subviews in order: `[rows, emptyArea]`. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| struct SidebarRowsFillLayout: Layout { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// The floored viewport height available to the scroll content. The empty | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// area fills the remainder of this height below the rows. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let viewportHeight: CGFloat | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func sizeThatFits(proposal: ProposedViewSize, subviews: Subviews, cache: inout ()) -> CGSize { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let width = proposal.replacingUnspecifiedDimensions().width | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let rowsHeight = subviews.first?.sizeThatFits( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ProposedViewSize(width: width, height: nil) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ).height ?? 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Fill the viewport when the rows are shorter; grow to the rows' natural | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // height when they overflow it. Driven by the explicit viewport, not the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // (unspecified in a vertical ScrollView) proposed height. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return CGSize(width: width, height: max(rowsHeight, viewportHeight)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func placeSubviews(in bounds: CGRect, proposal: ProposedViewSize, subviews: Subviews, cache: inout ()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| guard let rows = subviews.first else { return } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let rowsHeight = rows.sizeThatFits( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ProposedViewSize(width: bounds.width, height: nil) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ).height | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+30
to
+50
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| rows.place( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| at: CGPoint(x: bounds.minX, y: bounds.minY), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| proposal: ProposedViewSize(width: bounds.width, height: rowsHeight) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| guard subviews.count > 1 else { return } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Size the empty area against the explicit viewport (or the rows' height | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // when they overflow it), never against `bounds.height` — which could be | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // the rows' natural height alone if the parent placed us at our content | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // size. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let emptyHeight = SidebarWorkspaceScrollLayout.emptyAreaFillHeight( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| viewportHeight: viewportHeight, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| rowsHeight: rowsHeight | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| subviews[1].place( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| at: CGPoint(x: bounds.minX, y: bounds.minY + rowsHeight), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| proposal: ProposedViewSize(width: bounds.width, height: emptyHeight) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
insertionOrder.removeFirst()is O(n) on every eviction becauseArrayshifts all remaining elements. In a high-churn session where metadata blocks are constantly updated, each eviction walks all 512 entries. A ring-buffer eviction index avoids the per-eviction copy entirely while keeping the same[String: AttributedString?]cache dictionary.