Use partial analysis results to filter out analyzers that need to be executed.

[ToddGrun]

Speedometer scrolling test is showing a huge amount of CPU in MethodCompiler::CompileMethod and AnalyzerDriver.ProcessCompilationEventsCoreAsync. These method are used by several features to get diagnostics, one being lightbulbs. It appears this system already caches diagnostics, but wasn't using those results for partial file analysis. This PR just keeps track of analyzers that have generated results for span based analysis, and uses those results if a subsequent matching request comes in on the same compilation.

1st test insertion build (based on commit 1): https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/594706
2nd test insertion build (based on commit 3): https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/594883
3rd test insertion build (based on commit 3): https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/595116

The amount of time spent in both CompileMethod and ProcessCompilationEventsCoreAsync varies between runs, in some of the runs they are improved with this change, in some, they are unaffected. However, the lightbulb speedometer test consistently shows a marked improvement, as outlined in one of the comments in this PR. That improvement is consistent between runs, albeit for a bit contrived scenario (repeatedly invoking completion in the same location without modifying the document).

[CyrusNajmabadi]

Maybe add some docs? The part I didn't quite understand is that we're comparing spams, but I didn't know what the spans correspond to. For example, is the span what is on screen? If so, I wouldn't expect the prior and current values to match up as you scroll.

[ToddGrun]

Maybe add some docs? The part I didn't quite understand is that we're comparing spams, but I didn't know what the spans correspond to. For example, is the span what is on screen? If so, I wouldn't expect the prior and current values to match up as you scroll.

That's fair, I'll add a bit of documentation. My very rudimentary understanding says that the request for diagnostics is what passes in the span, so it usually corresponds to the line the caret is on (and sometimes the containing method body extent) depending on the caller.

[CyrusNajmabadi]

Ok. Can you verify that and comment if so. If it's method body, then that's a lot clearer as why the spans would be the same across edits

[CyrusNajmabadi]

Tentative approval if investigation matches

[ToddGrun]

Ok. Can you verify that and comment if so. If it's method body, then that's a lot clearer as why the spans would be the same across edits

Added some comments, it looks like the compiler analyzer conditionally increases the scope of the diagnostics request, otherwise, it's pretty much line based.

[ToddGrun]

@mavasani -- Would love your opinion on whether this change has merit or whether there are concerns that might make it worth not considering. Thanks!

[mavasani]

Hey @ToddGrun overall, the change looks fine to me. However, from what I recall, we never used to re-use the compilation in the IDE for lightbulb or any span based diagnostic computation, so caching these in the core analyzer driver layer would not have rendered any benefits. I am not sure if this changed with the move to LSP pull diagnostics or some other part of the system that is now holding onto the compilations used for line based diagnostic computation - if it does then this change will definitely have benefits, if not we would be unnecessarily adding complexity to an already complex piece of code.

[ToddGrun]

if it does then this change will definitely have benefits, if not we would be unnecessarily adding complexity to an already complex piece of code.

-- There are definite improvements when lightbulbs are repeatedly requested without any changes to the compilation. It's a bit of an artificial measurement, but the lightbulb speedometer tests were the ones that actually reported an improvement from the test insertions.

Also, I was definitely able to hit cases where the cached values were being used whereas they weren't before. As I said, there are various levels of caching on both the VS and OOP side, so I wasn't completely confident in this fix, but I felt like it fit in fairly well with the existing code in the area as it had most of the data that I needed to make this association.

[ToddGrun]

Going to elevate out of draft status so the @dotnet/roslyn-compiler folks can determine if they see any issues with this as the changes lie under the compiler folder.