Parallelize loading solution-level analyzers#82447
Conversation
| // 'CreateSolutionLevelAnalyzerReferences' needs to be broken out into its own service for us to be able to move this. | ||
| var miscellaneousFilesWorkspace = new LanguageServerWorkspace(hostServicesProvider.HostServices, WorkspaceKind.MiscellaneousFiles); | ||
| miscellaneousFilesWorkspace.SetCurrentSolution(s => s.WithAnalyzerReferences(CreateSolutionLevelAnalyzerReferencesForWorkspace(miscellaneousFilesWorkspace)), WorkspaceChangeKind.SolutionChanged); | ||
| Contract.ThrowIfFalse( |
There was a problem hiding this comment.
This is a basis for the assumption that reusing the same analyzer references array is fine.
There was a problem hiding this comment.
It looks likes the AnalyzerFileReference may reference the actual DiagnosticAnalyzer / ISourceGenerator instances? I am not the most familiar here, but at a glance it seems dangerous to share them between two different workspaces?https://github.com/dotnet/roslyn/blob/main/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerFileReference.cs#L39
Maybe @jasonmalinowski is more familiar here?
There was a problem hiding this comment.
Neither of those types are aware of the workspaces layer, and, are expected to be able to be used with a variety of compilations, etc. in parallel, so, it feels unexpected for the sharing to be problematic. Let's definitely get confirmation though.
There was a problem hiding this comment.
Note that if we went back to essentially doing this work twice, like before, we would probably add 0.5s to the wall clock time.
There was a problem hiding this comment.
yeah if these are shareable, then this definitely makes sense to do. Just not sure on the contract here for that type.
There was a problem hiding this comment.
Tagging @jasonmalinowski for guidance on sharing AnalyzeFileReference here
There was a problem hiding this comment.
So @RikkiGibson is this hitting the I/O issue mentioned in this bug? Because that's also bad for VS perf as well, so it might just be wise for us to make a proper fix for that rather than continuing to work around it.
Otherwise to the question itself: I would guess that sharing an AnalyzerFileReference across different projects where it represents a project analyzer would be bad, since there are edge cases where the dependencies of analyzers could be loaded differently. But if this is only impacting the solution-level analyzers, that's probably fine, especially if you're already checking the loader.
That all said, rather than having to have that Contract check in there, what does this look like if we just fix #78560? Does that let us remove it?
There was a problem hiding this comment.
So @RikkiGibson is this hitting the I/O issue mentioned in #79016
I believe no.
The original code was doing this while holding the workspace lock, but, I think the real badness of the original, was that handling the Initialize request, was blocked on running the constructor for this MEF part.
The new code is not doing the I/O while holding the workspace lock. It just obtains the workspace lock at the end to stick the results in. But most importantly the parallelism lets the constructor return much sooner.
I also considered doing a straight up Task.Run() here, in addition to the parallelism, as we don't need the solution level analyzers to be available instantly, and that did shave off another .3s or so in my Windows test runs. But didn't feel as necessary to do it.
That all said, rather than having to have that Contract check in there, what does this look like if we just fix #78560? Does that let us remove it?
Maybe. It's possible that some factoring of the underlying services that each workspace is using, could allow the same set of analyzers to be used for both, in some more straightforward and more obviously correct way. It feels reasonable to revisit that in the future.
jasonmalinowski
left a comment
There was a problem hiding this comment.
So I guess I signoff since the code is probably safe as written and it's a nice win regardless, and the debt is already tracked (and not changing by this fix.) Maybe if we merge it this way make a note in #79016 to revert this change once that's fixed, since that would moot it entirely.
I found this while investigating slow test perf in
FileBasedProgramsWorkspaceTests.I used test case
TestSemanticDiagnosticsNotEnabledWhenCoarseGrainedFlagDisabled(False)as a reference. This test is not supposed to perform any design time builds, etc., so is expected to be "low cost" compared to many tests in this class.trueargument.)I was able to get some info using "Profile Test" in VS, and measuring "Async Activities". Unfortunately, it seems like the profiler doesn't have full ability to 'trace cause and effect', when, what we are essentially doing is sending a request into a stream, and another thread is reading it out. So, the information I got from the profiler, hinted at CreateTestLspServerAsync, and the Initialize request taking a long time specifically.
However, I wasn't able to get more details than that from the profiler data. Eventually I simply resorted to stepping thru the work the Initialize handler was doing, and literally just noticing when things 'felt slow', and finally sticking in
Log()calls with timestamps to verify where the time was going. It would be fantastic to work out how to do this better in the future.This is not great, and possibly represents me misusing the tools. But, in this case I was at least able to identify a major cause of the slowness. Creating an
AnalyzerFileReferencewill cause us to crack the assembly file to get its name and stuff like that. Sometimes the wait time from that is significant. In practice there were ~45 analyzer DLLs to do this on. We were also doing essentially the exact same work back-to-back, so, sharing the resulting references array between host and misc workspaces, takes off ~0.5s from the test runtime.