minad / corfu

:desert_island: corfu.el - COmpletion in Region FUnction
GNU General Public License v3.0
1.15k stars 43 forks source link

Corfu calls the dynamic completion function many times #317

Closed andreyorst closed 1 year ago

andreyorst commented 1 year ago

Hi. I wanted to point out that I noticed that Corfu calls the completion function a lot of times, much more than company-mode or plain completion-at-point. Maybe this is intentional, but it has some implications so I think it worth looking

Here's a minimal example to illustrate what I mean:

(defvar x 0)

(defun delayed (sym) 
  (setq x (1+ x))  
  (message "called %s times" x) 
  '("foobar" "foobaz"))

(defun test-completions ()
  (interactive)
  (when-let ((bounds (bounds-of-thing-at-point 'symbol)))
    (list (car bounds)
          (cdr bounds)
          (completion-table-dynamic #'delayed))))

(add-hook 'completion-at-point-functions 'test-completions nil t)

Evaluating this code and putting a point after text foo in a fundamental-mode buffer with Corfu enabled prints this in the *Messages* buffer:

called 1 times
called 2 times
called 3 times
called 4 times
called 5 times

By comparison, calling completion-at-point without Corfu only calls the function two times (one for each completion-style perhaps?):

called 1 times
called 2 times

And with company-mode the function is called only once:

called 1 times

Why this is important to me

I'm making a minor mode for integration with a language process that provides a basic REPL that can give back completions via a special command. This means that with Corfu I'm sending five simultaneous requests for completion and in my case, the requests are synchronous because there's no asynchronous CAPF mechanism as far as I know. The manual suggests using the mentioned completion-table-dynamic function and I thought that it will help me defer the synchronous call, but as it seems Emacs still calls this function many times. The reasoning in the manual suggests that the completion-table-dynamic may be called fewer times than the completion-at-point-functions hook:

Emacs may internally call functions in completion-at-point-functions many times, but care about the value of collection for only some of these calls. By supplying a function for collection, Emacs can defer generating completions until necessary.

Because of these repeated calls, the client basically issues 5 blocking requests for completion, and if the REPL process is busy and can't respond immediately (it is single-threaded) the Emacs is blocked until either the request timeout is reached in the client or the client becomes able to process the request.

For now, I've migrated the issue by using the completion-table-with-cache function instead of completion-table-dynamic, though I'm not sure if it will work robust enough in my case.

minad commented 1 year ago

Corfu calls the completion table as much as necessary, but not more. Generally completion tables must be efficient and should use some internal caching. I recommend that you avoid completion-table-dynamic and completion-table-with-cache since these functions are sometimes not ideal. At least make sure that you understand their implementation and the direct implementation of completion tables without helpers. Furthermore I suggest that you checkout my Cape package, which provides some efficient Capfs.

minad commented 1 year ago

Also note that as soon as Corfu is active, it calls the completion table on every keypress since the completion table may produce new candidates for the new input. This means there is no alternative to a fast completion table anyway. Corfu at least makes sure that the completion table computation is interrupted when new input is arriving in the meantime.

andreyorst commented 1 year ago

Corfu calls the completion table as much as necessary, but not more.

Can you give more light on why Corfu calls the completion table 5 times while without Corfu it is called only twice?

E.g. without Corfu, calling completion on math. and logging from the completion function shows me that it is called with math first and then with math. second. With Corfu, it is math, math, math., math, math..

minad commented 1 year ago

You can print the stack traces and check where the calls are coming from. The reason is mainly that the completion table is called to obtain metadata (fast), boundaries (fast) and candidates (potentially slow). The only call which is potentially slow is the one to obtain the candidates. For completion-table-dynamic each of these calls will call your count function which is less than ideal. Checkout the source of completion-table-dynamic. Also note that completion-table-dynamic will only work with the basic completion style. I recommend that you avoid it. I don't know why default completion only makes two calls - it should rather make three calls at least, but maybe it is bailing out earlier somewhere. (EDIT: I think default completion may not call the metadata or boundaries action when completing manually, but none of this should matter since these actions must be fast.)

andreyorst commented 1 year ago

Checkout the source of completion-table-dynamic. Also note that completion-table-dynamic will only work with the basic completion style.

I've made my own version of completion-table-with-cache because we do need to be able to call it on a pretty specific string, as the backend doesn't support any fancy completions, as of today, so the basic style should be fine.

I recommend that you avoid it.

I'll look into CAPE, but I don't know if in my situation there are better ways. If the process is busy with other tasks it will not even accept the completion request, so my completion function bails out if the process is not ready to accept the input, but because the function is called several times, it bailed out 4 out of 5 calls and the last call always returned nil, because the first completion request was still processed.

minad commented 1 year ago

I've made my own version of completion-table-with-cache because we do need to be able to call it on a pretty specific string, as the backend doesn't support any fancy completions, as of today, so the basic style should be fine.

It is the correct thing to write your own completion table with a cache.

I checked your recipe with manual completion and Corfu. I have to correct myself - completion-table-dynamic filters out boundaries and metadata. These action calls are are at least not a problem.

When I type foo TAB, I get three calls to the completion table with Corfu. The first call tries to complete the existing string (1. try-completion). The result is the string fooba. Then Cofu checks if completion has finished or if further completion is possible (2. test-completion). Since further completion is possible Corfu computes all candidates (3. all-completions). I get the same three calls with default completion.

If you use auto completion (corfu-auto non-nil), then many more calls happen since Corfu has to check repeatedly if completion is possible and if new candidates are available. This behavior is especially pronounced if you use aggressive auto completion settings. Such aggressive settings work only well if your completion table is fast enough.

;; Not recommended in general: Aggressive auto completion settings.
;; Compare with the default settings.
(setq corfu-auto t
      corfu-auto-delay 0
      corfu-auto-prefix 0)