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

Company-capf calls the completion table with 'metadata action for each candidate #1351

Closed
blahgeek opened this issue Dec 2, 2022 · 9 comments
Milestone

Comments

@blahgeek
Copy link

blahgeek commented Dec 2, 2022

In the discussion of emacs bug #59678, Stefan mentioned that

if Company really calls the completion table with metadata once per candidate, then there's a bug in company.

So I want to create an issue here to report this possibly unexpected behavior.

Output of the command M-x company-diag

Emacs 29.0.50 (x86_64-pc-linux-gnu) of 2022-11-26
Company 0.9.13

Describe the issue

Company-capf would call the capf function with 'metadata action for each candidate, which makes some modes pretty slow on completing. (The above bug report contains a patch to fix one such issue for sh-mode though.)

Steps to reproduce

In the above sh-mode example (before applying that patch), adding a log in sh--cmd-completion-table can verify the behavior.

Expected behavior

I'm not sure. Maybe company-mode should cache the completion table? Or should it only call 'metadata action for visible candidates?

@dgutov
Copy link
Member

dgutov commented Dec 2, 2022

This was added in 390d9af. We do need every item's annotations to remove duplicates. And so far we haven't been caching the annotation function.

@monnier Is it expected that the metadata can be costly to fetch? IIUC the issue was that the completion table re-calculated the set of completion for every operation, metadata or not. So adding the caching looks like an all-around good idea.

@monnier
Copy link
Contributor

monnier commented Dec 2, 2022

Yes, the caching done in patch installed for bug#59678 is a good thing regardless of this here issue.
While usually the metadata is cheap to fetch, but re-fetching it each time seems like a performance bug [ unless there's a possibility that it changes for different candidates. ]

@dgutov
Copy link
Member

dgutov commented Dec 2, 2022

I suppose we could try to cache it per-string per-completion (since it might conceivably differ for different prefixes).

@blahgeek
Copy link
Author

blahgeek commented Dec 4, 2022

We do need every item's annotations to remove duplicates

Thanks for the information. After some debugging, I think I get it now:

In company--strip-duplicates function, it would only compare items' annotation when the name of the items are equal. So in normal circumstances it should not call 'metadata action if there are no duplicates.

However, in this case, the result completion table does include a lots of duplicated candidates. The completion table is calculated from (locate-file-completion-table exec-path ...), while exec-path contains both /usr/bin and /bin which are exactly the same directly (in my system, /bin is a symlink to /usr/bin).

So overall IMO this is not a bug in company-mode. The issue on sh-mode is fixed by the patch in bug#59678 and this behavior should not affect other modes.

@blahgeek
Copy link
Author

blahgeek commented Dec 4, 2022

I would close this issue then. Thanks for all your help.

@blahgeek blahgeek closed this as completed Dec 4, 2022
@monnier
Copy link
Contributor

monnier commented Dec 4, 2022 via email

@dgutov
Copy link
Member

dgutov commented Jun 15, 2023

But metadata doesn't give you annotations. It gives you a function
which can then give you the annotation. You're supposed to call
metadata once and then use the annotation function N times to retrieve the
N annotations you need.

But completion-metadata takes STRING as the first argument. So the result is not just dependent on the completion table, right? It least, it might vary, even if it doesn't most of the time. So I'm not sure what to use as the cache key.

@dgutov
Copy link
Member

dgutov commented Jun 15, 2023

Ah, never mind. STRING is the prefix, not a completion candidate. Then metadata could be cached, like other bits of information already are.

@dgutov dgutov reopened this Jun 15, 2023
@dgutov dgutov added this to the 1.0 milestone Oct 12, 2023
@dgutov dgutov closed this as completed in 97d3b35 Nov 27, 2023
@dgutov
Copy link
Member

dgutov commented Nov 27, 2023

I've added the cache for capf-metadata. Please test for any breakage, when you have time.

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

No branches or pull requests

3 participants