Fix some underlying causes of long UI blocks in new LS implementation #1819
Conversation
|
@Pilchie @KevinRansom @OmarTawfik @brettfo I have a couple of questions
|
|
|
The Ubuntu failure is some transient thing with restoring package FsLexYacc 7.0.1, it's unrelated to the PR. So I think this is ready to go in given the comments above |
Yes, the general rule for the F# compiler service is that things are more expensive. The cause of this are
We can make incremental improvements to all of the above.
I was wrong - ParseFileInProject creates the builder for the project and then parses the file and caches that result. That's sufficient. I've not experience validating breakpoints as slow - the problem is more if the operation gets requested and then dropped on the floor (without cancellation).
In the old language service it didn't block. Also, for F# type providers it's crucial that it doesn't. We should adjust the handler so it doesn't block. |
|
Filed #1825 to track the blocking completion. |
|
This also fixes #1756, see this change here We have a separate issue that too may errors/warnings are shown for out-of-project files - the old LS implementation showed the first 3 errors - a good choice which we decided on long ago. I'll add an issue for that. |
|
I'll merge this since all the key CI runs have passed (Ubuntu failure is transient, AppVeyor is just advisory and often times out) |
|
@Pilchie thanks for that |
@KevinRansom @Pilchie @OmarTawfik @brettfo @cartermp BTW one very good thing that's coming out of the Roslynization is that it becomes vastly more clear when a problem is caused by a fundamental problem in FCS, rather than in the layers of horrible C# code in the old FSHarp.LanguageServicee.Base. This makes it much simper and clearer to go fix the problem in FCS, and put it under unit test too. |
|
@dsyme That's great to hear! |
…dotnet#1819) Fixes underlying issues associated with dotnet#1815 After getting a 15+ second UI block in VS 2017, I've done a complete review of the way the new LanguageService implementation is using FSharp.Compiler.Service (the DLL internally called FSharp.Compiler.LanguageService in the Visual F# Tools repo, but it's basically the same component as FCS) In short, the new LS implementation expects FCS to honour cancellation of submitted tasks in a more timely way. This PR greatly improves the cancellability of FCS requests, which was one of the underlying causes of such a long UI block. 1. Improve FCS so that it respects cancellation in a more timely way. Cancellation will now be respected at each step of the evaluation of the incremental build graph (IncrementalBuild.fs). This represents a fundamental improvement to FCS which will be very useful to other F# editors as well, as long as they are cancelling tasks. In particular, some FCS requests could be _very_ long running, e.g. CheckFileInProject may require checking **all** the files prior to this file (and if cross-F#-to-F#-project checking is enabled then it will require checking all the dependent projects too). Prior to this PR FCS was not respecting cancellation of these requests, and was simply running them through and then throwing the results away. 2. The new LS implementation was using some unnecessary calls to Async.RunSynchronously. All of these could inject arbitrary non-responsiveness into some part of the causality chains. I've removed these in favour of fully async code. 3. In FCS the old IsResultObsolete logic can be removed in favour of checking a cancellation token This also fixes dotnet#1756, see [this change here](https://github.com/Microsoft/visualfsharp/pull/1819/files#diff-69fa840cd1c6d144d0cd489cad82e901R88) We have a separate issue that too may errors/warnings are shown for out-of-project files - the old LS implementation showed the first 3 errors - a good choice which we decided on long ago. I'll add an issue for that.
Fixes underlying issues associated with #1815
After getting a 15+ second UI block in VS 2017, I've done a complete review of the way the new LanguageService implementation is using FSharp.Compiler.Service (the DLL internally called FSharp.Compiler.LanguageService in the Visual F# Tools repo, but it's basically the same component as FCS)
In short, the new LS implementation expects FCS to honour cancellation of submitted tasks in a more timely way. This PR greatly improves the cancellability of FCS requests, which was one of the underlying causes of such a long UI block.
Improve FCS so that it respects cancellation in a more timely way. Cancellation will now be respected at each step of the evaluation of the incremental build graph (IncrementalBuild.fs). This represents a fundamental improvement to FCS which will be very useful to other F# editors as well, as long as they are cancelling tasks.
In particular, some FCS requests could be very long running, e.g. CheckFileInProject may require checking all the files prior to this file (and if cross-F#-to-F#-project checking is enabled then it will require checking all the dependent projects too).
Prior to this PR FCS was not respecting cancellation of these requests, and was simply running them through and then throwing the results away.
The new LS implementation was using some unnecessary calls to Async.RunSynchronously. All of these could inject arbitrary non-responsiveness into some part of the causality chains. I've removed these in favour of fully async code.
In FCS the old IsResultObsolete logic can be removed in favour of checking a cancellation token