Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework completion resolution #2126

Merged
merged 8 commits into from Apr 8, 2021
Merged

Conversation

@333fred
Copy link
Contributor

@333fred 333fred commented Apr 2, 2021

Recently, Roslyn undeprecated the CompletionChange.TextChanges API. This API gives changes in a significantly simpler manner for us to deal with, as it splits up things like imports and the actual main change element, so we can remove a whole bunch of code dedicated to finding the changed elements and mapping original source locations to the new document. The new pattern is much simpler: except for import completion, we always resolve the change up front. For most providers, this is an extremely quick call, as most providers just return the label, and for the ones that don't we can't do much about it anyway. We can then loop through the individual changes, putting changes that are not touching the current cursor location into AdditionalTextChanges. This shrinks the completion payload pretty significantly for many scenarios, and gets rid of a bunch of special handling around it. The only remaining special handling is adjusting the filter texts and snippitizing completions that want to move the cursor. I've also aligned the behavior of the Preselect flag with Roslyn's completion handler, and removed additional filtering of completion items so that they can be filtered by the client instead.

Fixes #2123 as well.

Recently, Roslyn undeprecated the CompletionChange.TextChanges API. This API gives changes in a significantly simpler manner for us to deal with, as it splits up things like imports and the actual main change element, so we can remove a whole bunch of code dedicated to finding the changed elements and mapping original source locations to the new document. The new pattern is much simpler: except for import completion, we always resolve the change up front. For most providers, this is an extremely quick call, as most providers just return the label, and for the ones that don't we can't do much about it anyway. We can then loop through the individual changes, putting changes that are not touching the current cursor location into AdditionalTextChanges. This shrinks the completion payload pretty significantly for many scenarios, and gets rid of a bunch of special handling around it. The only remaining special handling is adjusting the filter texts and snippitizing completions that want to move the cursor. I've also aligned the behavior of the Preselect flag with Roslyn's completion handler, and removed additional filtering of completion items so that they can be filtered by the client instead.

Fixes #2123 as well.
@@ -509,86 +455,17 @@ public async Task<CompletionResolveResponse> Handle(CompletionResolveRequest req
};
}

private (IReadOnlyList<LinePositionSpanTextChange>? edits, int endOffset) GetAdditionalTextEdits(

This comment has been minimized.

@333fred

333fred Apr 2, 2021
Author Contributor

Man am I happy to delete this method :).

Assert.False(c.Preselect);
break;
}
if (c.Label == "ToString")

This comment has been minimized.

@333fred

333fred Apr 2, 2021
Author Contributor

I honestly can't tell you why Roslyn wants to preselect this, but it does. The behavior in VSCode is that ToString is filtered out because of the prefix mismatch.

@@ -270,12 +270,34 @@ public static void Test(this object o)

Assert.Single(resolved.Item.AdditionalTextEdits);
var additionalEdit = resolved.Item.AdditionalTextEdits[0];
Assert.Equal(NormalizeNewlines("using N2;\n\nnamespace N1\r\n{\r\n public class C1\r\n {\r\n public void M(object o)\r\n {\r\n o"),
Assert.Equal(NormalizeNewlines("using N2;\n\n"),

This comment has been minimized.

@333fred

333fred Apr 2, 2021
Author Contributor

@bjorkstromm @NTaylorMullen this change will, I think, make import completion easier to support from Cake/Razor, as we're not sending the whole document anymore. Just the import changes.

[Theory]
[InlineData("dummy.cs")]
[InlineData("dummy.csx")]
public async Task ImportCompletion_OnLine0(string filename)

This comment has been minimized.

@filipw

filipw Apr 2, 2021
Member

thanks for fixing this!

{
// Except for import completion, we just resolve the change up front in the sync version. It's only expensive
// for override completion, but there's not a heck of a lot we can do about that for the sync scenario
var change = await completionService.GetChangeAsync(document, completion);

This comment has been minimized.

@filipw

filipw Apr 2, 2021
Member

does it now make sense to update the client to not call resolve on the server for each item except in specific cases?

This comment has been minimized.

@333fred

333fred Apr 2, 2021
Author Contributor

No, because resolve will still fill in documentation for all items.

This comment has been minimized.

@david-driscoll

david-driscoll Apr 2, 2021
Member

Any benefit (or problems!) with using Task.WhenAll to group all the async operations. Not that we were doing this before anyway.

This comment has been minimized.

@333fred

333fred Apr 4, 2021
Author Contributor

I'd need to get a real benchmark setup to determine this. A quick test didn't show any obvious impact either way.

This comment has been minimized.

@333fred

333fred Apr 5, 2021
Author Contributor

I've created a benchmark project using benchmarkdotnet and included in this PR. Some statistics from running it with and without Task.WhenAll:

Method Mean Error StdDev
ImportCompletionListAsyncOriginal 3.715 ms 0.0203 ms 0.0190 ms
ImportCompletionListAsyncWhenAll 4.232 ms 0.0200 ms 0.0178 ms
Method NumOverrides Mean Error StdDev Median
OverrideCompletionAsyncOriginal 10 11.54 ms 0.051 ms 0.045 ms 11.53 ms
OverrideCompletionAsyncOriginal 100 34.25 ms 0.681 ms 1.852 ms 35.29 ms
OverrideCompletionAsyncOriginal 250 65.92 ms 1.312 ms 3.194 ms 67.20 ms
OverrideCompletionAsyncOriginal 500 111.92 ms 0.566 ms 0.529 ms 111.89 ms
OverrideCompletionAsyncWhenAll 10 6.769 ms 0.1011 ms 0.0945 ms
OverrideCompletionAsyncWhenAll 100 16.147 ms 0.3139 ms 0.2936 ms
OverrideCompletionAsyncWhenAll 250 28.739 ms 0.4544 ms 0.3547 ms
OverrideCompletionAsyncWhenAll 500 73.618 ms 1.4516 ms 1.7827 ms
@filipw
Copy link
Member

@filipw filipw commented Apr 2, 2021

looks very nice, thank you

@david-driscoll
Copy link
Member

@david-driscoll david-driscoll commented Apr 2, 2021

Timeout of 2400000ms hit 😱

Copy link
Member

@david-driscoll david-driscoll left a comment

Not sure what's causing the issues with GitHub Actions, but that's concerning at least because Azure Devops was just fine.

:shipit: !

{
// Except for import completion, we just resolve the change up front in the sync version. It's only expensive
// for override completion, but there's not a heck of a lot we can do about that for the sync scenario
var change = await completionService.GetChangeAsync(document, completion);

This comment has been minimized.

@david-driscoll

david-driscoll Apr 2, 2021
Member

Any benefit (or problems!) with using Task.WhenAll to group all the async operations. Not that we were doing this before anyway.

@333fred
Copy link
Contributor Author

@333fred 333fred commented Apr 2, 2021

I'll try to look into the timeouts sometime this weekend.

@david-driscoll
Copy link
Member

@david-driscoll david-driscoll commented Apr 2, 2021

Oh wait, azure devops doesn't run tests anymore, so never mind there may be something there. I'll try and run them on my machines later as well.

@333fred
Copy link
Contributor Author

@333fred 333fred commented Apr 4, 2021

I'll try to look into the timeouts sometime this weekend.

So, I have no idea why the tests failing is causing the github action runner to take so long (perhaps all the output?), but the main issue is that I forgot I cloned all the tests for the lsp handler when I updated LSP to use the new service, and they're of course now all failing.

@@ -112,7 +112,7 @@ private static void VerifyEnumsInSync(Type enum1, Type enum2)
Debug.Assert(lspValues.Length == modelValues.Length);
for (int i = 0; i < lspValues.Length; i++)
{
Debug.Assert((int?)lspValues.GetValue(i) == (int?)modelValues.GetValue(i));
Debug.Assert((int)lspValues.GetValue(i) == (int)modelValues.GetValue(i));

This comment has been minimized.

@filipw

filipw Apr 6, 2021
Member

I changed this to int? because I noticed on the .NET 5.0 branch that it reports CS8605 "Unboxing possibly null value" there - and since we have TreatWarningsAsErrors enabled, it wouldn't build

This comment has been minimized.

@333fred

333fred Apr 6, 2021
Author Contributor

Well, if it's int? the assert fails because these are ints, not nullable ints :)

This comment has been minimized.

@filipw

filipw Apr 8, 2021
Member

oh 😅

Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "OmniSharp.Lsp.Tests", "tests\OmniSharp.Lsp.Tests\OmniSharp.Lsp.Tests.csproj", "{D67AA10B-8DB6-408D-A4C5-0B1DDCF5B3CC}"
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OmniSharp.Lsp.Tests", "tests\OmniSharp.Lsp.Tests\OmniSharp.Lsp.Tests.csproj", "{D67AA10B-8DB6-408D-A4C5-0B1DDCF5B3CC}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OmniSharp.Benchmarks", "src\OmniSharp.Benchmarks\OmniSharp.Benchmarks.csproj", "{6F5B209E-8DD3-4E90-A1F3-D85E7AF56588}"

This comment has been minimized.

@filipw

filipw Apr 6, 2021
Member

thank you for adding this... It has been long long long overdue

@filipw filipw enabled auto-merge Apr 8, 2021
@filipw filipw merged commit 38dbed5 into OmniSharp:master Apr 8, 2021
16 checks passed
16 checks passed
@github-actions
Build (ubuntu-18.04)
Details
@github-actions
Test (ubuntu-18.04, OmniSharp.MSBuild.Tests,OmniSharp.Roslyn.CSharp.Tests,OmniSharp.Cake.Tests,Om... Test (ubuntu-18.04, OmniSharp.MSBuild.Tests,OmniSharp.Roslyn.CSharp.Tests,OmniSharp.Cake.Tests,Om...
Details
@github-actions
Build (windows-2019)
Details
@github-actions
Test (ubuntu-18.04, OmniSharp.DotNetTest.Tests) Test (ubuntu-18.04, OmniSharp.DotNetTest.Tests)
Details
@github-actions
Build (macos-10.15)
Details
@github-actions
Test (windows-2019, OmniSharp.MSBuild.Tests,OmniSharp.Roslyn.CSharp.Tests,OmniSharp.Cake.Tests,Om... Test (windows-2019, OmniSharp.MSBuild.Tests,OmniSharp.Roslyn.CSharp.Tests,OmniSharp.Cake.Tests,Om...
Details
@github-actions
Test (windows-2019, OmniSharp.DotNetTest.Tests) Test (windows-2019, OmniSharp.DotNetTest.Tests)
Details
@github-actions
Test (macos-10.15, OmniSharp.MSBuild.Tests,OmniSharp.Roslyn.CSharp.Tests,OmniSharp.Cake.Tests,Omn... Test (macos-10.15, OmniSharp.MSBuild.Tests,OmniSharp.Roslyn.CSharp.Tests,OmniSharp.Cake.Tests,Omn...
Details
@github-actions
Test (macos-10.15, OmniSharp.DotNetTest.Tests) Test (macos-10.15, OmniSharp.DotNetTest.Tests)
Details
@azure-pipelines
OmniSharp.omnisharp-roslyn Build #1.37.9-PullRequest2126.11 succeeded
Details
@azure-pipelines
OmniSharp.omnisharp-roslyn (GitVersion) GitVersion succeeded
Details
@azure-pipelines
OmniSharp.omnisharp-roslyn (Linux) Linux succeeded
Details
@azure-pipelines
OmniSharp.omnisharp-roslyn (Release) Release succeeded
Details
@azure-pipelines
OmniSharp.omnisharp-roslyn (Windows) Windows succeeded
Details
@azure-pipelines
OmniSharp.omnisharp-roslyn (macOS) macOS succeeded
Details
license/cla All CLA requirements met.
Details
@333fred 333fred deleted the 333fred:completion-improvements branch Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants