Run HasRefactoringsAsync out-of-process by sharwell · Pull Request #40672 · dotnet/roslyn

sharwell · 2019-12-31T16:20:24Z

Marked as a draft until we verify that it works for both built-in refactorings and refactorings provided by a VSIX package.

This change means refactorings no longer have access to Visual Studio services, but our options are somewhat limited. The infrastructure to execute HasRefactoringsAsync cannot be executed in the limited process space of devenv.exe without risking OOM crashes in large sources. Moving out-of-process means the 64-bit address space can be leveraged when necessary.

We could provide a hook for individual refactorings to run in process, but it will have a tremendous negative performance impact on those refactorings (they will no longer share a semantic model with other refactorings running on the same compilation, so moving a refactoring in-process means the entire burden of parsing+binding+analysis will fall on the refactoring that chose to run in process).

Refactorings that require Visual Studio services always have the option of interacting directly with the Light Bulb APIs instead of operating as a CodeRefactoringProvider.

sharwell · 2020-04-15T04:19:33Z

@tmat @CyrusNajmabadi This is the change I was mentioning in our meeting last week

src/Features/Core/Portable/CodeRefactorings/CodeRefactoringService.cs

src/Workspaces/Remote/Core/Services/RemotePasteTrackingService.cs

src/Workspaces/Remote/ServiceHub/Services/CodeAnalysisService_CodeRefactoring.cs

CyrusNajmabadi · 2020-04-15T07:05:40Z

Overall 1000% for this. But i think we can simplify in a very easy way.

src/EditorFeatures/Core.Wpf/Suggestions/SuggestedActionsSource.cs

src/Workspaces/Remote/ServiceHub/Services/CodeAnalysisService_CodeRefactoring.cs

CyrusNajmabadi · 2020-05-06T18:57:14Z

src/EditorFeatures/Core.Wpf/Suggestions/SuggestedActionsSource.cs

+                var pastedTextSpanOpt =
+                    provider._pasteTrackingService.TryGetPastedTextSpan(sourceText.Container, out var pastedTextSpan)
+                    ? (TextSpan?)pastedTextSpan
+                    : null;


Suggested change

var pastedTextSpanOpt =

provider._pasteTrackingService.TryGetPastedTextSpan(sourceText.Container, out var pastedTextSpan)

? (TextSpan?)pastedTextSpan

: null;

var pastedTextSpanOpt = provider._pasteTrackingService.TryGetPastedTextSpan(sourceText.Container, out var pastedTextSpan)

? (TextSpan?)pastedTextSpan

: null;

The problem with this style is how difficult it is to distinguish the parent/child relationship. For exaple, this would be even more difficult to understand:

Suggested change

var pastedTextSpanOpt =

provider._pasteTrackingService.TryGetPastedTextSpan(sourceText.Container, out var pastedTextSpan)

? (TextSpan?)pastedTextSpan

: null;

var pastedTextSpanOpt = provider._pasteTrackingService.TryGetPastedTextSpan(sourceText.Container, out var pastedTextSpan)

? another.complex.expr()

? and.yet.another()

? baz

: quuz

: bar

: null;

As opposed to:

Suggested change

var pastedTextSpanOpt =

provider._pasteTrackingService.TryGetPastedTextSpan(sourceText.Container, out var pastedTextSpan)

? (TextSpan?)pastedTextSpan

: null;

var pastedTextSpanOpt = provider._pasteTrackingService.TryGetPastedTextSpan(sourceText.Container, out var pastedTextSpan)

? another.complex.expr()

? and.yet.another()

? baz

: quuz

: bar

: null;

Now i can very easily see what children are actually siblings, and i can easily tell the parent/child relationship.

i get that you continually push this strict adherence to indentation. But it's not going to fly with me. Custom indentation serves a valuable purpose and we do judiciously use it across the IDE codebase where appropriate because it really helps with clarity.

This is all covered by #38255 (comment). The form I used is correct (covered by 1(ii)), as are the first and third examples here (covered by 1(iii)).

i don't actually see the value of 1ii. in particular it seems to entirely not what i'd want for indenting whenever the expression itself starts on a new line. i.e. on a new line you'll always get:

.... startOfExpr ? whenTrue : whenFalse

Which never seems desirable. This also doesn't really match teh examples shown in that linked post, none of which show the whenTrue/whenFalse parts aligning with the condition.

sharwell · 2020-05-06T19:03:47Z

src/Workspaces/Remote/Core/Services/RemotePasteTrackingService.cs

+                    // Cannot track two different pasted spans for the same container
+                    throw ExceptionUtilities.UnexpectedValue(pastedTextSpan);


@CyrusNajmabadi The other option here is to assign null (no pasted text span) if multiple callers cannot agree on the same non-null span. As it stands, code that attempts to use a different span will fail if prior work has not yet completed.

CyrusNajmabadi · 2020-05-07T01:23:30Z

src/Features/Core/Portable/CodeRefactorings/CodeRefactoringService.cs

+                    return textSpan;
+
+                return null;
+            }


this is a bit concerning that we're exposing a synchronous method over to the OOP side. in general we don't do that. can this be made async?

CyrusNajmabadi · 2020-05-07T01:24:35Z

src/Workspaces/Remote/ServiceHub/Services/CodeAnalysisService_CodeRefactoring.cs

+                    var pasteTrackingService = (RemotePasteTrackingService?)mefHostExportProvider.GetExports<IPasteTrackingService>().SingleOrDefault()?.Value;
+                    var sourceText = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);
+                    var container = sourceText.Container;
+                    using var _ = pasteTrackingService?.SetPastedTextSpanForRemoteCall(container, pastedTextSpan);


is the ?. needed? can it actually fail to get the IPasteTrackingService?

CyrusNajmabadi · 2020-05-07T01:25:09Z

src/Features/Core/Portable/CodeRefactorings/CodeRefactoringService.cs

+                        textSpan,
+                        pastedTextSpan,
+                    },
+                    callbackTarget: new CodeRefactoringServiceCallback(this),


tbh.... i don't get what this callback is for. i don't eally see anyone consuming it on the remote side. could you clarify?

CyrusNajmabadi · 2020-05-07T01:28:36Z

src/Workspaces/Remote/Core/Services/RemotePasteTrackingService.cs

+            lock (_trackedSpans)
+            {
+                var result = _trackedSpans.TryGetValue(sourceTextContainer, out var spanAndCount);
+                if (!result || spanAndCount.referenceCount == 0 || spanAndCount.span is null)


the .referenceCount == 0 case seems impossible. wlud prefer we throw in that case vs bailing.

Will add an assertion

CyrusNajmabadi · 2020-05-07T01:29:13Z

src/Workspaces/Remote/Core/Services/RemotePasteTrackingService.cs

+                    throw ExceptionUtilities.UnexpectedValue(container);
+
+                Debug.Assert(spanAndCount.referenceCount > 0);
+                if (spanAndCount.referenceCount <= 1)


the < case seems imposible. would prefer we throw in that case.

CyrusNajmabadi

Overall looking good. One thing i didn't understand, and a few places i think coudl clean up.

CyrusNajmabadi · 2021-11-17T01:29:18Z

src/EditorFeatures/Core.Wpf/Suggestions/SuggestedActionsSource.cs

                {
                    if (await state.Target.Owner._codeRefactoringService.HasRefactoringsAsync(
-                            document, selection.Value, cancellationToken).ConfigureAwait(false))
+                            document, selection.Value, pastedTextSpanOpt, cancellationToken).ConfigureAwait(false))


so, generally, we've been trying to move to the model more where we have 'args' that can get passed along with the op, to avoid the 'just jam a ton of individual parameters along' . Just food for thought.

CyrusNajmabadi · 2021-11-17T01:30:15Z

src/Features/Core/Portable/CodeRefactorings/CodeRefactoringService.cs

+                if (result.HasValue)
+                {
+                    return result.Value;
+                }


i believe we're no longer following this pattern. if the operation fails, we do not try to fallback to local processing. instead, we jsut bail out (As we will also have had a yellowbar here). note we may not be 100% consistent here, but i believe that Tomas' strong position on what this ll needs to be.

CyrusNajmabadi · 2021-11-17T01:30:29Z

src/Workspaces/Remote/ServiceHub/Host/RemotePasteTrackingService.cs

+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+#nullable enable


CyrusNajmabadi · 2021-11-17T01:31:27Z

src/Workspaces/Remote/ServiceHub/Host/RemotePasteTrackingService.cs

+            return new Releaser(this, container);
+        }
+
+        internal readonly struct Releaser : IDisposable


nit: my preference is public on all nested symbols that are made 'internal' by virtue of their container.

CyrusNajmabadi · 2021-11-17T01:33:36Z

src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/RemoteDiagnosticAnalyzerService.cs

            }, cancellationToken);
        }
+
+        public ValueTask<bool> HasRefactoringsAsync(PinnedSolutionInfo solutionInfo, DocumentId documentId, TextSpan textSpan, TextSpan? pastedTextSpan, CancellationToken cancellationToken)


there's a potential future optimizatino we can do/track if we're willing to make the statement that a refactoring should be computable without knowing about projects that depend on. Specifically, OOP supports the idea of quickly syncing only a project (and the projects it depends on). we could use that to more quickly sync and find out if there are refactorings. i believe all our refactorings would satisfy this. but some third party ones might not.

CyrusNajmabadi · 2021-11-17T01:35:13Z

i have a general design concern here given that we run HasRefactorings OOP, but GetRefactorign in prc. It means that we cannot leverage all the work that HasRefactorings deos (in terms of warming up compilations, etc.) that happen externally. THis risks paying the same price again when we now actually do the work in proc to get the refactorings.

That said, maybe it's ok. an A/B test with code markers would help here.

Run HasRefactoringsAsync out-of-process

2f42ff8

jinujoseph added the Area-IDE label Jan 5, 2020

Merge remote-tracking branch 'dotnet/master' into oop-refactoring

71bb4e1

sharwell force-pushed the oop-refactoring branch from 4e3bebe to 71bb4e1 Compare April 15, 2020 04:19

CyrusNajmabadi reviewed Apr 15, 2020

View reviewed changes

src/Features/Core/Portable/CodeRefactorings/CodeRefactoringService.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Apr 15, 2020

View reviewed changes

src/Workspaces/Remote/Core/Services/RemotePasteTrackingService.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Apr 15, 2020

View reviewed changes

src/Workspaces/Remote/ServiceHub/Services/CodeAnalysisService_CodeRefactoring.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed May 6, 2020

View reviewed changes

src/EditorFeatures/Core.Wpf/Suggestions/SuggestedActionsSource.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed May 6, 2020

View reviewed changes

src/Workspaces/Remote/ServiceHub/Services/CodeAnalysisService_CodeRefactoring.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed May 6, 2020

View reviewed changes

src/Workspaces/Remote/ServiceHub/Services/CodeAnalysisService_CodeRefactoring.cs Outdated Show resolved Hide resolved

CyrusNajmabadi approved these changes May 6, 2020

View reviewed changes

Simplify the remote paste tracking service

b3fc97c

sharwell force-pushed the oop-refactoring branch from a7a21d0 to b3fc97c Compare May 6, 2020 18:30

CyrusNajmabadi reviewed May 6, 2020

View reviewed changes

Simplify code setting pasted text span for remote calls

4385ecd

sharwell commented May 6, 2020

View reviewed changes

sharwell marked this pull request as ready for review May 7, 2020 00:09

sharwell requested a review from a team as a code owner May 7, 2020 00:09

sharwell requested a review from a team May 7, 2020 00:09

CyrusNajmabadi reviewed May 7, 2020

View reviewed changes

CyrusNajmabadi suggested changes May 7, 2020

View reviewed changes

Base automatically changed from master to main March 3, 2021 23:52

Merge remote-tracking branch 'dotnet/main' into oop-refactoring

0afd6ff

CyrusNajmabadi reviewed Nov 17, 2021

View reviewed changes

runfoapp bot mentioned this pull request Nov 17, 2021

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

sharwell mentioned this pull request Feb 17, 2022

Use the same codepath for computing light-bulb actions to determine if hte lightbulb shoudl appear at all. #59599

Merged

		// Cannot track two different pasted spans for the same container
		throw ExceptionUtilities.UnexpectedValue(pastedTextSpan);

Conversation

sharwell commented Dec 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell commented Apr 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Apr 15, 2020

Uh oh!

Uh oh!

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!

CyrusNajmabadi 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!

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 commented Nov 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sharwell commented Dec 31, 2019 •

edited

Loading