improve VS update performance after project changes

[dsyme]

This is a set of improvements to VS perf, initially for #3094 but also #2107 and #3054, and indeed any situation where you're editing code across multiple files in a project.

1. Remove the 1 second pauses before background work is started when the queue is empty, and use a 5msec pause instead. The intention of the 1sec pause was to allow foreground UI intellisense requests to come in before starting background work. However we are relying on background work to happen to propagate some things in the UI (e.g. to push through changes in solution options by incrementally type checking the project and triggering a Reanalyze). Further, I suspect the 1 second pauses were happening all the time, especially when SimplifyName diagnostic analyzer was being used.

2. Remove the startup pauses for some of the analyzers. Normally these analyzers get run in parallel, and the async startup pause was effectively a cheapshot way of specifying priorities. However these analyers get run in sequence after a Reanalyze has been requested. Further they get run before semantic colorization is done. This means no colors get updated until all of them have been completed

3. Disable the SimplifyName analyzer. This can take a a long time to run because the names are checked one-by-one (and there is currently a pause checking each name). I strongly suspect that this is a real problem and a major cause of "lag" in different ways. If analyzers are run in sequence (e.g. when Reanalyze is called) , then any long-running analyzers are a real problem

@vasily-kirichenko From what I've seen in debugging we can't afford to have SimplifyName analyzer on if document diagnostic analyzers are being run in sequence in response to Reanalyze. Perhaps we could have an option to explicitly enable it, we could double check whether Roslyn really is running the analyzers in sequence and why. I'm a bit concerned about the other analyzers too for large files.

All in all the changes in this PR seem to significantly improve the performance of red-squiggly update speed in the face of

• edits in other files
• project configuration changes (from 10sec down to 5sec, see Coloring is unreliable between #if-#endif #3094)
• project option changes

[dsyme]

This also includes a fix to #2107, see this line. After this change we get these results v VS2015

[Pilchie]

Tagging @heejaechang, but regarding running analyzers serially: IIRC this was a deliberate decision for in-proc analyzers, since running them in parallel ended up generating enough garbage that collections significantly impacted UI thread responsiveness while typing.

[dsyme]

@dotnet-bot test Windows_NT Release_ci_part2 Build please

[dsyme]

running analyzers serially: was a deliberate decision for in-proc analyzers, since running them in parallel ended up generating enough garbage that collections significantly impacted UI thread responsiveness while typing.

@Pilchie @heejaechang Could we open a Roslyn issue about this? I think it's a significant issue. For F# we have more or less been assuming that all these analyzers get run in parallel on more or less every code edit (and get cancelled if another edit occurs). We put in some "async sleep" operations to effectively change their priority, delaying the low priority ones. Now that we understand that the document diagnostic analyzers are actually getting run serially in response to "Reanalyze" we will need to reconsider and scale back how much work we do in the analyzers.

[dsyme]

@KevinRansom @cartermp This is ready. It's an important set of fixes (and one feature removal) that we should include ASAP.

[forki]

;-(

[dsyme]

Yeah, I know. But I feel it's the right thing to do until we get really authoritative information on dotnet/roslyn#20390

SimplifyName is different to the other analyzers in that it makes many requests to FCS - one for each identifier

[vasily-kirichenko]

This analyzer has a setting that we can make off by default instead.

[dsyme]

@vasily-kirichenko ok, thanks, I will submit a follow-up PR

[dsyme]

#3244

[cartermp]

Looks good as a short-term fix.

Long term, I think we should try to push for an API to open up here so that we don't have to rely on DocumentDiagnosticAnalyzer. I think it's only used in a few places in Roslyn, and not intended to be run in parallel.

[dsyme]

Long term, I think we should try to push for an API to open up here so that we don't have to rely on DocumentDiagnosticAnalyzer. I think it's only used in a few small places in Roslyn, and not intended to be run in parallel.

I don't know Roslyn well enough, but I'd somehow be surprised if we can't implement these as some other kind of analyzer

[heejaechang]

we are in talk with data scientist (@roofkitty) to come up with formula to track performance of each analyzer. once we have that in place, we are planning to use that data to order analyzers in real time. cheaper one first, slow one last.

also, we could add some metadata to Document/ProjectDiagnosticAnalyzer to let people to specific ordering such as Low priority.

once we have that, you might not need to order things yourself.

[dsyme]

we are in talk with data scientist (@roofkitty) to come up with formula to track performance of each analyzer. once we have that in place, we are planning to use that data to order analyzers in real time. cheaper one first, slow one last.

This approach won't work for F#. The first one will always be the slow one, since FSharp.Compiler.Service caches the parse/check request. So you will be blaming the first one, you will move it to the back of the queue, and then you will blame the next one etc. etc. That is, you are assuming the performance of the analyzers is independent, when it isn't.

also, we could add some metadata to Document/ProjectDiagnosticAnalyzer to let people to specific ordering such as Low priority.

That will work, thanks

[heejaechang]

@dsyme we are in same situation. so for the problem, we make sure we always run compiler as the very first analyzer so that it can absorb all those lazy-ness cost. and we ignore that analyzer from perf analysis so only other analyzer gets proper ordering.

looks like we also need to have a way to mark [required] or high priority analyzers ..

[dsyme]

Reopening per #3267 (comment) - one line of the fix for this has been reverted - we need to re-check that the solution load performance results mentioned here are still valid