Changed LSP completion resolve to not cache data on items. #52123

NTaylorMullen · 2021-03-24T21:13:48Z

We used to cache displayText, text document, position and completion triggers on the data field of a completion item however in practice this resulted in having a pretty massive footprint on how completion lists serialized/deserialized.
- Because completion resolve used to cache more data and now we don't I added a telemetry event to capture how often a cache miss actually happens
- As part of this I had to migrate the CompletionListCache to be synchronous so that we can synchronously resolve cache entries at completion resolve time.
Updated tests to reflect new resolve behavior. As part of doing this I had to expand the completion resolve tests to actually generate completion lists so caches were properly updated. Ultimately this more accurately reflects what happens in real scenarios.
Wanted to use records so added an IsExternalInit.cs file to enable record usage in netstandard2.0.

Benchmark results

Scenario: Typing @ in a Razor file and getting a basic C# completion list.

Optimized = true represents utilizing the Optimized completion list (which Roslyn does now) and unoptimized represents anyone who's consuming Roslyn's completion list who doesn't use the optimized completion list structure.

Before (817KB)

Method	Optimized	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
'Completion List Roundtrip Serialization/Deserialization'	False	29.301 ms	0.5816 ms	0.6698 ms	34.13	1718.7500	937.5000	250.0000	9.8 MB
'Completion List Serialization'	False	10.223 ms	0.1926 ms	0.1608 ms	97.82	625.0000	250.0000	234.3750	3.27 MB
'Completion List Deserialization'	False	18.653 ms	0.3708 ms	0.5882 ms	53.61	1062.5000	531.2500	-	6.52 MB
'Completion List Roundtrip Serialization/Deserialization'	True	19.509 ms	0.3308 ms	0.3094 ms	51.26	1218.7500	718.7500	218.7500	7.03 MB
'Completion List Serialization'	True	2.121 ms	0.0259 ms	0.0216 ms	471.46	199.2188	167.9688	164.0625	1.1 MB
'Completion List Deserialization'	True	18.869 ms	0.3342 ms	0.3849 ms	53.00	1062.5000	531.2500	-	6.52 MB

After (458KB)

Method	Optimized	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
'Completion List Roundtrip Serialization/Deserialization'	False	19.438 ms	0.3835 ms	0.3400 ms	51.45	1093.7500	406.2500	125.0000	7216.06 KB
'Completion List Serialization'	False	8.355 ms	0.0735 ms	0.0651 ms	119.68	437.5000	171.8750	125.0000	2839.71 KB
'Completion List Deserialization'	False	10.303 ms	0.2045 ms	0.5701 ms	97.06	625.0000	312.5000	-	3844.78 KB
'Completion List Roundtrip Serialization/Deserialization'	True	10.847 ms	0.2057 ms	0.2021 ms	92.20	656.2500	234.3750	109.3750	4458.84 KB
'Completion List Serialization'	True	1.163 ms	0.0064 ms	0.0060 ms	860.16	123.0469	123.0469	123.0469	614.06 KB
'Completion List Deserialization'	True	10.103 ms	0.2004 ms	0.3909 ms	98.98	625.0000	312.5000	-	3844.78 KB

Results (Optimized = true)

Roundtrip Improvement: 1.8x
Serialization Improvement: 1.82x
Deserialization Improvement: 1.87x

Part of dotnet/aspnetcore#30232

- We used to cache displayText, text document, position and completion triggers on the data field of a completion item however in practice this resulted in having a pretty massive footprint on how completion lists serialized/deserialized. - Because completion resolve used to cache more data and now we don't I added a telemetry event to capture how often a cache miss actually happens - As part of this I had to migrate the `CompletionListCache` to be synchronous so that we can synchronously resolve cache entries at completion resolve time. - Updated tests to reflect new resolve behavior. As part of doing this I had to expand the completion resolve tests to actually generate completion lists so caches were properly updated. Ultimately this more accurately reflects what happens in real scenarios. - Wanted to use records so added an `IsExternalInit.cs` file to enable record usage in netstandard2.0. ## Benchmark results **Scenario:** Typing `@` in a Razor file and getting a basic C# completion list. Optimized = true represents utilizing the Optimized completion list (which Roslyn does now) and unoptimized represents anyone who's consuming Roslyn's completion list who doesn't use the optimized completion list structure. ### Before (817KB) | Method | Optimized | Mean | Error | StdDev | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated | |---------------------------------------------------------- |---------- |----------:|----------:|----------:|-------:|----------:|---------:|---------:|----------:| | 'Completion List Roundtrip Serialization/Deserialization' | False | 29.301 ms | 0.5816 ms | 0.6698 ms | 34.13 | 1718.7500 | 937.5000 | 250.0000 | 9.8 MB | | 'Completion List Serialization' | False | 10.223 ms | 0.1926 ms | 0.1608 ms | 97.82 | 625.0000 | 250.0000 | 234.3750 | 3.27 MB | | 'Completion List Deserialization' | False | 18.653 ms | 0.3708 ms | 0.5882 ms | 53.61 | 1062.5000 | 531.2500 | - | 6.52 MB | | 'Completion List Roundtrip Serialization/Deserialization' | True | 19.509 ms | 0.3308 ms | 0.3094 ms | 51.26 | 1218.7500 | 718.7500 | 218.7500 | 7.03 MB | | 'Completion List Serialization' | True | 2.121 ms | 0.0259 ms | 0.0216 ms | 471.46 | 199.2188 | 167.9688 | 164.0625 | 1.1 MB | | 'Completion List Deserialization' | True | 18.869 ms | 0.3342 ms | 0.3849 ms | 53.00 | 1062.5000 | 531.2500 | - | 6.52 MB | ### After (458KB) | Method | Optimized | Mean | Error | StdDev | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated | |---------------------------------------------------------- |---------- |----------:|----------:|----------:|-------:|----------:|---------:|---------:|-----------:| | 'Completion List Roundtrip Serialization/Deserialization' | False | 19.438 ms | 0.3835 ms | 0.3400 ms | 51.45 | 1093.7500 | 406.2500 | 125.0000 | 7216.06 KB | | 'Completion List Serialization' | False | 8.355 ms | 0.0735 ms | 0.0651 ms | 119.68 | 437.5000 | 171.8750 | 125.0000 | 2839.71 KB | | 'Completion List Deserialization' | False | 10.303 ms | 0.2045 ms | 0.5701 ms | 97.06 | 625.0000 | 312.5000 | - | 3844.78 KB | | 'Completion List Roundtrip Serialization/Deserialization' | True | 10.847 ms | 0.2057 ms | 0.2021 ms | 92.20 | 656.2500 | 234.3750 | 109.3750 | 4458.84 KB | | 'Completion List Serialization' | True | 1.163 ms | 0.0064 ms | 0.0060 ms | 860.16 | 123.0469 | 123.0469 | 123.0469 | 614.06 KB | | 'Completion List Deserialization' | True | 10.103 ms | 0.2004 ms | 0.3909 ms | 98.98 | 625.0000 | 312.5000 | - | 3844.78 KB | ### Results (Optimized = true) **Roundtrip Improvement:** 1.8x **Serialization Improvement:** 1.82x **Deserialization Improvement:** 1.87x Part of dotnet/aspnetcore#30232

src/Features/LanguageServer/Protocol/Handler/Completion/CompletionResolveData.cs

src/Features/LanguageServer/Protocol/Handler/Completion/CompletionResolveHandler.cs

dibarbet · 2021-03-24T21:34:49Z

src/Features/LanguageServer/Protocol/IsExternalInit.cs

+namespace System.Runtime.CompilerServices
+{
+    // Used to compile against C# 9 in a netstandard2.0
+    internal class IsExternalInit


@sharwell do we already have this somewhere / should this be moved somewhere else?

src/Features/LanguageServer/ProtocolUnitTests/Completion/CompletionResolveTests.cs

src/Features/LanguageServer/Protocol/Handler/Completion/CompletionResolveHandler.cs

allisonchou

lgtm!

src/Features/LanguageServer/Protocol/Handler/Completion/CompletionResolveHandler.cs

src/Features/LanguageServer/Protocol/Handler/Completion/CompletionResolveData.cs

…tionResolveData.cs Co-authored-by: David Barbet <dabarbet@microsoft.com>

ghost

Auto-approval

NTaylorMullen requested a review from a team as a code owner March 24, 2021 21:13

dotnet-issue-labeler bot added the Area-IDE label Mar 24, 2021

NTaylorMullen requested review from dibarbet and allisonchou March 24, 2021 21:14

dibarbet reviewed Mar 24, 2021

View reviewed changes

allisonchou approved these changes Mar 24, 2021

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/Completion/CompletionResolveHandler.cs Outdated Show resolved Hide resolved

Address code review comments.

e3a0414

NTaylorMullen requested a review from dibarbet March 25, 2021 21:30

dibarbet approved these changes Mar 25, 2021

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/Completion/CompletionResolveData.cs Outdated Show resolved Hide resolved

Update src/Features/LanguageServer/Protocol/Handler/Completion/Comple…

05998c0

…tionResolveData.cs Co-authored-by: David Barbet <dabarbet@microsoft.com>

NTaylorMullen added the auto-merge label Mar 25, 2021

ghost approved these changes Mar 25, 2021

View reviewed changes

ghost merged commit c344abc into main Mar 26, 2021

ghost deleted the nimullen/razor.30232.resolve branch March 26, 2021 19:27

ghost added this to the Next milestone Mar 26, 2021

allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021

allisonchou mentioned this pull request Mar 30, 2021

[LSP] Make cache handlers synchronous #50418

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed LSP completion resolve to not cache data on items. #52123

Changed LSP completion resolve to not cache data on items. #52123

NTaylorMullen commented Mar 24, 2021

dibarbet Mar 24, 2021

allisonchou left a comment

ghost left a comment

Changed LSP completion resolve to not cache data on items. #52123

Changed LSP completion resolve to not cache data on items. #52123

Conversation

NTaylorMullen commented Mar 24, 2021

Benchmark results

Before (817KB)

After (458KB)

Results (Optimized = true)

dibarbet Mar 24, 2021

Choose a reason for hiding this comment

allisonchou left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment