Avoid rooting document instances in NavigateToSearchResult by sharwell · Pull Request #68068 · dotnet/roslyn

sharwell · 2023-05-03T03:03:06Z

Addresses 11.1GB retained set observed in a local heap dump.

Submitted as draft for PR validation.

Follow-up to #68071

CyrusNajmabadi · 2023-05-04T17:44:32Z

+                    var document = await item.Document.GetDocumentAsync(context.Solution, cancellationToken).ConfigureAwait(false);
                    var location = await ProtocolConversions.TextSpanToLocationAsync(
-                        item.Document, item.SourceSpan, item.IsStale, cancellationToken).ConfigureAwait(false);
+                        document, item.SourceSpan, item.IsStale, cancellationToken).ConfigureAwait(false);


i don't think this is safe anymore. item.IsStale was previously used to say that the result info might not match teh current snapshot. But now that you're not rooting an actual snapshot, it may always be the case that things may be stale (e.g. spans map to things that are now no longer legal).

We use context.Solution to obtain the document, which is the same solution used for the call to GetItemsFromPreferredSourceLocations. It seems like this would allow it to be correct.

➡️ This thread is covered by #68068 (comment)

CyrusNajmabadi · 2023-05-04T17:45:31Z

+                        {
+                            return (self._location.IsInSource && self._solution.GetDocument(self._location.SourceTree) is { } document)
+                                ? INavigableItem.NavigableDocument.FromDocument(document)
+                                : null;


this is concerning. We can return null, but the API says it's not null.

It's not clear which item you are referring to (there are two return values in question).

This overload of EnsureInitialized operates on a StrongBox<T> specifically because it allows the initialized value to be null.

roslyn/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/LazyInitialization.cs

Lines 89 to 101 in 806dba8

/// <summary>

/// Ensure that the given target value is initialized in a thread-safe manner. This overload supports the

/// initialization of value types, and reference type fields where <see langword="null"/> is considered an

/// initialized value.

/// </summary>

/// <typeparam name="T">The type of the target value.</typeparam>

/// <param name="target">A target value box to initialize.</param>

/// <typeparam name="U">The type of the <paramref name="state"/> argument passed to the value factory.</typeparam>

/// <param name="valueFactory">A factory delegate to create a new instance of the target value. Note that this delegate may be called

/// more than once by multiple threads, but only one of those values will successfully be written to the target.</param>

/// <param name="state">An argument passed to the value factory.</param>

/// <returns>The target value.</returns>

public static T? EnsureInitialized<T, U>([NotNull] ref StrongBox<T?>? target, Func<U, T?> valueFactory, U state)

It's not clear which item you are referring to

Api is: public INavigableItem.NavigableDocument Document which is non-null. my guess is the file is not NRT annotated though. If that's the case, you don't have to change anything.

If this is NRT annotated, something is wrong :)

➡️ This file does not have null annotations enabled currently.

CyrusNajmabadi · 2023-05-04T17:46:17Z

+                };
+
+            internal ValueTask<Document> GetDocumentAsync(Solution solution, CancellationToken cancellationToken)
+                => solution.GetRequiredDocumentAsync(Id, includeSourceGenerated: IsSourceGeneratedDocument, cancellationToken);


this doesn't fele safe. Since we're not rooting the solution, then items may refer to docs that now no longer exist. we should reflect that in this api and make sure downstream consumers are resilient to that happening.

Wouldn't a failure here require the deletion of a document containing a Navigate To result while the Navigate To UI was active? I can try to make it not required though.

➡️ There was only one call to GetDocumentAsync (directly or indirectly) which didn't pass in the same solution used to create the items in the first place. It was the call to GetTextSynchronously, which has now been updated to TryGetTextSynchronously and the caller updated to handle the case where the document was no longer available.

The navto system (now called AIOSP) supports pinning the results, and going back to them and interacting with them post search. So it would be possible to potentially run into a doc that no longer exists. Def wonky for sure. I'm ok if you punt on that. just a note in the code would be good.

i support this given that this passes in the original solution. Can you doc this though that's mandatory for this to be safe.

I think you should also rename this to GetRequiredDocumentAsync to make it clear it will die if not there, and that's a bug in the caller for passing bad data.

If you do the above, i'm fine with this approach :)

➡️ Documented and renamed

CyrusNajmabadi · 2023-05-04T17:46:41Z

+                return await document.GetValueTextAsync(cancellationToken).ConfigureAwait(false);
+            }
+
+            internal SourceText GetTextSynchronously(Solution solution, CancellationToken cancellationToken)


what's teh scenario where the text is needed synchronously?

➡️ This method contains an extra parameter (solution) relative to the original use, so all calls to this method will appear in the diff if you search for GetTextSynchronously. There is one call in NavigateToItemDisplay.cs.

CyrusNajmabadi · 2023-05-04T17:47:10Z

    {
-        public int GetProvisionalViewingStatus(Document document)
-            => 0;
+        public __VSPROVISIONALVIEWINGSTATUS GetProvisionalViewingStatus(INavigableItem.NavigableDocument document)


don't love exposing this API directly. Can we have our own enum to wrap?

➡️ This non-public API is tightly coupled to Visual Studio, so wrapping seems unnecessary. In addition to duplicating values that already exist, it makes it more difficult to find documentation and other references to the same structure/values.

CyrusNajmabadi · 2023-05-04T17:49:15Z


-            var sourceText = document.GetTextSynchronously(CancellationToken.None);
+            var sourceText = document.GetTextSynchronously(document.Workspace.CurrentSolution, CancellationToken.None);
            var span = NavigateToUtilities.GetBoundedSpan(_searchResult.NavigableItem, sourceText);


ick. i never noticed this before. This really violates a core principle of navto (that no work is done client side to avoid costly UI perf). This is on the UI thread and goes and potentially does IO. Can you make issue that says that server should compute and add this info to items (ideally only if "show descriptions" is selected by user), and the client just has to present. no need for GetTextSynchronously then.

➡️ #68104

sharwell · 2023-05-11T16:44:38Z

/azp run roslyn-integration-corehost

azure-pipelines · 2023-05-11T16:44:48Z

Azure Pipelines successfully started running 1 pipeline(s).

sharwell · 2023-05-11T19:32:00Z

@CyrusNajmabadi this one finally has a passing build. Good to merge?

CyrusNajmabadi · 2023-05-12T18:47:22Z

                {
                    // No need to compute anything if there is no active document.  Consider all documents equal.
-                    if (_activeDocument == null)
+                    if (_activeDocument is not { } activeDocument)


feels unnecessary right? since we did a null check, the later lines below shoudl be safe to refer to _activeDocument, right?

(that's my personal preference, as i always find it harder to reason when there are two varaibles (with almost identical names) pointing to the same value in teh same scope.

➡️ _activeDocument is Nullable<T>, but activeDocument is T.

CyrusNajmabadi · 2023-05-12T18:47:51Z

+        public record NavigableDocument(NavigableProject Project, string Name, string? FilePath, IReadOnlyList<string> Folders, DocumentId Id)
+        {
+            public required bool IsSourceGeneratedDocument { get; init; }
+            public required Workspace Workspace { get; init; }


why have some methods be parameters, and some be required props?

➡️ Moved all to parameters

CyrusNajmabadi · 2023-05-12T18:49:03Z

+            internal ValueTask<Document> GetDocumentAsync(Solution solution, CancellationToken cancellationToken)
+                => solution.GetRequiredDocumentAsync(Id, includeSourceGenerated: IsSourceGeneratedDocument, cancellationToken);
+
+            internal async ValueTask<SourceText> GetTextAsync(Solution solution, CancellationToken cancellationToken)


GetRequiredText.

➡️ GetText is always required in Roslyn.

CyrusNajmabadi · 2023-05-12T18:50:10Z

            private readonly Solution _solution;
            private readonly ISymbol _symbol;
            private readonly Location _location;
+            private StrongBox<INavigableItem.NavigableDocument> _lazyDocument;


Maybe doc this? That null means, uncomputed, but we could compute and still point at null?

⏱️ Enabling nullable reference types on this file to improve clarity

➡️ Ended up documenting this, but not changing the nullability. There is a discrepancy in nullability which existed prior to this work, and I opted to defer fixing that.

CyrusNajmabadi

Signing off, with the feedback on a few things to touch up. If you disagree, lmk and we can discuss. Primarily it was doc'ing a couple of things, and then using 'Required' in the names of things to convey the expectations. Thanks, this looks great.

ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 3, 2023

dotnet deleted a comment from azure-pipelines bot May 3, 2023

sharwell marked this pull request as ready for review May 4, 2023 02:27

sharwell requested review from a team, 333fred and dibarbet as code owners May 4, 2023 02:27

sharwell requested a review from a team May 4, 2023 02:27

sharwell requested a review from a team as a code owner May 4, 2023 02:27

sharwell marked this pull request as draft May 4, 2023 02:27

sharwell added 2 commits May 4, 2023 10:11

Avoid holding _activeDocument as Document instance

ae18d23

Avoid rooting document instances in NavigateToSearchResult

92459b8

sharwell force-pushed the navigable-document branch from 1d0c2af to 92459b8 Compare May 4, 2023 15:11

sharwell marked this pull request as ready for review May 4, 2023 15:11

sharwell commented May 4, 2023

View reviewed changes

Comment thread src/Features/Core/Portable/Navigation/INavigableItem.cs

Comment thread src/Features/Core/Portable/Navigation/INavigableItem.cs Outdated

sharwell commented May 4, 2023

View reviewed changes

Comment thread src/EditorFeatures/Core.Wpf/NavigateTo/NavigateToItemDisplay.cs

sharwell commented May 4, 2023

View reviewed changes

Comment thread src/Features/Core/Portable/ExternalAccess/VSTypeScript/VSTypeScriptNavigableItemWrapper.cs

sharwell added 2 commits May 4, 2023 10:39

Only request source generated documents when necessary

092a9ec

Avoid unnecessary task allocations

a9f7d94

sharwell closed this May 4, 2023

sharwell reopened this May 4, 2023

CyrusNajmabadi reviewed May 4, 2023

View reviewed changes

Comment thread src/VisualStudio/Core/Def/Progression/GraphBuilder.cs Outdated

CyrusNajmabadi reviewed May 4, 2023

View reviewed changes

sharwell added 2 commits May 4, 2023 16:13

Handle failure to resolve a navigable document text

60bcec9

Restore original expression from

78e6e08

sharwell mentioned this pull request May 4, 2023

CreateDescriptionItems should not load document text on the UI thread #68104

Closed

CyrusNajmabadi reviewed May 12, 2023

View reviewed changes

Comment thread src/EditorFeatures/Core.Wpf/NavigateTo/NavigateToItemDisplay.cs

CyrusNajmabadi reviewed May 12, 2023

View reviewed changes

CyrusNajmabadi approved these changes May 12, 2023

View reviewed changes

Updates for clarity and consistency

35dcfa7

sharwell enabled auto-merge May 12, 2023 21:20

sharwell merged commit bafaa31 into dotnet:main May 13, 2023

ghost added this to the Next milestone May 13, 2023

sharwell deleted the navigable-document branch May 15, 2023 16:29

allisonchou mentioned this pull request May 16, 2023

[Automated] PRs inserted in VS build main-33716.03 #68205

Closed

Cosifne modified the milestones: Next, 17.7 P2 May 31, 2023

	/// <summary>
	/// Ensure that the given target value is initialized in a thread-safe manner. This overload supports the
	/// initialization of value types, and reference type fields where <see langword="null"/> is considered an
	/// initialized value.
	/// </summary>
	/// <typeparam name="T">The type of the target value.</typeparam>
	/// <param name="target">A target value box to initialize.</param>
	/// <typeparam name="U">The type of the <paramref name="state"/> argument passed to the value factory.</typeparam>
	/// <param name="valueFactory">A factory delegate to create a new instance of the target value. Note that this delegate may be called
	/// more than once by multiple threads, but only one of those values will successfully be written to the target.</param>
	/// <param name="state">An argument passed to the value factory.</param>
	/// <returns>The target value.</returns>
	public static T? EnsureInitialized<T, U>([NotNull] ref StrongBox<T?>? target, Func<U, T?> valueFactory, U state)

Conversation

sharwell commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

sharwell May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

sharwell commented May 11, 2023

Uh oh!

azure-pipelines bot commented May 11, 2023

Uh oh!

sharwell commented May 11, 2023

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!

sharwell May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

sharwell commented May 3, 2023 •

edited

Loading

sharwell May 4, 2023 •

edited

Loading

sharwell May 12, 2023 •

edited

Loading

sharwell May 12, 2023 •

edited

Loading