Background analysis support #1507
Conversation
|
Is there something that is preventing this PR from going through? It seems like a really hot feature to have. |
|
Is there a way I can take this for a test @savpek? I'm using VSCode. |
|
You can setup vscode with dotnet/vscode-csharp#3089 (build vsix plugin from branch and install it to vscode) and configure "omnisharp.path" to omnisharp-roslyn version built from this PR. One of new tests has been broken in mac for some reason for several builds, have to check it out before this is ready. Otherwise this PR is good to go in my mind, but theres new API and events in this PR so feedback/reviewers are really needed 😄 Vscode side requires tests, otherwise it is ready i think. |
|
@david-driscoll fixed test |
There was a problem hiding this comment.
@savpek thanks for the great pull request yet again!
I just have the one comment about maybe using a blocking collection (or a few of them looking at the code), however I'm fine without that as well.
@filipw, @rchande, @DustinCampbell (if you have time 😄 ) let me know your guys thoughts.
src/OmniSharp.Abstractions/Models/v1/ReAnalyze/ReanalyzeRequest.cs
Outdated
Show resolved
Hide resolved
src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs
Outdated
Show resolved
Hide resolved
src/OmniSharp.Roslyn.CSharp/Services/Diagnostics/ReAnalyzeService.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public AnalyzerWorkQueue(ILoggerFactory loggerFactory, Func<DateTime> utcNow = null, int timeoutForPendingWorkMs = 15*1000) | ||
| public AnalyzerWorkQueue(ILoggerFactory loggerFactory, Func<DateTime> utcNow = null, int timeoutForPendingWorkMs = 15 * 1000) |
There was a problem hiding this comment.
should this timeout be configurable in omnisharp.json?
There was a problem hiding this comment.
In previous version i think it could, but now in this version it only applies to request for diagnostics from single file. It's basically just failsafe to prevent api hang in this version.
However i am not sure is this needed any more at all since theres timeout for file analysis too on worker side which should prevent that situation. Timeout for single file analysis in worker could indeed be configurable and that way it's implemented only on single location.
|
Thanks for feedback. On trip for week now but i will fix raised issues after that 😄 |
|
Added new configuration |
|
Ready for re-review @david-driscoll @filipw |
Implements analysis with foreground - background queues.
Added event of project analyzed which can be used to notify that theres more background results available. VScode current fetch is based on editor actions -> they really don't know when more data is available.
Gives faster feedback during (initial) analysis (omnisharp-roslyn codes during startup):
Old:
New:
Add re-analyze api for omnisharp that can be used to trigger either project or all scoped analysis:
When ever diagnostics are not based "calculate everything for every request (which is expensive if theres anything more than simple semantic diagnostics)" we face following problem: how to know when we need to diagnose other files that may have references to current directly or indirectly, for example in case of manual rename of interface (this is true on current LSP implementation too i think). Adds manual option to work with that problem. Even after some sophisticated logic is added theres very tricky cases like change git branches that can modify things in very crazy way and make things offsync -> this manual option is likely needed anyway.
Do anyone know does roslyn expose api to figure those indirect refences out? cc: @rchande @filipw @david-driscoll @mholo65
Current perf:
Original diagnostic solution without analyzer get diagnostics from all files every time, for that reason it linearly scales up when solution is bigger. Heres perf comparison with omnisharp code base:
With analyzers (no background work running):
With version (analyzers enabled) that doesn't cause pressure of diagnostic fetch api (updated on vscode side) on every change, instead if is based on project events:
With further optimizations analyzer enabled version are getting close to get par with original version.
Perf is currently better with analyzers enabled when project get large. In VScode this is currently worked around by flag that limits fetching all diagnostics if there is over 1000 files (configurable). However with that you cannot get errors from all files. I think similar flag should be added to analyzer too and what it does is that it doesn't trigger initial full solution analysis on startup, however it can be invoked manually -> get good from both worlds when working with larger projects.
Ready for review @rchande @filipw @mholo65 @david-driscoll