Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider using fsprojects/fsharp-language-server as a starting point for LSP #361

Closed
georgewfraser opened this issue Apr 1, 2019 · 12 comments

Comments

@georgewfraser
Copy link

I recently read on @Krzysztof-Cieslak twitter that he'll be working full-time on contract with MS for the next few months, and that one of the major focuses will be finishing the LSP implementation of FSAC. Congrats!

For one moment, let me try to convince you (@Krzysztof-Cieslak) to use https://github.com/fsprojects/fsharp-language-server as a starting point for your efforts:

  • Because of FSAC's history as a separate protocol, it contains a considerable amount of incidental complexity that isn't necessary in an LSP implementation.
  • In contrast, fsharp-language-server is a "clean slate" implementation of LSP. Under-the-hood, it uses FSharp.Compiler.Service, just like FSAC.
  • If you migrate FSAC to the LSP, you will have to support both LSP and the legacy FSAC protocol for some time, which will make the job harder.
  • On the other hand, if you "adopt" fsharp-language-server and port features from FSAC to it, you can make a clean break with the past.

I would be very happy to make @Krzysztof-Cieslak , @enricosada and any other maintainers of this repo admins on https://github.com/fsprojects/fsharp-language-server. I think you could very quickly port the features of FSAC that aren't in fsharp-language-server, such as the "type hints" that appear to the right of function definitions.

If I have failed to convince you of this proposal, there are several notable features of fsharp-language-server that it would still be worth trying to replicate in FSAC:

  1. It builds as a standalone executable, see here. This means it has no dependencies on system-installed dotnet, which eliminates a large category of errors.
  2. It's significantly faster than FSAC. This is most easily observed in large projects---open something big, like FSharp.Compiler.Service, and click around and type to see how long autocomplete takes. The reason for this is that FSharp.Compiler.Service is extremely sensitive to the exact order-of-operations you send to it, because of the way its internal caches work. By fiddling with seemingly insignificant details of how I interacted with FSCS, I was able to obtain huge speedups. I don't recall all the details, but you should be able to get FSAC to be just as fast by fiddling with the order of operations you send to FSCS, and you can use fsharp-language-server as a reference of what speed is possible.
  3. It contains some reusable e2e tests of cracking difficult projects and tests of LSP features. Because the LSP tests are written against the LSP protocol rather than any particular implementation, you could re-use them without too much difficulty.

On the other hand, if I have succeed in convincing you to try to "adopt" fsharp-language-server and port the missing functionality from FSAC, just let me know who to add as maintainers 😄

@cartermp FYI

@Krzysztof-Cieslak
Copy link
Member

Krzysztof-Cieslak commented Apr 1, 2019

TLDR; Thanks for the suggestion, but no, we won't do this.

Longer explanation

While I appreciate that https://github.com/fsprojects/fsharp-language-server exists and I welcome anyone wanting to work on the F# tooling, I don't think it provides advantages outweighing potential costs of such move. FSAC is a long-standing project (7 years old) that has been battle-tested by thousands of users on multiple different editors.

Technical part

What you call "incidental complexity" it's usually not an incident - FSAC enhance what FCS does in every single feature - from simple tooltips including signature formatting and markdown formatting of documentation, through autocomplete doing the ordering, autocomplete of external symbols and inserting opens, making sure Attribute autocomplete doesn't insert unnecessary Attribute suffix if it's between [< ... >] to advanced features like background symbol caching persisted in local per-repo DB allowing us super fast (tens of milliseconds) cross-project finding references. And this is just tip of the iceberg - there are tens of smaller or bigger other features that makes FSAC one of the best language servers I've seen.

Those features were created over the years, with hundreds of hours of work of different contributors. Moving away from FSAC is not "clean break". I rather see this as abandoning things that are not trivial to port, that are pretty well designed and what's most important for the users are working right now.
On the other hand, implementing LSP is trivial - it's just a thin communication layer, and FSAC has been designed to be easily extendable with new communication layers. As we can see in #320 it's just boring plumbing code, yes there is quite a lot of this code, but it's nothing special. The value, and at the same time complexity of the FSAC is not in one or another communication protocol, it's in the features it has.

Performance

Also, I'd like once again (I've done this in Slack some time ago as far as I remember) address this "fsharp-lang-server is faster" point. Of course, it's faster, it does fewer things. Simple as that. As I've mentioned - FSAC is not just a thin layer of calling FCS, it does a lot of heavy lifting. As such, obviously, FSAC puts a lot more pressure on FCS reactor queue. In any complex system that's bottlenecked by single-threaded queue, you cannot judge the performance of a single feature. Because it's impacted by any other action that's happening right now in the server. And we do a lot of computing in the background, in fact, most from any F# editor that's on the market - code lenses, custom analyzers, linter, error reporting across files without saving, error reporting across projects without rebuilding projects, etc. All those features impact the general performance of any feature at any moment in time. Additionally as I've said before - for basically any feature we're doing some additional enhancements - ofc, simple autocomplete that's calling one function from FCS and mapping results is faster than the one that orders entries, uses entities cache to provide completions for not opened namespaces/modules, checks if we're in Attribute scope, and do other stuff. Especially type checking of the projects depending on current project to get cross-project errros without rebuilding solutution is something puting lot of pressure on reactor, and accidentally is something that VS is not doing.

Of course, FSAC performance is not perfect - but not beacuse LSP is somehow inharently better. Over the years performance was never huge point of focus for the FSAC maintainers, and I'm strongly sure that there are multiple low hanging friuts, easy to fix, that will imporve FSAC performance. Fortunatly, this is something Microsoft wants to help me with, so I expect improvements in this area over next few months. I think such performance focused work is a way forward rather than reimplementing same set of features in other projects and hoping that we won't have same performance problems.

Community

Abandoning one project and moving to another is not only a technical question, nor it's the "let me know who to add as maintainers" problem, but it's also a thing impacting the community of given projects in general. FSAC is a long-standing project, with a decent amount of contributors that's been contributing for years. Those people know how to contribute, they know parts of the code base they created, they are experts in some particular niches. I'm the only one lucky (and this is just temporary state for me, so I understand them well) - all of them are doing this in their free time, they care about some particular functionalities, or they just want to help. Moving to another project is just an additional thing making their life more difficult - they'd need to learn new code base, they'd potentially need to port something they've already written to the project.
It would make also a life of the users more difficult - maybe someone doesn't want to use LSP but prefers HTTP protocol, what would I tell them? "Hey, you won't get any other fixes and updates as we moved to the project that supports only one protocol?". This wouldn't be really serious from me, and I guess any other of the maintainers of this package also wouldn't feel good about it - in the end, all maintainers of this, strategic for F# ecosystem, project care really deeply about good of ecosystem, and abandoning this project couldn't hurt ecosystem stability. Additionally - every complex distributed system (like F# OSS ecosystem) is prone to inertia - maybe maintainers of vim or emacs plugin won't move from to LSP from day one, maybe they have a reason (and lack of time is a really good reason!) to stay on old protocol and just abandoning something they use, and telling them that they should use this completely different project is problematic for them, and for me it feels like betraying users, and that's the last thing I want to do.
Moving from one project to another is a problem for just casual users of the Ionide (and other clients) - over the years they (or some of them) have learned were they should report issues, how to do, etc. Changing how they should behave is problematic also for them - and we need to remember that those are often "normal" (not-contributing) users so they may miss some twitter announcment or something, and whole situation could be confusing for them.
Speaking of the announcments, there is also PR (Public Relationship) issue - the FSAC is in fsharp org, it's owned by the F# Software Foundation, and for years it's been shown as an example of critical part of the ecosystem created and owned by community. Abandoning such important project may send a bad message, and that's last thing we as a community need.

@georgewfraser
Copy link
Author

Thanks for writing such a detailed response! For what it’s worth, based on my experience traipsing through FSharp.Compiler.Service and the FSAC code about a year ago, I think you will be able to get the same performance with FSAC, even with all its features. My experience was that it’s a matter of calling FSharp.Compiler.Service in just the right order that makes its internal caches happy and then suddenly it goes really fast. The fsharp-language-server implementation might be a useful speed comparison for you once you turn your attention to performance—you should be able to get the exact same autocomplete times, for example. Good luck!

@enricosada
Copy link
Contributor

Thanks a lot @georgewfraser for the suggestion.
If you want to chat to share ideas or explain your solution or want a quick overview of FSAC, i am really happy to do it anytime, just ping me or we can schedule it.

If we can share knowledge will be awesome, also if the two projects continue to exists as separate entities, because the issues are shared.

I think @Krzysztof-Cieslak has already explained really well what i think about it too, so i'll try answer about your points and my high level view of FSAC.

As a personal note about FSAC, my personal feeling and my effort as maintainer is:

  • it's a long living community effort, owned by community.
  • move complexity from editor extension side, to a shared backend.
  • have a stable api based on commands with request/reply, to make editors integration easier when they want to do it (i expect they do that in their free time, so i value that a lot, and try to not waste it)
  • be feature rich, and make it easy for editors to integrate features.
  • do plumbing of different specialized community libraries, to wrap these to a stable api, refactor as needed
  • different editors extensions implementation have different needs
  • accept contributions and make it easy, refactor to best place later
  • long term fixes/effort to make it robust, but hacks as needed.
  • help new areas of development in their scenario, also if that add complexity.

That's what's important for me.
Editor tooling is a long living community effort, we leverage existing libraries (FSharp.Compiler.Service, FSharpLint, sln/Dotnet.ProjInfo, FSharp.Analyzers.SDK, ICSharpCode.Decompiler/Mono.Cecil) where maintainers can improve their specialized area.
Most of the maintenance work is to be up to date with their progress and react to change in ecosystem.
Lot of complexity is to handle corner cases too, because these exists (tooling in mono, .net, .net core are different, OS too, real world code workload and projects) and we try move these to libraries, where possibile.

That mean we can put some enhancement/bugfix inside FSAC asap who seems accidental complexity, but we later refactor and extract to new libraries so easier to maintain (like with current effort of project system to Dotnet.Proj.Info.Workspace), or send bugfix/featurerequests to libraries.
Seems slow, but is always moving forward, at the fastest the free time contributors allow.

A single library like FCS cannot do everything (project system, F# info, additional features, different scenarios and integration with extenal tools), because will become too big to be maintained and entangled (difficult to reason about), so features are splitted in multiple libs. FSAC do the plumbing.

About architecture, the core layer is independent from the transport and api layer, to maintain IO mode (vim/emacs) and http (ionide) in parallel, making easier to add LSP now.

@georgewfraser your project is really interesting, every new solution helps improving the state of the art because we can share ideas, and try different approach.

I'll check the repo sources to see if we can reuse some of the code and pattern to speedup things later.

  • It builds as a standalone executable, see here. This means it has no dependencies on system-installed dotnet, which eliminates a large category of errors.

That's a good point.
FSAC is already a normal console app, so we can do dotnet publish -r osx to produce a native .exe, but with all dependencies it mean it's quite big, and the consumers (vim/emacs/ionide) doesnt have yet a way to generate packages with OS specific bits, so bundling all variations (unix/win/osx) make it really big to download. as framework dependent is a lot smaller and just one version.
Plus the .net core version have some features and limitation vs the .net version (corner cases hits another time, we support users where possibile trying to plan how to reduce complexity).
We expect a dotnet runtime (any version is ok) to be installed, because FSAC is used in editors.
But when LSP lands @georgewfraser that's a good point, because can be used as language server for different applications not only editor, so we will add native artifacts too to releases (and publish as .NET tool).

2. It's significantly faster than FSAC

That's really interesting, i really when community try different ways to fix the same solution, because all projects can benefit of it.
A good example is when Rider team has shown everyone really better performance was possibile with instant type check, so all editors got improvements too.
Or when the Microsoft VF# team improved memory performance of FCS in latest releases.

3. It contains some reusable e2e tests of cracking difficult projects and tests of LSP features. Because the LSP tests are written against the LSP protocol rather than any particular implementation, you could re-use them without too much difficulty.

I'll check these thanks, the test suite is an area who helps a lot with maintenance.
It's a big tradeoff because it's needed but time consuming (to run and to maintain).
Splitting in libraries helps a lot, most of the tests of whole FSAC are not in this repo but in the libraries test suites, we check the integration (who is slow to run).

@7sharp9
Copy link
Contributor

7sharp9 commented Apr 2, 2019

I actually agree with @georgewfraser this is the approach we took with the tooling in Visual Studio for Mac. (back in the day it was xamarin studio) Some of the first work I did was to strip away some incidental complexity from FSAC originally 80% of the internals were also used by xamarin studio, rip out the parser, remove the cracker, remove json encoding etc. Basically remove all the bits that we had issues with. Last I checked in FSAC there were still some parts of what we removed still present, so this could be a good chance to do some internal clean up too.

@cartermp
Copy link
Contributor

cartermp commented Apr 2, 2019

We're considering cleanup and perf work as a part of the deliverables in our contract with @Krzysztof-Cieslak. However, we want to rebaseline performance and measurements after LSP support is in. Among other things, it changes how data is serialized to/from the FSAC process and VSCode, which means issues like #324 will now be different. How that shapes up is TBD, but it will be based on measurements before/after specific changes.

@granicz
Copy link

granicz commented May 24, 2019

@cartermp Are these performance measurements available now that LSP has been merged in? Also, regarding the intended feature cleanup and performance optimizations, as @7sharp9 forecasts some FSAC features will end up removed - is there a definitive list of FSAC features you are aiming for? While I have huge respect for FSAC, I also liked the simple approach fsharp-language-server took, and at the end of the day performance is what matters to most. Some of what has been brought up in this ticket, such as markdown formatting, rarely belongs to a language server.

@enricosada
Copy link
Contributor

enricosada commented May 24, 2019

While I have huge respect for FSAC, I also liked the simple approach fsharp-language-server took, and at the end of the day performance is what matters to most

@granicz i dont think most of the features create performance issues.

While i think performance matter, also real world scenarios matter, both project specific or user specific

Some are not features, like said before, but are corner case, so are quality fix.
Or corner case to support specific scenario.
It's easier to support just basic scenarios, but real world is different.

Each command in FSAC is separate and async.

For example if Ionide ask for linter command, yes, that may add some time.
But is a new feature, editor mainters or users can disable that. Otherwise doesnt add up. Likewise tons of others.

I dont think conversion from xmldoc to markdown (done in a single command, to get specific symbol documentation) is really that time consuming or add up.
Is optin, and really it's just XmlDocument -> string, and editors will need to do that client side, so why?
Also, we need anyway to do XmlDocument -> string for the tooltip, just with a different encoding, so is not zero

If there are perf issues (like #324) happy is someone want to address these.
Last time we measured time (so left forget for a seconds the memory leak bug), the overhead of suave+json was negligible because:

  • clients invoke ~10 request/second
  • most of the time of the command is spent in the logic itself (usually FCS).

@granicz just to check xam studio, there the story was a bit different

was to strip away some incidental complexity from FSAC originally 80% of the internals were also used by xamarin studio, rip out the parser, remove the cracker, remove json encoding etc

  • rip out the parser, remove the cracker

Project parsing was removed because handled builtin in Xamarin. We cannot do that, because it mean editors will need to reimplement that. Instead i am currently improving it (ref #347 )

  • remove json encoding

the LSP mode like other two FSAC modes (stdio/http) is a server, need to talk to client (LSP based clients or FSAC based like vim, emacs or ionide atm) using a protocol.
Currently modes are:

  • stdio: console i/O with json (FSAC protocol)
  • http: http server with json (FSAC protocol)
  • LSP: http server with json (JSON-RPC protocol)

Xam studio was in process (like Visual Studio), i think that's not a good way forward for editors.
The In proc architecture have different tradeoff and issues.

Also only editors built with .NET can do that, not emacs/vim/vscode who need anyway a server layer.
For sure it cannot be used in LSP.

@granicz FSAC is a real world codebase, we are evolving (like previous contributors), it not rewriting it.
Happy to help explain anything of the codebase if someone want to contribute to anything (perf fixes, or otherwise)

@Krzysztof-Cieslak
Copy link
Member

as @7sharp9 forecasts some FSAC features will end up removed

We don’t plan to remove any features

@Krzysztof-Cieslak
Copy link
Member

If performance is all that matters let’s replace all implementation of features with async { return 42} - it will work really fast.

I’m pretty sure such implementation will provide even better gifs that can be shown to prove “better performance”

Some of what has been brought up in this ticket, such as markdown formatting, rarely belongs to a language server.

I guess our users who enjoy detailed tooltips or info panel wouldn’t agree with such opinions.

@cartermp
Copy link
Contributor

@granicz Performance analysis work has started, #384 showing up. Given the nature of perf work, we can't really speculate on anything, but I hardly imagine markdown formatting is the bottleneck based on our own perf work in the F# compiler so far.

@granicz
Copy link

granicz commented May 24, 2019

@cartermp I didn't imply it was, it just doesn't belong there IMHO. I will watch this space for more upcoming details on the perf work then, thanks!

@7sharp9
Copy link
Contributor

7sharp9 commented Oct 4, 2019

Heh, old thread but I never got notified at the time :-)

I didn't mean to imply features would be removed just that slow problematic code would be. At Xamarin we removed the parser and used an alternative, we removed cracking and added an msbuild information gatherer much in the same way @enricosada did years later when msbuild was improved with dotnet i.e. the project-info stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants