Cache MEF catalog in servicehub process #78122
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
Area-IDE
untriaged
dibarbet
reviewed
Apr 11, 2025
dibarbet
reviewed
Apr 11, 2025
2) Create arguments record instead of adding more parameters to various methods
JoeRobich
approved these changes
Apr 11, 2025
ToddGrun
changed the title
dibarbet
approved these changes
Apr 11, 2025
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServerExportProviderBuilder.cs
Outdated
Show resolved
Hide resolved
switch to record struct for args encapsulation
2) Add new expected error'ed type in CA process MEF composition
ToddGrun
commented
Apr 15, 2025
| Resolver: new Resolver(SimpleAssemblyLoader.Instance), | ||
| CacheDirectory: Path.Combine(localSettingsDirectory, "Roslyn", "RemoteHost", "Cache"), | ||
| CatalogPrefix: "RoslynRemoteHost", | ||
| ExpectedErrorParts: ["PythiaSignatureHelpProvider", "VSTypeScriptAnalyzerService", "RazorTestLanguageServerFactory", "CodeFixService"], |
There was a problem hiding this comment.
@CyrusNajmabadi -- any idea why this type has started not resolving in the CA process MEF composition?
There was a problem hiding this comment.
Is that some fallout of the recent removal of EditorFeatures? That was rolled back, but it's still suspicious.
There was a problem hiding this comment.
I merged main and this type still errors.
Microsoft.CodeAnalysis.CodeFixes.CodeFixService.ctor(diagnosticAnalyzerService): expected exactly 1 export matching constraints:
Contract name: Microsoft.CodeAnalysis.Diagnostics.IDiagnosticAnalyzerService
TypeIdentityName: Microsoft.CodeAnalysis.Diagnostics.IDiagnosticAnalyzerService
but found 0.
There was a problem hiding this comment.
my pr hasn't merged in yet.
There was a problem hiding this comment.
OK, I tested with both our branches combined and it gets rid of the errors for VSTypeScriptAnalyzerService and CodeFixService
There was a problem hiding this comment.
Here are the errors for those two:
Microsoft.CodeAnalysis.ExternalAccess.Razor.RazorTestLanguageServerFactory.ctor(languageServerFactory): expected exactly 1 export matching constraints:
Contract name: Microsoft.CodeAnalysis.LanguageServer.ILanguageServerFactory
TypeIdentityName: Microsoft.CodeAnalysis.LanguageServer.ILanguageServerFactory
but found 0.
Microsoft.CodeAnalysis.ExternalAccess.Pythia.PythiaSignatureHelpProvider.ctor(implementation): expected exactly 1 export matching constraints:
Contract name: Microsoft.CodeAnalysis.ExternalAccess.Pythia.Api.IPythiaSignatureHelpProviderImplementation
TypeIdentityName: Microsoft.CodeAnalysis.ExternalAccess.Pythia.Api.IPythiaSignatureHelpProviderImplementation
but found 0.
@jasonmalinowski -- ptal |
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServerExportProviderBuilder.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServerExportProviderBuilder.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServerExportProviderBuilder.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServerExportProviderBuilder.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Services/ExtensionAssemblyManager.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Remote/VisualStudioRemoteHostClientProvider.cs
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Telemetry/IRemoteProcessTelemetryService.cs
Show resolved
Hide resolved
|
Test insertion doesn't show any regressions: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/629927 |
2) Try to not load assemblies during when mef cache is used
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServerExportProviderBuilder.cs
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServerExportProviderBuilder.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServerExportProviderBuilder.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServerExportProviderBuilder.cs
Show resolved
Hide resolved
2) Add comment around IO exception expectations 3) Change AddRange => Add call
src/VisualStudio/Core/Def/Remote/VisualStudioRemoteHostClientProvider.cs
Outdated
Show resolved
Hide resolved
DustinCampbell
approved these changes
Apr 22, 2025
src/Workspaces/Remote/ServiceHub/Services/Initialization/RemoteInitializationService.cs
Outdated
Show resolved
Hide resolved
| ? Path.Combine(thisAssemblyFolder, assemblySimpleName + ".dll") | ||
| : null; | ||
|
|
||
| if (File.Exists(potentialAssemblyPath)) |
There was a problem hiding this comment.
I don't think File.Exists exposes IO failures
CyrusNajmabadi
approved these changes
Apr 22, 2025
2) Tuples are camel cased (blech) 3) Comment IO exception strategy 4) Use threading context in call to ServiceHubRemoteHostClient.CreateAsync
davidwengier
added a commit
that referenced
this pull request
Apr 24, 2025
#78122 broke Razor tests, because we now have to initialize the export provider builder first. Initializing the export provider builder broke Razor tests, because I had an old copy of the EA dll laying around Fixing the name of the EA dll broke Razor tests, because of a MEF composition error with the RazorTestLanguageServerFactory And thats how we got here.
This was referenced Apr 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cache the MEF catalog created in the roslyn code analysis process.
On my dev machine, the creation of the MEF catalog in our CA process takes about 500 ms. With this change, runs that were able to load from the cache saw that time reduced to about 150 ms.
The high-level change here is to piggy back on the work that Joey did a while back to add MEF catalog caching to our VSCode language server. This PR just moves that code down to a layer where our service hub code can reference, and generalized it a bit to fit into what the service hub code can provide.
One slight additional change in this PR was to add a new remote service to handle passing over the proper location to store the mef cache. I didn't feel comfortable fitting this into the previous first message sent across (IRemoteProcessTelemetryService), so I created IRemoteInitializationService to better fit what I wanted. This led to creating a separate type to handle the export provider creation (RemoteExportProvider) instead of tying this up into RemoteWorkspaceManager.