From 2846f00080b0d792355166e52db3ae9d4b5b5941 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Mon, 29 Jun 2026 23:39:45 +0200 Subject: [PATCH] Stop rescanning references to paint the bookmark gutter every frame Two costs on the gutter's hot path: The margin rebuilt its document-line -> glyph map on every Render, resolving each bookmark by scanning the document's reference segments. A scroll, a hover, or the 600 ms post-navigation pulse repaints many times a second, so this rescanned the references per bookmark per frame. Compute the map once and reuse it, clearing the cache only when the document or the bookmark set changes (the model's DebugInfo/References are set before the text, so the document-change hook sees them current). CanToggleBookmarkAtLine, called for every gutter line the pointer moves over, built a full bookmark including a captured editor view state -- a foldings snapshot, scroll offsets, and a tree-path walk -- only to test it for null. It now does an anchor-only check; the view state is captured only when a bookmark is actually created. Assisted-by: Claude:claude-opus-4-8:Claude Code --- ILSpy.Tests/Bookmarks/BookmarkGutterTests.cs | 65 ++++++++++++++++++++ ILSpy/Bookmarks/BookmarkMargin.cs | 57 ++++++++++++----- ILSpy/TextView/DecompilerTextView.axaml.cs | 24 ++++++-- 3 files changed, 127 insertions(+), 19 deletions(-) diff --git a/ILSpy.Tests/Bookmarks/BookmarkGutterTests.cs b/ILSpy.Tests/Bookmarks/BookmarkGutterTests.cs index 5215c0043..d73f845bc 100644 --- a/ILSpy.Tests/Bookmarks/BookmarkGutterTests.cs +++ b/ILSpy.Tests/Bookmarks/BookmarkGutterTests.cs @@ -30,8 +30,10 @@ using AvaloniaEdit.Rendering; using AwesomeAssertions; +using ICSharpCode.ILSpy; using ICSharpCode.ILSpy.AppEnv; using ICSharpCode.ILSpy.Bookmarks; +using ICSharpCode.ILSpy.Properties; using ICSharpCode.ILSpy.TextView; using ICSharpCode.ILSpy.TreeNodes; @@ -210,4 +212,67 @@ public class BookmarkGutterTests Dispatcher.UIThread.RunJobs(); margin.HoverPreviewLine.Should().Be(line0, "leaving and re-entering the line restores its preview"); } + + // Regression: the editor reuses one TextDocument and only swaps its text on navigation, so the + // gutter's cached glyph map must rebuild off the document's content (DebugInfo/References), not off + // the document reference -- otherwise switching nodes leaves the gutter showing the previous (or no) + // document's bookmarks. + [AvaloniaTest] + public async Task Gutter_Glyph_Map_Rebuilds_When_The_Displayed_Document_Changes() + { + var (window, vm) = await TestHarness.BootAsync(3); + var manager = AppComposition.Current.GetExport(); + manager.Clear(); + var coreLibName = typeof(object).Assembly.GetName().Name!; + + async Task Show(string typeName) + { + var node = vm.AssemblyTreeModel.FindNode(coreLibName, "System", typeName); + vm.AssemblyTreeModel.SelectNode(node); + await vm.DockWorkspace.WaitForDecompiledTextAsync(); + var shown = await window.WaitForComponent(); + for (int i = 0; i < 8; i++) + { + Dispatcher.UIThread.RunJobs(); + await Task.Delay(25); + } + return shown; + } + + var view = await Show("System.Object"); + var margin = view.Editor.TextArea.LeftMargins.OfType().Single(); + var toggle = AppComposition.Current.GetExport().GetEntry(nameof(Resources.BookmarkToggle)); + + // A body (IL-offset) anchor, so it only resolves inside System.Object -- a token/line anchor on + // the type would also resolve in System.String, which merely references System.Object. + Bookmark? bookmark = null; + foreach (int candidate in Enumerable.Range(1, view.Editor.Document.LineCount).Where(view.CanToggleBookmarkAtLine)) + { + int candidateOffset = view.Editor.Document.GetLineByNumber(candidate).Offset; + view.Editor.TextArea.Caret.Offset = candidateOffset; + toggle.Execute(new TextViewContext { TextView = view, TextLocation = candidateOffset }); + Dispatcher.UIThread.RunJobs(); + var candidateBookmark = manager.Bookmarks.Single(); + if (candidateBookmark.Kind == BookmarkKind.Body) + { + bookmark = candidateBookmark; + break; + } + manager.Clear(); + } + bookmark.Should().NotBeNull("System.Object has a method-body line to anchor to"); + int objectLine = view.GetLineForBookmark(bookmark!)!.Value; + margin.GlyphLinesForTest().Keys.Should().Contain(objectLine, "the gutter shows the bookmark on its own document"); + + // Navigate to another type. The preview tab reuses the same view (and gutter), only swapping the + // document text, so the gutter must drop the stale glyph: System.Object's bookmark is not here. + var stringView = await Show("System.String"); + stringView.Should().BeSameAs(view, "the preview tab reuses the view, so this exercises the stale cache"); + margin.GlyphLinesForTest().Should().BeEmpty("the gutter must rebuild for System.String, where the bookmark does not resolve"); + + // Returning to the bookmarked document must surface its glyph again. + await Show("System.Object"); + margin.GlyphLinesForTest().Keys.Should().Contain(view.GetLineForBookmark(bookmark!)!.Value, + "returning to the bookmarked document must show its glyph again"); + } } diff --git a/ILSpy/Bookmarks/BookmarkMargin.cs b/ILSpy/Bookmarks/BookmarkMargin.cs index 68a413462..038abdd86 100644 --- a/ILSpy/Bookmarks/BookmarkMargin.cs +++ b/ILSpy/Bookmarks/BookmarkMargin.cs @@ -60,6 +60,9 @@ namespace ICSharpCode.ILSpy.Bookmarks int hoverLine = -1; // A line whose hover preview stays hidden after a removal click, until the pointer leaves it. int suppressedHoverLine = -1; + // Cached document-line -> glyph map and the content version it was built against. + Dictionary? glyphByLine; + (object?, object?) glyphByLineVersion; public BookmarkMargin(TextView.DecompilerTextView owner) { @@ -105,9 +108,14 @@ namespace ICSharpCode.ILSpy.Bookmarks pulseTimer.Stop(); pulseLine = -1; hoverLine = -1; + glyphByLine = null; } - void OnBookmarksChanged(object? sender, EventArgs e) => InvalidateVisual(); + void OnBookmarksChanged(object? sender, EventArgs e) + { + glyphByLine = null; + InvalidateVisual(); + } /// Plays the one-shot bounce on the bookmark glyph at . public void PulseLine(int line) @@ -146,20 +154,7 @@ namespace ICSharpCode.ILSpy.Bookmarks if (manager == null || textView == null || !textView.VisualLinesValid) return; - // Map the document lines that currently hold a bookmark to their glyph. Bookmarks not in - // this document resolve to no line and are skipped. - var glyphByLine = new Dictionary(); - foreach (var bookmark in manager.Bookmarks) - { - if (owner.GetLineForBookmark(bookmark, updateRenderedLine: false) is { } line) - { - if (bookmark.LineNumber != line) - { - Dispatcher.UIThread.Post(() => bookmark.UpdateRenderedLineNumber(line), DispatcherPriority.Background); - } - glyphByLine[line] = bookmark.Enabled ? Images.Bookmark : Images.BookmarkDisable; - } - } + var glyphByLine = GetGlyphByLine(manager); foreach (var visualLine in textView.VisualLines) { int lineNumber = visualLine.FirstDocumentLine.LineNumber; @@ -194,6 +189,33 @@ namespace ICSharpCode.ILSpy.Bookmarks } } + // The document line -> glyph map for the current document and bookmark set. Resolving a bookmark + // to its line scans the document's references, so it is computed once and reused across the many + // repaints a scroll, hover or pulse triggers. It is rebuilt when the bookmark set changes (which + // clears the cache) or when the displayed document changes -- detected by its content version, + // since the editor reuses one TextDocument and only swaps its text, so the document reference + // never changes. + Dictionary GetGlyphByLine(BookmarkManager manager) + { + var version = owner.BookmarkContentVersion; + if (glyphByLine != null && version.Equals(glyphByLineVersion)) + return glyphByLine; + glyphByLineVersion = version; + + // Bookmarks not anchored in this document resolve to no line and are skipped. + var map = new Dictionary(); + foreach (var bookmark in manager.Bookmarks) + { + if (owner.GetLineForBookmark(bookmark, updateRenderedLine: false) is { } line) + { + if (bookmark.LineNumber != line) + Dispatcher.UIThread.Post(() => bookmark.UpdateRenderedLineNumber(line), DispatcherPriority.Background); + map[line] = bookmark.Enabled ? Images.Bookmark : Images.BookmarkDisable; + } + } + return glyphByLine = map; + } + // A bitmap copy of the bookmark glyph, shared by all gutters, used only for the hover preview. // SvgImage paints through a custom Skia draw operation that ignores DrawingContext.PushOpacity, // so the SVG can't be faded directly; a rasterized bitmap is composited at the pushed opacity. @@ -300,5 +322,10 @@ namespace ICSharpCode.ILSpy.Bookmarks // The line currently drawing a hover-preview glyph, or -1 when none. Exposed for tests. internal int HoverPreviewLine => hoverLine; + + // The document lines that currently carry a bookmark glyph, resolved against the live document. + // Exposed for tests because the gutter only paints when rendering is on (off in the headless CI). + internal IReadOnlyDictionary GlyphLinesForTest() + => Manager is { } m ? GetGlyphByLine(m) : new Dictionary(); } } diff --git a/ILSpy/TextView/DecompilerTextView.axaml.cs b/ILSpy/TextView/DecompilerTextView.axaml.cs index 96aa68971..a0878826f 100644 --- a/ILSpy/TextView/DecompilerTextView.axaml.cs +++ b/ILSpy/TextView/DecompilerTextView.axaml.cs @@ -686,14 +686,22 @@ namespace ICSharpCode.ILSpy.TextView // Bookmarks live only on the decompiled C# view, not on IL / metadata / resource output. bool ShowsBookmarkableCode => DataContext is DecompilerTabPageModel { SyntaxExtension: ".cs" }; - Bookmarks.Bookmark? CreateBookmarkForLine(int line) + // Just the anchor for a line, or null when the line can't be bookmarked. Cheap enough to call + // per hovered line: it does not capture the editor view state (foldings, scroll, tree path), + // which only a bookmark that is actually being created needs. + Bookmarks.Bookmark? CreateAnchorForLine(int line) { if (!ShowsBookmarkableCode || DataContext is not DecompilerTabPageModel model) return null; var fallbackOwner = model.CurrentNode is IMemberTreeNode memberNode ? memberNode.Member : null; - var bookmark = Bookmarks.BookmarkAnchoring.CreateForLine(model.DebugInfo, model.References, Editor.Document, line, + return Bookmarks.BookmarkAnchoring.CreateForLine(model.DebugInfo, model.References, Editor.Document, line, fallbackOwner, GetLocationNodeName(model.CurrentNode)); - if (bookmark != null) + } + + Bookmarks.Bookmark? CreateBookmarkForLine(int line) + { + var bookmark = CreateAnchorForLine(line); + if (bookmark != null && DataContext is DecompilerTabPageModel model) { var displaySettings = AppComposition.TryGetExport()?.DisplaySettings; var selectedTreeNodePath = AssemblyTreeModel.GetPathForNode(model.CurrentNode); @@ -706,7 +714,7 @@ namespace ICSharpCode.ILSpy.TextView => node is IMemberTreeNode { Member: { } member } ? member.FullName : node?.ToString(); /// Whether can hold a bookmark in the current document. - internal bool CanToggleBookmarkAtLine(int line) => CreateBookmarkForLine(line) != null; + internal bool CanToggleBookmarkAtLine(int line) => CreateAnchorForLine(line) != null; internal bool CanToggleBookmarkAtOffset(int offset) { @@ -855,6 +863,14 @@ namespace ICSharpCode.ILSpy.TextView scrollViewer.Offset = new Vector(scrollViewer.Offset.X, Math.Max(0, target)); } + // The identity of the inputs that decide where bookmarks resolve in the current C# document. + // Both are replaced on every decompile, while the editor reuses one TextDocument (only its text + // changes), so this -- not the document reference -- is what tells the gutter its cache is stale. + internal (object? DebugInfo, object? References) BookmarkContentVersion + => DataContext is DecompilerTabPageModel { SyntaxExtension: ".cs" } model + ? (model.DebugInfo, model.References) + : default; + /// /// The document line a bookmark sits on in the currently shown C# document, or null when the /// bookmark belongs to other code. Body anchors resolve via the IL-offset map; token anchors