Inline Hints Perf by akhera99 · Pull Request #55259 · dotnet/roslyn

akhera99 · 2021-07-30T00:22:24Z

Reconfigured code surrounding the creation of inline hint tags. Now we make sure we are in a span that is going to have a hint added prior to calling for the creation of a hint.
Also, Measure is very expensive and I was calling it on the border object rather than the textblock, but it was only necessary for the textblock.

ryzngard · 2021-07-30T04:45:44Z

src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTagger.cs

+
            var selectedSpans = new List<ITagSpan<IntraTextAdornmentTag>>();
-            foreach (var tagSpan in _cache)
+            for (var i = 0; i < _cache.Count; i++)


Suggested change

for (var i = 0; i < _cache.Count; i++)

foreach (var (mappingTagSpan, tagSpan) in _cache)

The index is required for assigning a value back.

ryzngard · 2021-07-30T04:46:56Z

src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTagger.cs

+                if (spans.IntersectsWith(tagSpan))
                {
-                    selectedSpans.Add(tagSpan);
+                    if (_cache[i].tagSpan is not { } hintTagSpan)


Suggested change

if (_cache[i].tagSpan is not { } hintTagSpan)

if (_cache[i].tagSpan is null)

changing to deconstructed foreach will change this slightly, but there's name collisions so it will depend on what you name the deconstructed parts

This doesn't work for lazy initialization.

ryzngard · 2021-07-30T04:49:08Z

src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTagger.cs

            }

+            var document = snapshot.GetOpenDocumentInCurrentContextWithChanges();
+            var classify = document?.Project.Solution.Workspace.Options.GetOption(InlineHintsOptions.ColorHints, document?.Project.Language) ?? false;


Suggested change

var classify = document?.Project.Solution.Workspace.Options.GetOption(InlineHintsOptions.ColorHints, document?.Project.Language) ?? false;

var classify = document is null

? false

: document.Project.Solution.Workspace.Options.GetOption(InlineHintsOptions.ColorHints, document.Project.Language);

I prefer to keep the original form here so it's clear this is the same code just moved down.

ryzngard · 2021-07-30T04:49:50Z

src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTag.cs

            }

-            // Encapsulates the textblock within a border. Gets foreground/background colors from the options menu.
+            block.Measure(new Size(double.PositiveInfinity, double.PositiveInfinity));


While it's good to measure only what's needed, I imagine this won't be much of a perf gain. TextBlock measurement is the expensive part, the border likely had very little overhead here.

Yes, this was an opportunistic improvement from the performance trace. The lazy evaluation is the big win, and this is the distant second but easy to fix.

ryzngard · 2021-07-30T04:51:28Z

src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTagger.cs

        /// stores the parameter hint tags in a global location 
        /// </summary>
-        private readonly List<ITagSpan<IntraTextAdornmentTag>> _cache;
+        private readonly List<(IMappingTagSpan<InlineHintDataTag> mappingTagSpan, ITagSpan<IntraTextAdornmentTag>? tagSpan)> _cache;


Suggested change

private readonly List<(IMappingTagSpan<InlineHintDataTag> mappingTagSpan, ITagSpan<IntraTextAdornmentTag>? tagSpan)> _cache;

private readonly List<(IMappingTagSpan<InlineHintDataTag> mappingTagSpan, ITagSpan<IntraTextAdornmentTag>? tagSpan)> _cache = new();

This lets you remove the line in the constructor and keeps it easy to change in the future if needed

that's a better pattern to have, changed.

ryzngard

Still not 100% sure what cases we're hitting the cache here. snapshot != _cacheSnapshot will be true for scrolling as well as typing right? or is that snapshot only for text changes and not the view changing?

sharwell · 2021-07-30T07:34:24Z

src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTagger.cs

-                            tag.Tag.Hint, Format, _textView, dataTagSpan, _taggerProvider, _formatMap, classify);
-
-                        _cache.Add(new TagSpan<IntraTextAdornmentTag>(dataTagSpan, parameterHintUITag));
+                        _cache.Add((tag, null));


💡 named tuple element for null would be good

sharwell · 2021-07-30T07:36:23Z

@ryzngard cache hits during scrolling. Lazy calculation of tags in the current view (previously was whole document) happens during typing.

ryzngard · 2021-07-30T07:58:58Z

Ah, so the snapshot is the full document text and not the same as the ITextView.Snapshot property.

sharwell · 2021-07-30T15:47:00Z

💭 They should be the same

akhera99 added 2 commits July 29, 2021 16:48

only creating the UI for the hints that intersect with the spans

2a7ec70

removed measure call on border and moved it to block

bb5170d

akhera99 added the Feature - Inline Parameter Hints label Jul 30, 2021

akhera99 requested review from CyrusNajmabadi, ryzngard and sharwell July 30, 2021 00:22

akhera99 self-assigned this Jul 30, 2021

akhera99 requested a review from a team as a code owner July 30, 2021 00:22

akhera99 requested a review from a team July 30, 2021 00:22

ghost added the Area-IDE label Jul 30, 2021

ryzngard reviewed Jul 30, 2021

View reviewed changes

sharwell approved these changes Jul 30, 2021

View reviewed changes

ryzngard approved these changes Jul 30, 2021

View reviewed changes

pr feedback

5d20e88

akhera99 merged commit 5c151ec into dotnet:main-vs-deps Aug 4, 2021

ghost added this to the Next milestone Aug 4, 2021

dibarbet modified the milestones: Next, 17.0.P4 Aug 31, 2021

	for (var i = 0; i < _cache.Count; i++)
	foreach (var (mappingTagSpan, tagSpan) in _cache)

	if (_cache[i].tagSpan is not { } hintTagSpan)
	if (_cache[i].tagSpan is null)

-            var classify = document?.Project.Solution.Workspace.Options.GetOption(InlineHintsOptions.ColorHints, document?.Project.Language) ?? false;
+            var classify = document is null
+                ? false
+                : document.Project.Solution.Workspace.Options.GetOption(InlineHintsOptions.ColorHints, document.Project.Language);

	private readonly List<(IMappingTagSpan<InlineHintDataTag> mappingTagSpan, ITagSpan<IntraTextAdornmentTag>? tagSpan)> _cache;
	private readonly List<(IMappingTagSpan<InlineHintDataTag> mappingTagSpan, ITagSpan<IntraTextAdornmentTag>? tagSpan)> _cache = new();

Conversation

akhera99 commented Jul 30, 2021 • edited by Pilchie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryzngard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Jul 30, 2021

Uh oh!

ryzngard commented Jul 30, 2021

Uh oh!

sharwell commented Jul 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

akhera99 commented Jul 30, 2021 •

edited by Pilchie

Loading