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

use timer to improve the ivy-read performance #1218

Closed
redguardtoo opened this issue Sep 28, 2017 · 25 comments · May be fixed by #2941
Closed

use timer to improve the ivy-read performance #1218

redguardtoo opened this issue Sep 28, 2017 · 25 comments · May be fixed by #2941

Comments

@redguardtoo
Copy link
Contributor

redguardtoo commented Sep 28, 2017

This trick is used frequently in javascript search box.

When user input characters, don't update the collection display immediately. Cache the user input until user has not input any new characters say for 1 second.

After one second, we continue the UI update.

Could you at least make a option for ivy-read like :seconds-wait-for-user-input. Its default value is zero which means updating UI immediately. But if it's more than zero, UI will wait to make sure the user finish the input.

jeberger added a commit to jeberger/swiper that referenced this issue Oct 12, 2017
This is probably a good idea performance-wise since it limits the number
of collection updates when the user is typing. However, the main reason
for this change is to work around an Emacs freeze on Windows when the
dynamic collection is generated by an external command (see for example
this bug in helm-ag which also affects counsel-ag and similar commands:
emacsorphanage/helm-ag#188).

Related to abo-abo#1218 (but only applies for dynamic collections, where the
difference is really noticeable).
Helps with abo-abo#1198 and abo-abo#786.
abo-abo pushed a commit that referenced this issue Nov 13, 2017
This is probably a good idea performance-wise since it limits the number
of collection updates when the user is typing. However, the main reason
for this change is to work around an Emacs freeze on Windows when the
dynamic collection is generated by an external command (see for example
this bug in helm-ag which also affects counsel-ag and similar commands:
emacsorphanage/helm-ag#188).

Related to #1218 (but only applies for dynamic collections, where the
difference is really noticeable).
Helps with #1198 and #786.

Fixes #1237
@ambihelical
Copy link
Contributor

I tried hacking the new delay code added by @jeberger used for dynamic collections by disabling the actual check for dynamic collection. It works well except for up/down movements which are also delayed. Probably something would need to be done to allow ivy--queue-exhibit to call ivy--exhibit with filtering disabled so the list can be updated quickly. However besides this issue, it is a great quality of life improvement for really large trees and counsel-find-file or similar. I ended up using 200ms delay which seems to suit my typing speed. With an AOSP tree using counsel-projectile-find-file, I was seeing >1s delay or more per character typed. Now I can actually type whole words before the filtering kicks in.

@randymorris
Copy link

Sorry for digging up an old topic here, but I'm in pretty much the same exact situation as @ambihelical described. Any tips on implementing this hack so I can get by until this issue gets some attention?

@redguardtoo
Copy link
Contributor Author

(setq ivy-dynamic-exhibit-delay-ms 250)

@randymorris
Copy link

Unfortunately that doesn't help me here because the collection provided by counsel-projectile-find-file is not dynamic. Setting that timeout has no effect in this case.

@abo-abo
Copy link
Owner

abo-abo commented Jul 25, 2018

@randymorris If I understood correctly, your problem with counsel-projectile-find-file is the start-up cost. There isn't much that can be done here, since we need to call projectile-current-project-files which creates the list of files. After the list is created, the filtering should be very fast.

@redguardtoo Can this issue be closed?

@randymorris
Copy link

Startup time is noticeable but acceptable. My issue is that the filtering happens on each keypress. Often I type a second, third, or fourth key before the filtering finishes for the first keypress. This leads to me having to wait for the filtering to happen four times before I can see what keys I need to press to filter further.

Debouncing as described in the original issue would make the filtering happen only after a timeout rather than after every keypress, just as appears to be implemented for dynamic collections.

The way I see it I have two options:

  1. Somehow make non-dynamic collections have the same behavior that ivy-dynamic-exhibit-delay-ms provides for dynamic collections.
  2. Make counsel-projectile-find-file provide a dynamic collection and set ivy-dynamic-exhibit-delay-ms appropriately.

@abo-abo
Copy link
Owner

abo-abo commented Jul 25, 2018

My issue is that the filtering happens on each keypress. Often I type a second, third, or fourth key before the filtering finishes for the first keypress.

That sounds really bad, I've never noticed such poor performance. What's the amount of candidates you have and what config are you using?

@randymorris
Copy link

The collection of candidates for me was just under 8,000 files. In coming up with a minimal config to give you, I found that this is the culprit:

(setq ivy-re-builders-alist '((t . ivy--regex-fuzzy)))

I think I had followed this post when I migrated to ivy a long time ago, thus I've always had this issue and not realized that the fuzzy matching was the cause. I suppose this "fixes" it for me, although I'll have to get used to the less fuzzy matching if I want it to be fast. I still think that debouncing the keypresses even with a non-dynamic collection is probably not a bad idea.

Thanks for taking the time to reply, and again, sorry for digging up an old topic.

@ambihelical
Copy link
Contributor

I was dealing with a tree with about 200-400K candidates (an AOSP tree), I don't remember the exact number of files it had to filter through, but it was in that range. I added the hack I mentioned to deal with it, with good results, although there were some issues. I don't use fuzzy matching though. You can see the hack I am using in my fork of swiper, branch try-async.

@redguardtoo
Copy link
Contributor Author

redguardtoo commented Jul 25, 2018

@randymorris, try find-file-in-project-by-selected from https://github.com/technomancy/find-file-in-project

It's much faster.

@abo-abo
Copy link
Owner

abo-abo commented Jul 26, 2018

@randymorris I see, I suspected that ivy--regex-fuzzy was the issue. Could you test it with and without flx installed? I wonder if most of the time is being spent on filtering the candidates or sorting them by frequency.

@randymorris
Copy link

I've set my config back to always use ivy--regex-fuzzy and removed flx. My initial reaction is that it feels faster, but I really need to try it for a bit to be sure. I'll be away for a long weekend but I'll check back in next week with a conclusion after I get some time to actually work with this config for a while.

@basil-conto
Copy link
Collaborator

CCing @ericdanan, as a part of this discussion pertains to https://github.com/ericdanan/counsel-projectile.

@ericdanan
Copy link
Contributor

@randymorris You may also want to test whether calling projectile-find-file instead counsel-projectile-find-file is faster. If so, I guess the issue would be with either the matcher or the sort function used by counsel-projectile-find-file (both are customizable, see variables counsel-projectile-find-file-matcher and counsel-projectile-sort-files).

@sebastiansturm
Copy link

I'm also using ivy-dynamic-exhibit-delay-ms but it seems to me that debouncing is currently so aggressive that resting your finger on the up/down arrow will suspend all updates to the minibuffer if your key repeat rate is faster than the delay interval (right?)
While I guess the nicest option (but probably more difficult to implement?) would be to debounce only those commands that change the list of available candidates, a compromise might be to call ivy--exhibit immediately if several successive calls to ivy--queue-exhibit together exceed the delay interval?
For example, the following snippet allows me to browse candidates interactively even with a large delay-ms value, though of course the update rate is pretty low:

(defvar ivy--exhibit-waiting-since nil)

(defun ivy--queue-exhibit ()
  "Insert Ivy completions display, possibly after a timeout for
dynamic collections.
Should be run via minibuffer `post-command-hook'."
  (if (and (> ivy-dynamic-exhibit-delay-ms 0)
           (ivy-state-dynamic-collection ivy-last))
    (let* ((now (float-time))
           (float-delay (/ ivy-dynamic-exhibit-delay-ms 1000.0))
           (t0 (or ivy--exhibit-waiting-since now))
           (out-of-patience (> (- now t0) float-delay)))
      (when ivy--exhibit-timer (cancel-timer ivy--exhibit-timer))
      (if out-of-patience
          (progn
            (setq ivy--exhibit-waiting-since nil)
            (ivy--exhibit))
        (setq
         ivy--exhibit-waiting-since t0
         ivy--exhibit-timer (run-with-timer float-delay nil 'ivy--exhibit))))
    (ivy--exhibit)))

Or maybe there's an easy way to check if the input string has been changed since the last invocation of ivy--queue-exhibit, and use that to decide how aggressively the display update should be debounced?

@Inc0n
Copy link

Inc0n commented Sep 18, 2020

Hmm, I might have a better fix for this:

but it seems to me that debouncing is currently so aggressive that resting your finger on the up/down arrow will suspend all updates to the minibuffer if your key repeat rate is faster than the delay interval

We just need to implement one extra predicate that we can use to indicate whether another ivy--exhibit should be queued, which is when the user has entered commands that modified the prompt, otherwise ivy--exhibit can be executed immediately. Hence, the following:

(let (last-pos)
  (defun ivy-input-changed? ()
    (let* ((pos (line-end-position))
           (changed (not (eq pos last-pos))))
      (setq last-pos pos)
      changed)))

then inside ivy--queue-exhibit we call the newly implemented predicate.

(defun ivy--queue-exhibit (args ...)
  (if (and (> ivy-dynamic-exhibit-delay-ms 0)
           (ivy-state-dynamic-collection ivy-last)
           (ivy-input-changed?))
      then
    else ...))

@kiennq
Copy link
Contributor

kiennq commented Sep 23, 2020

I think this can be done with while-no-input? That will interrupt updating collection when there's new key and thus provide the same effect?
#2567

@Inc0n
Copy link

Inc0n commented Sep 23, 2020

Does looks like a better fix than mine

@huangfeiyu
Copy link

My issue is that the filtering happens on each keypress. Often I type a second, third, or fourth key before the filtering finishes for the first keypress. This leads to me having to wait for the filtering to happen four times before I can see what keys I need to press to filter further.

@randymorris @abo-abo I am experiencing the same issue. I use counsel-projectile to open open file, when I type the file name, the issue occurs. But if I just paste the file name without typing, I get the result quickly.

@basil-conto
Copy link
Collaborator

I am experiencing the same issue. I use counsel-projectile to open open file, when I type the file name, the issue occurs. But if I just paste the file name without typing, I get the result quickly.

Is issue #2911 relevant to this? Thanks.

@huangfeiyu
Copy link

Thanks @basil-conto By the way, @dminuoso has filed an issue in counsel-projectil #2911

@mgalgs
Copy link

mgalgs commented Oct 29, 2021

Does anyone have a workaround for the issue described by @sebastiansturm where moving through the results incurs a delay even though the input isn't changing? I would like to be able to move through results with C-n/C-p without any delay. I don't see any reason to add the delay unless the input changes.

Here's an example demonstrating the issue. Type something and then move through the results. Each ivy-next-line (C-n) or ivy-previous-line (C-p) also gets the 1000ms delay, even though the result set isn't changing.

(defun debounced-ivy-ready-example ()
  (interactive)
  (let ((ivy-dynamic-exhibit-delay-ms 1000))
    (ivy-read "Test: " (lambda (q) (if (string= q "")
                                       '()
                                     (list q
                                           (format "%s/%s" q q)
                                           (format "%s/%s/%s" q q q))))
              :dynamic-collection t
              :action (lambda (result) (message "Got: %s" result)))))

@Gleek
Copy link

Gleek commented Nov 6, 2021

I have a small advice in my config that helps me solve the C-n/ C-p problem while having dynamic exhibit delay set to some number. I'm not sure if this will actually create any other issue as I'm not aware of the internals of ivy but it seems to have solved the primary issue for me.

(defvar +ivy--queue-last-input nil)
(defun +ivy-queue-exhibit-a(f &rest args)
  (if (equal +ivy--queue-last-input (ivy--input))
      (ivy--exhibit)
    (apply f args))
  (setq +ivy--queue-last-input (ivy--input)))
(advice-add 'ivy--queue-exhibit :around #'+ivy-queue-exhibit-a)

Ideally it would make sense to have such change upstream instead.

@mgalgs
Copy link

mgalgs commented Nov 8, 2021

@Gleek thank you for sharing, this is working great!

@redguardtoo
Copy link
Contributor Author

@Gleek , Could you send a pull request to https://github.com/abo-abo/swiper ? Thank you for your great code, btw.

Gleek added a commit to Gleek/swiper that referenced this issue Dec 18, 2021
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
mgalgs added a commit to mgalgs/.emacs.d that referenced this issue Feb 3, 2022
kiennq pushed a commit to kiennq/swiper that referenced this issue Aug 3, 2022
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
kiennq pushed a commit to kiennq/swiper that referenced this issue Aug 10, 2022
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
kiennq pushed a commit to kiennq/swiper that referenced this issue Jan 11, 2023
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
kiennq pushed a commit to kiennq/swiper that referenced this issue Jan 11, 2023
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
kiennq pushed a commit to kiennq/swiper that referenced this issue Jan 11, 2023
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
kiennq pushed a commit to kiennq/swiper that referenced this issue Apr 24, 2023
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
github-actions bot pushed a commit to kiennq/swiper that referenced this issue Jun 22, 2023
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
github-actions bot pushed a commit to kiennq/swiper that referenced this issue Jul 15, 2023
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
github-actions bot pushed a commit to kiennq/swiper that referenced this issue Aug 1, 2023
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
kiennq pushed a commit to kiennq/swiper that referenced this issue Oct 1, 2023
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
kiennq pushed a commit to kiennq/swiper that referenced this issue Nov 29, 2023
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
kiennq pushed a commit to kiennq/swiper that referenced this issue Mar 22, 2024
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
kiennq pushed a commit to kiennq/swiper that referenced this issue May 15, 2024
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
github-actions bot pushed a commit to kiennq/swiper that referenced this issue May 22, 2024
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
kiennq pushed a commit to kiennq/swiper that referenced this issue Jul 6, 2024
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
kiennq pushed a commit to kiennq/swiper that referenced this issue Aug 31, 2024
Fixes abo-abo#1218

Adds an extra check to `ivy--queue-exhibit` to only debounce if the input has
actually changed. It also maintains the previous input variable to match against.
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.