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.

Sign up for GitHub

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

Jump to bottom

Cache the MEF composition in the Roslyn LSP. #76276

Merged
merged 7 commits into from
Dec 10, 2024
Merged

Cache the MEF composition in the Roslyn LSP. #76276

merged 7 commits into from
Dec 10, 2024

Conversation

JoeRobich
Copy link
Member

To improve startup time of the LSP we can cache the MEF composition and load it from file instead of building up a new composition.

The cache is invalidated when:

  1. The location of the Microsoft.CodeAnalysis.LanguageServer changes (such as installing an updated C# extension or dotnet.server.path is updated).
  2. Changing major .NET runtime version.
  3. The set of assembly paths that make up the composition changes.
  4. The last write time of any assembly that is part of the composition changes.

Resolves #68568

@JoeRobich JoeRobich requested a review from dibarbet December 5, 2024 08:02
@JoeRobich JoeRobich requested a review from a team as a code owner December 5, 2024 08:02
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 5, 2024
@JoeRobich JoeRobich added Area-IDE LSP issues related to the roslyn language server protocol implementation and removed Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 5, 2024
@JoeRobich JoeRobich force-pushed the dev/jorobich/cache-lsp-mef branch from 312cd6b to 6b38161 Compare December 5, 2024 08:27
@JoeRobich JoeRobich force-pushed the dev/jorobich/cache-lsp-mef branch from 6b38161 to 2e9a0e7 Compare December 5, 2024 08:42
jasonmalinowski
jasonmalinowski reviewed Dec 5, 2024
View reviewed changes
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/ExportProviderBuilder.cs
{
// Include the .NET runtime version in the cache path so that running on a newer
// runtime causes the cache to be rebuilt.
var cacheSubdirectory = $".NET {Environment.Version.Major}";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that Version.Major mean a minor update won't create a new cache? Is that a problem?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure. I took this bit from DevKit and have updated the comment to match theirs. I know we have changed target runtime from 8.0.9 to 8.0.10 and now 8.0.11 seemingly without incident.

@JoeRobich
Copy link
Member Author

@ToddGrun @dibarbet Ready for another review.

dibarbet
dibarbet approved these changes Dec 6, 2024
View reviewed changes
Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally looks good - assuming the telemetry is fixed/removed and the async call to save the cache is tracked

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/ExportProviderBuilder.cs Outdated Show resolved Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/ExportProviderBuilder.cs Outdated Show resolved Hide resolved
@build-analysis build-analysis bot mentioned this pull request Dec 6, 2024
Open
3 tasks
ToddGrun
ToddGrun reviewed Dec 7, 2024
View reviewed changes
...LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/ExportProviderBuilderTests.cs Outdated
{
await using var testServer = await CreateLanguageServerAsync(includeDevKitComponents: false);

var mefCompositions = Directory.EnumerateFiles(MefCacheDirectory.Path, "*.mef-composition", SearchOption.AllDirectories);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var mefCompositions = Directory.EnumerateFiles(MefCacheDirectory.Path, "*.mef-composition", SearchOption.AllDirectories);

Sorry, I'm probably being dense here. Since the saving is asynchronous, how do you know that it's completed before we check for it here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, thanks. Will fix up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ToddGrun Updated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, I thought the IAsynchronousOperationListener was the way that we usually exposed async operations to tests. Was that not an option?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will admit I have not tested a lot of these async operations. Looking through the source it seems the IAsynchronousOperationListenerProvider is typically MEF imported but this code is building the MEF composition, so we couldn't follow that pattern. Nothing is stopping us from creating our own listener instance and making it available through the TestAccessor, but I am not sure that has any benefits over using a Task. If you know of any reasons, I am happy to rework this bit in a follow up.

@JoeRobich JoeRobich merged commit 7997469 into main Dec 10, 2024
25 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Dec 10, 2024
This was referenced Dec 12, 2024
Closed
Closed
Closed
@JoeRobich JoeRobich deleted the dev/jorobich/cache-lsp-mef branch December 20, 2024 18:57
@dibarbet dibarbet modified the milestones: Next, 17.13 P3 Jan 7, 2025
nickodei
nickodei reviewed Jan 23, 2025
View reviewed changes
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs
@@ -86,7 +86,9 @@ static async Task RunAsync(ServerConfiguration serverConfiguration, Cancellation
var assemblyLoader = new CustomExportAssemblyLoader(extensionManager, loggerFactory);
var typeRefResolver = new ExtensionTypeRefResolver(assemblyLoader, loggerFactory);

using var exportProvider = await ExportProviderBuilder.CreateExportProviderAsync(extensionManager, assemblyLoader, serverConfiguration.DevKitDependencyPath, loggerFactory);
var cacheDirectory = Path.Combine(Path.GetDirectoryName(typeof(Program).Assembly.Location)!, "cache");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I just wanted to mention that this breaks read-only filesystem systems like NixOS where this is throwing and Exception because it cant create the folder in the nix-store. It's a bit unfortunate that this path could not be configured:

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nickodei! I opened #76892 to track this.

@JoeRobich JoeRobich mentioned this pull request Jan 23, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonmalinowski jasonmalinowski jasonmalinowski left review comments

@ToddGrun ToddGrun ToddGrun left review comments

@nickodei nickodei nickodei left review comments

@dibarbet dibarbet dibarbet approved these changes

Assignees
No one assigned
Labels
Area-IDE LSP issues related to the roslyn language server protocol implementation VSCode
Projects
None yet
Milestone
17.13 P3
Development

Successfully merging this pull request may close these issues.

Cache vs-mef composition to improve startup perf
5 participants
@JoeRobich @jasonmalinowski @dibarbet @ToddGrun @nickodei