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

marginalia-annotate-command-binding is slow #16

Closed
minad opened this issue Dec 7, 2020 · 8 comments
Closed

marginalia-annotate-command-binding is slow #16

minad opened this issue Dec 7, 2020 · 8 comments

Comments

@minad
Copy link
Owner

minad commented Dec 7, 2020

This is something I observed a few times now. If I have marginalia-command-annotate-binding activated, scrolling through the list of M-x candidates is slow and lags. Scrolling through M-x is even slower than scrolling through a list of files with the marginalia-annotate-file enabled.

I did a bit of investigation and the problem is the where-is-internal function, which seems to be pretty bad. It seems to allocate a lot of memory (responsible for 70% of the allocations in the profile). This means when scrolling through the candidates the gc sets in every few lines. I find it acceptable that it sets in from time to time. This is how the gc works, not much can be done about that. But for my feeling it sets in way too often given how simple the annotator function is and given that only a handful of candidates is annotated at once. It certainly helps to increase the gc limit at the cost of longer pauses from time to time.

I am not sure what we can do - do we have to do anything? Do some caching? Rewriting the where-is-internal function and contributed to upstream if there is some potential for improvement?

Anyone else seeing this? I am on Emacs 27.1.

@oantolin
Copy link
Collaborator

oantolin commented Dec 7, 2020

Huh, the built-in annotation shows the key binding too. Is it also slow? Does it also use where-is-internal?

Maybe the slowdown is because of our buffer switching to show the keybindings in the previously selected buffer (instead of showing the bindings in the minibuffer)?

@minad
Copy link
Owner Author

minad commented Dec 7, 2020

Yes, it does and I think it is also slow, but I would have to recheck this. The slowdown is not due to the switching. The switching has only marginal cost (I tested without switching too). But Fwhich_key_internal is a mess, take a look at the c code. It autoloads keymaps, it can call gc, it does some internal caching etc.

@oantolin
Copy link
Collaborator

oantolin commented Dec 7, 2020

Probably where-is-internal, then. I just checked and read-extended-command--annotation does use it too.

@minad
Copy link
Owner Author

minad commented Dec 7, 2020

I just did the check of the Emacs 28 annotation function. It is as slow. Responsible for 94% of the allocations and 31% of the runtime when scrolling through the M-x list. I originally took the binding annotation code from Emacs 28.

minad added a commit that referenced this issue Dec 7, 2020
* See #16.
* This eliminates the annotation overhead entirely.
* Unfortunately we are back to showing minibuffer bindings again (Emacs 28 behavior).
* It is not entirely clear to me why it is fast to loop once over
  all symbols doing the where-is-internal lookup.
  For some reason it is better to do the work in bulk.
  This is not uncommon for work depending on gc throughput.
* Should we add a different function marginalia-annotate-binding-exact
  and rename the caching function to marginalia-annotate-binding-fast?
  Then the user can select between the two functions via fset.
@minad
Copy link
Owner Author

minad commented Dec 7, 2020

@oantolin See 1269445. Do you like that or do you think this is overdoing it? Should we add the alternative function? Given the Emacs 28 annotation behavior I am okay with showing the minibuffer bindings. Alternatively we could do caching per buffer, but I wouldn't want to make things more complicated here.

@minad
Copy link
Owner Author

minad commented Dec 7, 2020

Hash table per buffer 6567947

@minad minad closed this as completed Dec 7, 2020
@oantolin
Copy link
Collaborator

oantolin commented Dec 8, 2020

The per buffer caching is great!

@minad
Copy link
Owner Author

minad commented Dec 8, 2020

I still wonder why it is like it is. It is astonishing that the bulk workload is so much better for the garbage collector. But as of now our annotations are both faster and more correct than the ones from Emacs 28 :)

minad added a commit that referenced this issue May 24, 2021
I cannot reproduce the allocation problem anymore with Vertico or Selectrum.
Maybe this was due to executing the annotators in a temporary buffer in
Selectrum back then, which lead to flushes of the where-is-internal cache?
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

2 participants