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

counsel-yank-pop is very slow #3040

Closed
a13 opened this issue Apr 6, 2024 · 3 comments · Fixed by #3045
Closed

counsel-yank-pop is very slow #3040

a13 opened this issue Apr 6, 2024 · 3 comments · Fixed by #3045

Comments

@a13
Copy link

a13 commented Apr 6, 2024

My (emacs-uptime) is 80 days so far and kill-ring has 10k elements in it. The default (benchmark-run (counsel--yank-pop-kills)) runs in 14s, mostly because of two reasons:

  • equal-including-properties is slow
  • cl-delete-duplicates is very slow (if I use -distinct with the same comparator, it's 2.5 times faster)

Could you provide the way to customize the way we compare elements in kill-ring or to remove the usage equal-including-properties completely (this way we can use delete-dups or seq-uniq).

Thanks.

@basil-conto
Copy link
Collaborator

kill-ring has 10k elements in it.

Is that an explicit choice on your part, by customising kill-ring-max? Or does your kill-ring somehow grow beyond kill-ring-max elements?

Could you provide the way to customize the way we compare elements in kill-ring or to remove the usage equal-including-properties completely (this way we can use delete-dups or seq-uniq).

This should be doable, but note that this would diverge from the built-in behaviour: kill-new uses equal-including-properties when the user option kill-do-not-save-duplicates is non-nil.

Another option would be to deduplicate in counsel-yank-pop only when the user option kill-do-not-save-duplicates is non-nil. But this might confuse users who are used to the current behaviour of unconditionally deduplicating.

@a13
Copy link
Author

a13 commented Apr 17, 2024

Is that an explicit choice on your part, by customising kill-ring-max?

correct

Another option would be to deduplicate in counsel-yank-pop only when the user option kill-do-not-save-duplicates is non-nil.

it's still an option, yes. But consult for example, just ignores properties.

basil-conto added a commit that referenced this issue Apr 28, 2024
cl-delete-duplicates rapidly slows down on large inputs.
Alternatives to the approach in this patch:
- Avoid deduplication beyond a certain threshold.
- Deduplicate only when kill-do-not-save-duplicates is non-nil.
- Make equality test customizable.
- Ignore text properties by default.

* counsel.el (counsel--idx-of): New macro.
(counsel--yank-pop-position): Use it.
(counsel-string-non-blank-p): Simplify.
(counsel--equal-w-props, counsel--yank-pop-filter): New functions
for replicating delete-dups under equal-including-properties.
(counsel--yank-pop-kills): Use counsel--yank-pop-filter.
(counsel-yank-pop-action-remove): Prefer setq over set.

* ivy-test.el (counsel-string-non-blank-p, counsel--equal-w-props)
(counsel--yank-pop-filter): New tests.

Fixes #3040.
@basil-conto
Copy link
Collaborator

Which Emacs version are you on? If you're on 28 or newer, I think PR #3045 should improve performance. If you can test it before I merge, that would be great; otherwise I'll merge it later this week and everyone can become a tester :).

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

Successfully merging a pull request may close this issue.

2 participants