Skip to content

Commit 7225ade

Browse files
authored
Performance fixes (#66)
1 parent aef9470 commit 7225ade

10 files changed

Lines changed: 276 additions & 25 deletions

File tree

shell/mac/Packages/JayJayDiffUI/Sources/JayJayDiffUI/NativeDiffView+WrappedGutter.swift

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,9 +154,14 @@ extension NativeDiffView {
154154
? NSColor.controlAccentColor
155155
: NSColor.selectedTextBackgroundColor
156156
}
157+
// Reuse updateNSView's display lines; expandedHunkRange would re-run the diffDisplayLines FFI per line
158+
// (O(n^2)).
157159
return groupStripeColor(
158160
for: line,
159-
groupRange: expandedHunkRange(containing: lineNumber ... lineNumber),
161+
groupRange: DiffGutterGrouping.expandedChangedRange(
162+
in: context.lines,
163+
containing: lineNumber ... lineNumber
164+
),
160165
theme: context.theme
161166
)
162167
}
Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
import AppKit
2+
import JayJayCore
3+
@testable import JayJayDiffUI
4+
import XCTest
5+
6+
/// Guards the gutter render against the per-line `diffDisplayLines` FFI regression (O(n^2)) that froze large diffs.
7+
final class NativeDiffViewPerformanceTests: XCTestCase {
8+
func test_gutterRender_largeDiff_doesNotBlockMainThread() {
9+
// ~1500 display lines with scattered change groups, like a lockfile bump.
10+
let lines: [DiffLine] = (0 ..< 1500).map { i in
11+
let style: DiffSpanStyle = (i % 6 == 0 || i % 6 == 1) ? .added : .context
12+
return DiffLine(
13+
oldLineNo: UInt32(i + 1),
14+
newLineNo: UInt32(i + 1),
15+
style: style,
16+
spans: [],
17+
conflictKind: .none,
18+
noEofNewline: false
19+
)
20+
}
21+
let view = NativeDiffView(diff: FileDiff(
22+
path: "Cargo.lock",
23+
language: "",
24+
lines: lines,
25+
whitespaceOnlyHidden: false
26+
))
27+
let (gutterTextView, gutterLayoutManager) = makeGutter()
28+
let context = makeContext(lines: lines)
29+
30+
let start = Date()
31+
_ = view.renderWrappedGutter(
32+
gutterTextView: gutterTextView,
33+
gutterLayoutManager: gutterLayoutManager,
34+
context: context
35+
)
36+
let elapsedMs = Date().timeIntervalSince(start) * 1000
37+
38+
// Post-fix: single-digit ms; pre-fix O(n^2) FFI: multiple seconds. 500ms gates the regression with CI headroom.
39+
XCTAssertLessThan(
40+
elapsedMs, 500,
41+
"gutter render took \(Int(elapsedMs))ms for 1500 lines — per-line diffDisplayLines FFI likely reintroduced"
42+
)
43+
}
44+
45+
// MARK: - Fixtures
46+
47+
private func makeGutter() -> (DiffGutterTextView, DiffLayoutManager) {
48+
let container = NSTextContainer(containerSize: NSSize(
49+
width: 80,
50+
height: CGFloat.greatestFiniteMagnitude
51+
))
52+
container.widthTracksTextView = true
53+
container.lineFragmentPadding = 0
54+
55+
let manager = DiffLayoutManager()
56+
manager.addTextContainer(container)
57+
58+
let storage = NSTextStorage()
59+
storage.addLayoutManager(manager)
60+
61+
let textView = DiffGutterTextView(
62+
frame: NSRect(x: 0, y: 0, width: 80, height: 400),
63+
textContainer: container
64+
)
65+
return (textView, manager)
66+
}
67+
68+
private func makeContext(lines: [DiffLine]) -> NativeDiffGutterRenderContext {
69+
let font = NSFont.monospacedSystemFont(ofSize: 12, weight: .regular)
70+
let paragraph = NSMutableParagraphStyle()
71+
return NativeDiffGutterRenderContext(
72+
lines: lines,
73+
visualLineCounts: Array(repeating: 1, count: lines.count),
74+
font: font,
75+
theme: DiffColors(isDark: false),
76+
gutterAttrs: [.font: font],
77+
gutterParagraphStyle: paragraph,
78+
maxLineDigits: 4,
79+
showsReviewCheckboxes: false,
80+
showsCheckboxColumn: false,
81+
firstLineOfGroup: [:],
82+
groupIndexAtLineNumber: [:],
83+
reviewActions: nil,
84+
groupStripeWidth: 6,
85+
gutterHorizontalInset: 8,
86+
gutterTrailingPadding: 10,
87+
currentSelectedLineRange: nil
88+
)
89+
}
90+
}

shell/mac/Sources/JayJay/Detail/DetailHeader.swift

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,10 +123,11 @@ extension ChangeDetailView {
123123
loadTrackedGitLfsPaths()
124124
loadDiffStats()
125125
refreshReviewedPaths()
126-
diffStore.clear()
126+
// No clear(): content-addressed by commit id, so prior changes stay warm and never go stale.
127127
diffStore.preload(
128128
hunks: detail.diff,
129129
rev: detailRevision,
130+
commitId: detail.info.commitId,
130131
repo: repo,
131132
compareFromRev: compareFromId,
132133
ignoreWhitespace: appSettings.ignoreWhitespace

shell/mac/Sources/JayJay/Detail/DetailView.swift

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,7 @@ struct ChangeDetailView: View {
288288
DiffSection(
289289
hunk: hunk,
290290
rev: detailRevision,
291+
commitId: detail.info.commitId,
291292
repo: repo,
292293
actions: actions,
293294
isWorkingCopy: detail.info.isWorkingCopy,

shell/mac/Sources/JayJay/Diff/DiffSection.swift

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import SwiftUI
55
struct DiffSection: View, DiffGutterEditActions, DiffGutterReviewActions {
66
let hunk: DiffHunk
77
let rev: String?
8+
var commitId: String?
89
let repo: JayJayRepo?
910
let actions: (any ChangeActions & DAGActions)?
1011
let isWorkingCopy: Bool
@@ -279,11 +280,12 @@ struct DiffSection: View, DiffGutterEditActions, DiffGutterReviewActions {
279280
fileDiff = nil
280281

281282
if let cached = await diffStore.loadDiff(
282-
hunk: hunk, rev: rev, repo: repo,
283+
hunk: hunk, rev: rev, commitId: commitId, repo: repo,
283284
compareFromRev: compareFromRev,
284285
ignoreWhitespace: settings.ignoreWhitespace
285286
) {
286-
guard hunk.path == path else { return }
287+
// Bail if a newer .task superseded us so we don't overwrite fresh state with a stale diff.
288+
guard !Task.isCancelled, hunk.path == path else { return }
287289
fileDiff = cached.diff
288290
loadedOldContent = cached.oldContent
289291
loadedNewContent = cached.newContent

shell/mac/Sources/JayJay/Diff/DiffStore.swift

Lines changed: 79 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -14,32 +14,32 @@ final class DiffStore {
1414
}
1515

1616
private let cache = DiffCache()
17-
18-
func get(rev: String?, path: String) async -> CachedDiff? {
19-
await cache.get(Self.key(rev: rev, path: path))
20-
}
21-
22-
func set(rev: String?, path: String, value: CachedDiff) async {
23-
await cache.set(Self.key(rev: rev, path: path), value: value)
24-
}
17+
@ObservationIgnored private var preloadTask: Task<Void, Never>?
2518

2619
func clear() {
2720
Task { await cache.clear() }
2821
}
2922

3023
/// Load a single file's diff. Returns cached if available, otherwise computes and caches.
24+
///
25+
/// `commitId` is the immutable content hash used as the cache identity. `rev`
26+
/// (the mutable selection revision) is what jj resolves to fetch content, but
27+
/// keying on it would serve stale diffs after an amend/rebase reuses the id.
3128
func loadDiff(
3229
hunk: DiffHunk,
3330
rev: String?,
31+
commitId: String? = nil,
3432
repo: JayJayRepo?,
3533
compareFromRev: String? = nil,
3634
ignoreWhitespace: Bool = false
3735
) async -> CachedDiff? {
3836
guard let repo else { return nil }
3937
guard !hunk.isSubmodulePlaceholder else { return nil }
4038

41-
let cacheRev = compareFromRev != nil ? "\(compareFromRev!)\(rev ?? "")" : rev
42-
let key = Self.key(rev: cacheRev, path: hunk.path)
39+
let key = Self.key(
40+
commitId: commitId, rev: rev, compareFromRev: compareFromRev,
41+
ignoreWhitespace: ignoreWhitespace, path: hunk.path
42+
)
4343

4444
if let cached = await cache.get(key) {
4545
return cached
@@ -81,28 +81,40 @@ final class DiffStore {
8181
func preload(
8282
hunks: [DiffHunk],
8383
rev: String?,
84+
commitId: String? = nil,
8485
repo: JayJayRepo?,
8586
compareFromRev: String? = nil,
8687
ignoreWhitespace: Bool = false
8788
) {
8889
guard let repo, let rev else { return }
89-
preloadHunks(hunks, rev: rev, repo: repo, compareFromRev: compareFromRev, ignoreWhitespace: ignoreWhitespace)
90+
// Cancel any in-flight preload so rapid commit navigation doesn't pile
91+
// up detached tasks all racing the FFI.
92+
preloadTask?.cancel()
93+
preloadTask = preloadHunks(
94+
hunks, rev: rev, commitId: commitId, repo: repo,
95+
compareFromRev: compareFromRev, ignoreWhitespace: ignoreWhitespace
96+
)
9097
}
9198

9299
// MARK: - Private
93100

101+
@discardableResult
94102
private func preloadHunks(
95103
_ hunks: [DiffHunk],
96104
rev: String,
105+
commitId: String?,
97106
repo: JayJayRepo,
98107
compareFromRev: String?,
99108
ignoreWhitespace: Bool
100-
) {
109+
) -> Task<Void, Never> {
101110
Task.detached(priority: .utility) { [cache] in
102111
for hunk in hunks {
112+
if Task.isCancelled { return }
103113
guard !hunk.isSubmodulePlaceholder else { continue }
104-
let cacheRev = compareFromRev != nil ? "\(compareFromRev!)\(rev)" : rev
105-
let key = DiffStore.key(rev: cacheRev, path: hunk.path)
114+
let key = DiffStore.key(
115+
commitId: commitId, rev: rev, compareFromRev: compareFromRev,
116+
ignoreWhitespace: ignoreWhitespace, path: hunk.path
117+
)
106118
if await cache.get(key) != nil { continue }
107119

108120
var old = hunk.oldContent ?? ""
@@ -198,24 +210,73 @@ final class DiffStore {
198210
return LoadedFileContent(oldContent: "", newContent: "", oldPreview: nil, newPreview: nil)
199211
}
200212

201-
nonisolated private static func key(rev: String?, path: String) -> String {
202-
"\(rev ?? "")|\(path)"
213+
/// Content-addressed cache key: immutable `commitId` (falling back to `rev`),
214+
/// the compare-from side, the whitespace mode, and the path. Whitespace is
215+
/// part of the key because it changes the computed diff for the same content.
216+
nonisolated static func key(
217+
commitId: String?,
218+
rev: String?,
219+
compareFromRev: String?,
220+
ignoreWhitespace: Bool,
221+
path: String
222+
) -> String {
223+
let base = (commitId?.isEmpty == false ? commitId : rev) ?? ""
224+
let identity = compareFromRev.map { "\($0)\(base)" } ?? base
225+
return "\(identity)|\(ignoreWhitespace ? "iw" : "")|\(path)"
203226
}
204227
}
205228

206-
/// Thread-safe diff cache actor.
229+
/// Thread-safe LRU diff cache bounded by the total bytes of cached file content.
207230
actor DiffCache {
208231
private var entries: [String: DiffStore.CachedDiff] = [:]
232+
private var order: [String] = [] // LRU recency; front = least recently used
233+
private var totalBytes = 0
234+
private let budgetBytes: Int
235+
236+
init(budgetBytes: Int = 64 * 1024 * 1024) {
237+
self.budgetBytes = budgetBytes
238+
}
209239

210240
func get(_ key: String) -> DiffStore.CachedDiff? {
211-
entries[key]
241+
guard let value = entries[key] else { return nil }
242+
touch(key)
243+
return value
212244
}
213245

214246
func set(_ key: String, value: DiffStore.CachedDiff) {
247+
if let existing = entries[key] {
248+
totalBytes -= bytes(existing)
249+
order.removeAll { $0 == key }
250+
}
215251
entries[key] = value
252+
order.append(key)
253+
totalBytes += bytes(value)
254+
evict()
216255
}
217256

218257
func clear() {
219258
entries.removeAll()
259+
order.removeAll()
260+
totalBytes = 0
261+
}
262+
263+
private func touch(_ key: String) {
264+
order.removeAll { $0 == key }
265+
order.append(key)
266+
}
267+
268+
/// Drop least-recently-used entries until under budget, always keeping the
269+
/// most recent one (so a single oversized file is still cached for its view).
270+
private func evict() {
271+
while totalBytes > budgetBytes, order.count > 1, let oldest = order.first {
272+
order.removeFirst()
273+
if let removed = entries.removeValue(forKey: oldest) {
274+
totalBytes -= bytes(removed)
275+
}
276+
}
277+
}
278+
279+
private func bytes(_ diff: DiffStore.CachedDiff) -> Int {
280+
diff.oldContent.utf8.count + diff.newContent.utf8.count
220281
}
221282
}

shell/mac/Sources/JayJay/DiffEdit/DiffEditFileSection.swift

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import SwiftUI
55
struct DiffEditFileSection: View, DiffGutterSelectionActions {
66
let hunk: DiffHunk
77
let rev: String
8+
var commitId: String?
89
let repo: JayJayRepo?
910
let diffStore: DiffStore
1011
let selectedChangedLines: Set<Int>
@@ -129,7 +130,8 @@ struct DiffEditFileSection: View, DiffGutterSelectionActions {
129130

130131
// Reuse DiffStore for file content loading (cached if already loaded by DiffSection)
131132
let cached = await diffStore.loadDiff(
132-
hunk: hunk, rev: rev, repo: repo, ignoreWhitespace: settings.ignoreWhitespace
133+
hunk: hunk, rev: rev, commitId: commitId, repo: repo,
134+
ignoreWhitespace: settings.ignoreWhitespace
133135
)
134136
let old = cached?.oldContent
135137
let new = cached?.newContent

shell/mac/Sources/JayJay/DiffEdit/DiffEditView.swift

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ struct DiffEditView: View {
3232
DiffEditFileSection(
3333
hunk: hunk,
3434
rev: detailRevision,
35+
commitId: detail.info.commitId,
3536
repo: repo,
3637
diffStore: diffStore,
3738
selectedChangedLines: selectedChangedLinesByPath[hunk.path] ?? [],

shell/mac/Sources/JayJay/Repo/ViewModel/Core/RepoViewModel+Selection.swift

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -226,10 +226,19 @@ extension RepoViewModel {
226226
compareDisplay = display
227227
selectedChangeId = to
228228
load {
229-
try $0.interdiffSummary(fromRev: from, toRev: to)
230-
} onSuccess: { viewModel, detail in
229+
let detail = try $0.interdiffSummary(fromRev: from, toRev: to)
230+
// Resolve the compare source to its immutable commit id so the diff
231+
// cache key is content-addressed on both sides; otherwise amending a
232+
// mutable `from` (a change id) would keep serving a stale interdiff.
233+
let fromCommitId = (try? $0.log(revset: from))?.first?.commitId
234+
return (detail, fromCommitId)
235+
} onSuccess: { viewModel, result in
236+
let (detail, fromCommitId) = result
231237
viewModel.selectedChange = detail
232238
viewModel.selectedChangeId = detail.info.selectionRevision
239+
if let fromCommitId, !fromCommitId.isEmpty {
240+
viewModel.compareFromId = fromCommitId
241+
}
233242
} onFailure: { viewModel, error in
234243
viewModel.compareFromId = nil
235244
viewModel.compareToId = nil

0 commit comments

Comments
 (0)