Switch to balanced source generator execution in VSCode with refresh support#82330
Switch to balanced source generator execution in VSCode with refresh support#82330dibarbet merged 2 commits intodotnet:mainfrom
Conversation
and add refresh support
dibarbet
force-pushed
the
balanced_vscode
branch
from
85ab061 to
176e003
Compare
| Description = "Controls when source generators are executed.", | ||
| Required = false, | ||
| // Balanced mode requires additional client side support (to trigger refreshes), so by default run in automatic to ensure tool scenarios without client support run generators. | ||
| DefaultValueFactory = _ => SourceGeneratorExecutionPreference.Automatic, |
There was a problem hiding this comment.
this seemed like a sane default if not provided to allow tools to work more easily. In VSCode we explicitly pass this and pick Balanced as the default there
| DefaultValueFactory = _ => false, | ||
| }; | ||
|
|
||
| var sourceGeneratorExecutionOption = new Option<SourceGeneratorExecutionPreference>("--sourceGeneratorExecutionPreference") |
There was a problem hiding this comment.
While we do have support live option changes, source generator execution preference does not support live modification, so this is passed in as a CLI arg.
dibarbet
force-pushed
the
balanced_vscode
branch
from
8f8bbe3 to
73a7d5f
Compare
| [Method(Methods.TextDocumentDidSaveName)] | ||
| [method: ImportingConstructor] | ||
| [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| internal class DidSaveHandler() : ILspServiceNotificationHandler<DidSaveTextDocumentParams>, ITextDocumentIdentifierHandler<DidSaveTextDocumentParams, TextDocumentIdentifier> |
| [Method(MethodName)] | ||
| [method: ImportingConstructor] | ||
| [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| internal class WorkspaceRefreshSourceGeneratorsHandler(LspWorkspaceRegistrationService workspaceRegistrationService) : ILspServiceNotificationHandler<RefreshSourceGeneratorsParams> |
| [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| internal class WorkspaceRefreshSourceGeneratorsHandler(LspWorkspaceRegistrationService workspaceRegistrationService) : ILspServiceNotificationHandler<RefreshSourceGeneratorsParams> | ||
| { | ||
| public const string MethodName = "workspace/_roslyn_refreshSourceGenerators"; |
There was a problem hiding this comment.
does this need to be public? (maybe it's tests using this. if so, that's fine).
| } | ||
|
|
||
| [Theory, CombinatorialData] | ||
| internal async Task TestSaveRefreshesSourceGenerators(bool mutatingLspWorkspace, SourceGeneratorExecutionPreference sourceGeneratorExecution) |
There was a problem hiding this comment.
odd that this is internal. but don't care.
| using Roslyn.Test.Utilities; | ||
| using Xunit; | ||
| using LSP = Roslyn.LanguageServer.Protocol; | ||
| using SumType = Roslyn.LanguageServer.Protocol.SumType<Roslyn.LanguageServer.Protocol.FullDocumentDiagnosticReport, Roslyn.LanguageServer.Protocol.UnchangedDocumentDiagnosticReport>; |
There was a problem hiding this comment.
nit. if you move aliases into the namespace, you cn then use your outer aliases like LSP.
|
|
||
| public LspSourceGeneratorBenchmarks() : base(null) | ||
| { | ||
| } |
There was a problem hiding this comment.
nit: primary constructor.
|
It might be good to look into, or consider a tracking issue, about triggering this on build, not just save. |
this is done already - https://github.com/dotnet/vscode-csharp/pull/8970/changes#diff-6b492de5f696ddcacaa4b8d6d23e6ac05715105d61be575e464f279fe01ca6c5R39 |
4 participants
Client side PR: dotnet/vscode-csharp#8970
In VS we switched to balanced source gen execution a while back (#73618) to avoid perf issues with running generators on every keystroke. This ports the same change to VSCode.
Also added a small benchmark to compare perf between balanced and automatic. Balanced shows an improvement in cpu and allocations, even with an extremely simple generator.