Faster completions

[nosami]

This makes completions faster by not requiring a full parse to complete before issuing completions.

We parse once per line, and subsequent requests send the lineStr to FSAC. Corresponding FSAC PR incoming.

[nosami]

[nosami]

probably need a better way to escape the linestr here.

[nosami]

This is with the 0.5 second delay reduced

[nosami]

Reducing the idle delay means the popup is triggered faster, but then we lose the benefit of the StartsWith filter which seems to help a lot with the deserialization cost for large lists.

[nosami]

These PRs definitely aren't ready to be merged. I just submitted them so that you could try them out and get a feel for the difference. There are problems with this approach, e.g. lambda completions don't work so well now. Maybe a parse command could be triggered when typing -> ?

[nosami]

I also had the idea of serializing directly to an elisp structure. Emacs seems to struggle with large json objects. No idea if it would help or not.

[rneatherway]

I also had the idea of serializing directly to an elisp structure.

That is an interesting idea, I can imagine it might help quite a bit.

We could also trigger parsing after shorter idle delays perhaps? I'll give it a go and see how it feels.

[nosami]

I noticed too that emacs doesn't use all the columns that are returned by autocomplete. Maybe we could specify on the request which columns we want?

[rneatherway]

It's possible, but it would complicate the serialization code in FsAutoComplete quite a bit. I'm surprised that the deserialization is so slow.

[nosami]

We could also trigger parsing after shorter idle delays perhaps? I'll give it a go and see how it feels.

As far as I can tell, the only time the parse step is needed is at the start of a new line and moving into a new lambda context.

[juergenhoetzel]

It's possible, but it would complicate the serialization code in FsAutoComplete quite a bit. I'm surprised that the deserialization is so slow.

For me completion is quite fast. I instrumented the fsharp- and json- packages and got this after 10 completions on System. prefix:

Function name                         Call Count  Elapsed Time  Average Time
json-read                             11881       2.5372475199  0.0002135550
fsharp-ac-filter-output               80          1.733714629   0.0216714328
json-read-object                      2395        1.4825002189  0.0006189980
fsharp-ac-handle-completion           10          0.8634924860  0.0863492486
fsharp-ac-completion-done             10          0.8634353940  0.0863435394

So handling of the completion response just takes 0.086 seconds in the whole System namespace.

[nosami]

Looks to be a lot of time sunk on reading json there.

This PR is about speeding up the time between calling fsharp-ac-make-completion-request and getting the response on fsharp-ac-handle-completion. I don't think that you are measuring that.

[juergenhoetzel]

Looks to be a lot of time sunk on reading json there.

This PR is about speeding up the time between calling fsharp-ac-make-completion-request and getting the response on fsharp-ac-handle-completion. I don't think that you are measuring that.

Yes. Because the request/respone is async the results just reflect the time spent on the Elisp side.

I wonder whiyjson-read > fsharp-ac-filter-output even though json-read is only called from the process filter. Maybe i have some other json code in my Emacs setup or this is just a elp profiling issue.

elp also slows down the evaluation. Anyway The instrumented coded needed only 0.086 seconds to deserialize the 228 candidates. Maybe you can provide an example where deserialization is slower?

What's also interesting is the fact, that most time of fsharp-ac-completion-done is spent in this function:

fsharp-ac--isNormalId                 2280        0.7079651460  0.0003105110

[nosami]

Maybe we should start a new ticket to talk about serialization. This PR wasn't meant to be about that. Nor am I saying it's slow. Just that it can be faster :)

In my mind, it should be almost as fast as XS.

This is a gif recorded from F# interactive in my latest code from Xamarin Studio. These completions are also out of process and there is almost zero lag.

[nosami]

The optimisation that I'm describing here is the same one that has been in Xamarin Studio for a few weeks now and it's made a dramatic improvement there.

[juergenhoetzel]

The optimisation that I'm describing here is the same one that has been in Xamarin Studio for a few weeks now and it's made a dramatic improvement there.

👍

@rneatherway fsharp-ac--isNormalId which is taking so much time seems to be a helper function for this old issue:

fsprojects/zarchive-fsharpbinding#233

But i can't complete backticked identifiers. Can you confirm? Maybe reopen?

[nosami]

@juergenhoetzel can you confirm if the json-read times are from emacs-fsharp-mode or from something else?

Regarding fsharp-ac--isNormalId, I think that could be removed.
FsAutocomplete sends Name and ReplacementText, where ReplacementText is the identifier surrounded with backticks when necessary.

This duplication could probably be removed too and maybe replaced with a bool flag

[rneatherway]

Regarding fsharp-ac--isNormalId, I think that could be removed.

This is possible, but historically it was also necessary to use it so that autocomplete knew which text to actually replace. For example, if you do a replacement from

``bla $

Where bla bla is in scope, then we have to make sure to replace the leading backticks.

[juergenhoetzel]

@juergenhoetzel can you confirm if the json-read times are from emacs-fsharp-mode or from something else?

I can confirm the times are from fsharp-modeonly. But i profiled in interactive mode. So fsharp-ac--parse-current-file was also invoked multiple times (via idle-timer) while typing. So the info and errors also had to be parsed.

Now I evaluated:

(dotimes (x 10) (with-current-buffer debug_fs
          (goto-char (point-min))
          (search-forward "open System.")
          (company-complete)
          (wait-for-condition (eq fsharp-ac-status 'idle))
          (company-abort)))

and so profiled only completion handling:

json-read                             1320        0.5793947319  0.0004389354
json-read-object                      280         0.3074649659  0.0010980891
fsharp-ac-filter-output               11          0.1860427469  0.0169129769
fsharp-ac--get-msg                    31          0.159459154   0.0051438436
json-read-array                       30          0.154363244   0.0051454414
json-skip-whitespace                  5010        0.1027159319  2.050...e-05
fsharp-ac-handle-completion           10          0.024553135   0.0024553135
fsharp-ac-completion-done             10          0.024497708   0.0024497708
fsharp-ac--isNormalId                 240         0.0221880499  9.245...e-05
fsharp-ac--isIdChar                   2000        0.0123237139  6.161...e-06
json-read-string                      2050        0.0118085600  5.760...e-06
fsharp-company-candidates             10          0.006647409   0.0006647409

[nosami]

@rneatherway Yeah... that makes sense. It's also currently being used to parse every completion returned here

emacs-fsharp-mode/fsharp-mode-completion.el

Line 372 in 7389ac8

(if (fsharp-ac--isNormalId s) (fsharp-ac-add-annotation-prop s candidate)

@juergenhoetzel That's interesting. Looks like around 80% of the processing time in emacs is spent parsing json.

[rneatherway]

Thanks so much for putting some hard numbers on this guys, it's really helpful. @nosami I agree changes to the serialization would be a different PR. We just need to nail down escaping of double quotes in the lineStr here. As I said on the other PR either backslashes or using JSON in both directions would work. What do you think?

I did find one slightly annoying case where I type:

module XA =
  let funky x = x + 1

let x = List.map (fun x -> x.$

Just after I press the . at the bottom it gets completed to XA. I'm not sure if that is introduced here, but it is a bit annoying. Any thoughts?

I was testing the lambda completion like you suggested, and when I put (x: string) then I got the right completions from x.$). I was wondering if we should issue a parse just if the user has requested completions explicitly (e.g. with C-c C-.).

[nosami]

Just after I press the . at the bottom it gets completed to XA. I'm not sure if that is introduced here, but it is a bit annoying. Any thoughts?

This is what I meant about triggering a parse as soon as -> is typed. I think it would mean that x is added to the completion list.

I was wondering if we should issue a parse just if the user has requested completions explicitly (e.g. with C-c C-.).

This is probably a good idea.

My biggest worry about this is that the other editors would need to change too.

[rneatherway]

My biggest worry about this is that the other editors would need to change too.

Yes, good point. We've looped in @ionide, so that part is fine, we need to check with @kjnilsson for VIM, and @guillermooo for Sublime (although I think that is dormant). They will continue to work with their version of FsAutoComplete until they upgrade, so it won't affect existing functionality. I think the change is small enough (as long as the escaping is easy).

[nosami]

We just need to nail down escaping of double quotes in the lineStr here

This would require an FParsec change in FSAC. I'm not so good with that yet :)

[guillermooo]

@rneatherway My plan is to resume work on the Sublime plugin, but I don't know when yet 😢 Feel free to make breaking changes, as I intend to rewrite a good deal of the plugin anyway, 😱 including the interaction with fsac.

[kjnilsson]

This makes things a fair bit more complicated as vim is not async I want to interact with fsac as little as possible, especially when editing text. I assume I can still keep my current reparse strategy just add the additional current line? Parsing on going into insert mode doesn't make sense as I only display errors in normal mode.

[nosami]

@guillermooo both fsautocomplete and FSC are both line based. You can probably make it work somehow though.

@kjnilsson Yeah... you don't need to make any other changes if you don't want to other than adding the line text. Parsing on entering normal mode will probably work too. This change means that you need to interact with fsautocomplete less than you do now.

[kjnilsson]

@nosami doesn't this mean I have to reparse the file for each line edit in insert mode? currently I don't parse at all in insert mode unless completion is requested. I will give it a go and see how it works out but as long as I can keep my current strategy in case it doesn't then I'm all for it.

[nosami]

No.... So - right now, when you press tab with supertab or whatever, you issue 2 commands, parse, followed by complete.

If you implement this, then on the first completion on a line, you would invoke parse, then complete. For the other completions on the same line, you can just invoke complete

[kjnilsson]

AAH! I get it now. doh. :)

[nosami]

Now, it's as fast as XS :)

[rneatherway]

Wow, looking really great! Do you not need to send the lineStr for the other commands too then?

[nosami]

Yeah. The PosCommand code is common between those commands, so that works by default :)

I think the typesig command was slowing completions down previously.

[rneatherway]

Oops obviously! Thanks.

[rneatherway]

Happy to merge this and the FSAC PR once the tests pass. Thanks for working on this!

[nosami]

Cool... It's almost done. Just need to reparse on -> I think. Can you think of any other cases?

I also spotted a bug that must have been there before (maybe introduced in my company PR) but needs fixing. If you have a line like this Console.WriteLine(|) where | is the cursor, then when you complete, it erases back to the .

I also figure there could be another gain to be had by not requiring the initial parse on a line if the background parse was triggered there.

[rneatherway]

I suppose if you close a bracket then that will indicate that a new typed expression could do with being given a type, so that it one possibility. Are you thinking of triggering the parse immediately? For large files it could be a bit of an issue due to Emacs being single-threaded. Parsing on idle isn't really noticeable, but in the middle of typing it might be. Of course, if we could just send differences that would be great, but a) each editor would be different, b) I don't know how to get that info easily in Emacs, and c) it's a separate PR if at all!

Ah, I hadn't seen that bug, but yes it would be good to fix.

[nosami]

Yeah, I was thinking of issuing a parse as soon as -> is entered so that hopefully the parse is already done before you want completions. It was previously parsing for every completion request, so it should still be faster than it was before. Will try it out and see how it feels.

I already have some code for sending changes as they happen for Emacs. This is usually a single keypress and location, but can be a selection replacement etc too. It means that the server always has the same state as the editor. Now that we aren't sending the buffer for each completion request, I'm not so sure it's needed any more. It would definitely need to be a separate PR :)

[nosami]

I just had an idea. Now we are sending the lineStr with requests, it should be possible to do a background parse in FSAC with the new reconstructed source without it affecting the editor at all.

This would probably help with the (some type).| completions (although they seem fairly rare in F#)

[nosami]

The tests are passing for me locally when I run against my locally compiled FSAC.

[rneatherway]

Thanks a lot for this!

[nosami]

Thanks for merging / testing :)

On Tue, Apr 5, 2016 at 8:37 PM, Robin Neatherway notifications@github.com
wrote:

Thanks a lot for this!

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#65 (comment)

[juergenhoetzel]

I just tried with the new 0.28 release. And got:

1459969197.4844868: completion "/home/juergen/fsharp/SuaveTest/src/SuaveTest/Library.fs" 11 12 400 filter=StartsWith
1459969197.493854: {"Kind":"info","Data":"Background parsing started"}
....
1459969197.4945111: Received 'errors' message of length 65
1459969197.4987485: {"Kind":"error","Data":"Unknown command or wrong arguments"}

Any hints appreciated.

BTW. Seems the version Info-String wasn't updated in the 0.28 release:

→ mono bin/fsautocomplete.exe -version
FsAutoComplete 0.27.4

[nosami]

Are you on the master branch of emacs-fsharp-mode?

Background parse is working here with the MELPA build.

1459969713.008002: {"Kind":"info","Data":"Background parsing started"}

1459969713.008106: Received 'info' message of length 65
1459969713.008153: {"Kind":"errors","Data":[{"FileName":"/Users/jason/src/monodevelop/main/external/fsharpbinding/MonoDevelop.FSharpBinding/FSharpTooltipProvider.fs","StartLine":33,"EndLine":33,"StartColumn":12,"EndColumn":13,"Severity":"Error","Message":"Missing qualification after '.'","Subcategory":"parse"},{"FileName":"/Users/jason/src/monodevelop/main/external/fsharpbinding/MonoDevelop.FSharpBinding/FSharpTooltipProvider.fs","StartLine":33,"EndLine":33,"StartColumn":9,"EndColumn":13,"Severity":"Warning","Message":"This expression should have type 'unit', but has type 'string * Shared.XmlDoc * string'. Use 'ignore' to discard the result of the expression, or 'let' to bind the result to a name.","Subcategory":"typecheck"}]}

[juergenhoetzel]

Yes, master branch, commit 70c238e

[nosami]

Deleted any .elc files?

[juergenhoetzel]

Sry 😢

I had a long day. Indeed i had outdated elc files. I was misled by the outdated version string.
All good.

[rneatherway]

@juergenhoetzel you are 100% right about the version string issue though, I am looking into that thanks!