Repository navigation
Sidebar: allocation-free Hashable render-item id (hottest app frame in 5764 spindump) - #6021
Closed
azooz2003-bit wants to merge 2 commits into
Closed
azooz2003-bit wants to merge 2 commits into
azooz2003-bit wants to merge 2 commits into
Conversation
…le enum
ForEach(renderItems, id: \.id) gathers every row's identifier on each list
diff, and the sidebar re-diffs all rows per update. The previous computed
String id ("workspace.\(uuid.uuidString)") allocated and formatted a fresh
36-char string per access; SidebarWorkspaceRenderItem.id.getter was the
hottest app-owned frame in the
#5764 livelock spindump.
SidebarWorkspaceRenderItemID is a two-case enum over UUID: identity compare
and hash with zero heap allocation, and group headers can never collide with
workspace rows on the same UUID (same guarantee the string prefixes gave).
Identity values are unchanged in meaning, so row lifetime and animations are
unaffected; nothing persisted the string form (the only consumers are the
ForEach key path and scrollTo, which targets the explicit inner .id(tab.id)
UUIDs, not the ForEach identity).
Pure per-pass cost cut for #5764,
#5845,
#2586; complements the structural
loop fix in #6019.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Collaborator
Author
|
@codex review |
📝 WalkthroughWalkthroughThe ChangesSidebar Identity Type Safety
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 21✅ Passed checks (21 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
To use Codex here, create a Codex account and connect to github. |
Contributor
Greptile SummaryReplaces the computed
Confidence Score: 5/5Safe to merge. The change is a pure type substitution in a single file with no behavioral side effects. The new No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[SwiftUI list diff triggered] --> B["ForEach gathers row IDs via id: \.id"]
B --> C{SidebarWorkspaceRenderItem case}
C -->|groupHeader| D["group(group.id)"]
C -->|workspace| E["workspace(workspace.id)"]
D --> F[SidebarWorkspaceRenderItemID - stack-allocated enum]
E --> F
F --> G[Synthesized Hashable delegates to UUID.hashValue]
G --> H[Diff result - zero heap allocation per row]
Reviews (2): Last reviewed commit: "Merge origin/main (pick up workflow-guar..." | Re-trigger Greptile |
Collaborator
Author
|
Superseded by the combined PR #6033 (this PR's change is included verbatim there), opened to run a single CI/review/dogfood pass across all three sidebar-perf fixes. Closing; track 6033. |
This branch was successfully 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
1 participant
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.
Problem
ForEach(renderItems, id: \.id)gathers every row's identifier on each sidebar list diff, and the sidebar currently re-diffs all rows per update.SidebarWorkspaceRenderItem.idwas a computedString("workspace.\(uuid.uuidString)") that allocates and formats a fresh string on every getter call. In the #5764 livelock spindump,SidebarWorkspaceRenderItem.id.getteris the hottest app-owned frame (plusinitializeWithCopy/destroy for SidebarWorkspaceRenderItem), so this cost multiplies every pass of the layout loop and every normal scroll/diff.Fix
SidebarWorkspaceRenderItemID, a two-caseHashableenum overUUID(.group(UUID)/.workspace(UUID)): identity compare and hash with zero heap allocation. The case keeps group headers and workspace rows from colliding on the same UUID, the same guarantee the string prefixes gave. Identity values are unchanged in meaning, so row lifetime, state, and animations are unaffected.Safety audit: the only consumers of
.idare theForEachkey path (Sources/ContentView.swift:12535) and nothing else;scrollTotargets the explicit inner.id(tab.id)UUIDs registered on row views, not the ForEach identity, and no code persists the string form. WWDC rule backing: identifiers must be cheap to create because they are gathered often (WWDC23 Demystify SwiftUI performance, https://developer.apple.com/videos/play/wwdc2023/10160/).This is the per-pass cost companion to the structural loop fix in #6019; the two are independent and merge in either order.
Related: #5764, #5845, #2586.
Note on tests: existing
WorkspaceGroupTestscover renderItems semantics and pass unchanged; an id-shape unit test would be a source-shape assertion with no behavioral value, so none is added. Preflighted on tagrid: 5 workspaces + a workspace group (exercises both id cases), simulated sidebar drags, rows/group header/selection all render and respond.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Replaces the sidebar render-item String id with an allocation-free Hashable enum to cut per-pass diff cost and scrolling overhead. Addresses #5764; no behavior changes.
SidebarWorkspaceRenderItemIDwith.group(UUID)and.workspace(UUID)for zero-allocation identity and hashing.ForEach(..., id: \.id), row state/animations, andscrollTotargets remain unchanged.Written for commit 83322e5. Summary will update on new commits.
Summary by CodeRabbit
Release Notes