Make completions faster by not requiring a parse on each request

[nosami]

To go with fsharp/emacs-fsharp-mode#65

This is a breaking change, lineStr is sent here along with each PosCommand

[nosami]

Failing tests, will fix if we agree this is a good approach.

[rneatherway]

Cool, thanks for PR. Can you explain a bit more about how this helps? What is the cause of the latency that we are avoiding here? Is it sending the whole text of the file over the pipe each time before requesting completions?

This definitely won't allow completions in the case of (typed expression).$ as there won't be type information available for typed expression, but I don't think that works properly at the moment anyway (see TODO comment near your change).

What's the reason for sending the completions first? I sent the helptext for the first completion first so that it is available to be displayed in the client editor immediately when the completions arrive.

[nosami]

This PR is to avoid having to parse on each completion request. This is what was making it slow.

I sent the completions before the helptext because I think it will mean that the popup is displayed faster. I didn't test to see if that was the case, it just looked as though it would work better that way around.

[rneatherway]

This PR is to avoid having to parse on each completion request. This is what was making it slow.

I see that, but where exactly is the slowdown? Is it in transmitting the text, or in FsAutoComplete, or in FCS? Once you get to FCS it should be asynchronous and not necessarily affect the speed. Likely we are using cached results by that point anyway.

I'm not opposed, just trying to understand exactly what will happen. We should make sure this will still work for @Krzysztof-Cieslak with the Suave frontend as well.

I sent the completions before the helptext because I think it will mean that the popup is displayed faster. I didn't test to see if that was the case, it just looked as though it would work better that way around.

Is it the case that you can see the helptext immediately for the first completion? I didn't see those popups in your gif.

[nosami]

I think I would prefer not to send the helptext with completions at all if it means slowing the completion popup.

I think sending the whole buffer on each keystroke isn't necessarily be bad unless the buffer is very large (1000s of lines). In OmniSharp, we mitigated this by sending only the changes across the wire as they happened. This change isn't really to fix that although it helps here too. The main delay is the parse step.

[nosami]

The popup still works (after a delay).

[Krzysztof-Cieslak]

I think I would prefer not to send the helptext with completions at all if it means slowing the completion popup.

👍 That's one of things I want to change / discuss. Not only from performance point of view but it would also simplify Core API a bit - as far as I remember it's only place when we return multiple responses from one request so we are force to use lists around whole API.

[Krzysztof-Cieslak]

Also, change in "frontend" side looks pretty simple, adding it to Suave version shouldn't be a problem

[rneatherway]

I think I would prefer not to send the helptext with completions at all if it means slowing the completion popup.

As long as the help comes up cleanly, it's good for me! I agree it would be cleaner. That was originally a bit of a hack for using the autocomplete package so it might not be necessary for company. I will give this combination a go at my end.

[rneatherway]

Seems excellent! Working really well for me including the helptext so I'm happy to go ahead with this. There is the question of escaping in the lineStr though. Should we just use backslash to escape double quotes, or is it time to switch to using json in both directions?

[nosami]

I think switching to json or some other structured format is probably inevitable at some point. I'd rather not have to do that now though :)

I'd prefer to escape the double quotes.

[rneatherway]

Cool, happy to test and merge on that basis.

[nosami]

As a sanity test, I put the helptext back before completions before seeing if the tests went green. If you want, I can remove it and the array responses.

[rneatherway]

It's up to you. I guess as we don't seem to need to send the helptext first anymore it's cleaner without.

[nosami]

I would remove the helptext, but I'm pretty snowed under with work right now. I think the PRs are mergeable as-is, so maybe merge and I'll get back to it some time?

[Krzysztof-Cieslak]

I can work on helptext, if you don't have time for it.

[nosami]

@Krzysztof-Cieslak that would be great!)

[rneatherway]

OK I'll merge now and happy to also merge the helptext change when a PR comes

[nosami]

Great. Thanks Robin!

[rneatherway]

Thanks to you!

I'm away at the moment, back in action on Tuesday, and I'll release this and the Emacs improvements then.