ivy-rich icon indicating copy to clipboard operation
ivy-rich copied to clipboard

Improve buffer listing performance with projectile

Open rgrinberg opened this issue 5 years ago • 15 comments

The code that gets the buffer path referred to projectile far too eagerly. Only when the file path of the buffer wasn't known it was necessary to get the projectile root.

This commit makes use of the above to fetch the buffer path and the project root in separate functions. This is slower in somse cases where both values are required, but is much faster in the majority of cases.

The old code path for whenever ivy-rich-path-style isn't one of full, abbrev is retained as it's more difficult to implement the optimization correctly in those clauses.

The last 2 commits change the defaults so that users always get a responsive user experience out of the box:

  • Remove the project name from the default list of columns
  • Change the style to 'abbrev as it doesn't require the project root in most cases.

This commit fixes all the performance problems of #56 for me.

rgrinberg avatar May 13 '19 17:05 rgrinberg

I believe that https://github.com/syl20bnr/spacemacs/issues/10101 can be closed if this PR is accepted. I use macosx and the performance is satisfactory with this PR.

rgrinberg avatar May 13 '19 17:05 rgrinberg

It's a bit of a shame that we're adding these hacks for projectile, and perhaps project.el doesn't suffer from these performance problems. But I think that projectile is easily more popular than project.el (see spacemacs for example), so I still think it's the right default.

rgrinberg avatar May 13 '19 17:05 rgrinberg

This is why I switched away from projectile. It lead to too many performance issues, and project.el is very fast.

CeleritasCelery avatar May 13 '19 18:05 CeleritasCelery

project.el seems generally slower than projectile on my machine

(benchmark-run 10000 (projectile-project-root))  ; (2.690558 2 0.34833799999999826)
(benchmark-run 10000 (project-current))          ; (5.332584 3 0.5320549999999997)

Yevgnen avatar May 14 '19 02:05 Yevgnen

Note that your benchmark might not be representative. There are two issues with projectile:

  • It will not cache (projectile-project-root) whenever a buffer doesn't have project. This may really down ivy-rich depending on the use case. I quite often open buffers that don't belong to any project for example.

  • Projectile will do hundreds of file existence syscalls in some pessimistic cases to discover the root via its various functions and combinations of custom project types (see projectile-project-type). So if your hard drive isn't very fast, you will feel the slow down much more. And as I've said, its oddball caching doesn't help in a key use case.

rgrinberg avatar May 14 '19 02:05 rgrinberg

It's a bit of a shame that we're adding these hacks for projectile

I'm not sure what do you mean by 'these hacks'. The 'truename' part is used to make sure the relative path is computed correctly and I think it should be used everywhere. The project.el way indeed may give strange result when user open a file /Users/user/dotfile/emacs/init/init-git.el and /Users/user/.emacs.d/init/init-editor.el where /Users/user/.emacs.d/ is a symbolic link to /Users/user/dotfile/emacs/.

Screen Shot 2019-05-14 at 10 37 56

This may be affected by vc-follow-symlinks when opening files. But since file-truename is a slow function and the issue is generally not big, I didn't fix it.

Yevgnen avatar May 14 '19 02:05 Yevgnen

I'm not sure what do you mean by 'these hacks'.

I mean the hacks I'm adding in this PR. I think the old code was more clear and it would be nice to have the project name in the columns list and relativize the paths by default. But unfortunately, projectile is the default project manager doesn't allow these defaults to work well for all users.

rgrinberg avatar May 14 '19 02:05 rgrinberg

Have you tried to adjust projectile-project-root-files-functions and did it help?

Yevgnen avatar May 14 '19 02:05 Yevgnen

Indeed it would help, but I would lose functionality. I have some project setups where the various project discovery settings are useful. Also, it will not fix the problem for other users who may be unwilling to change, or unaware of this setting. Things should be fast by default. I think this issue is one of the main causes why ivy-rich was disabled on spacemacs for example.

rgrinberg avatar May 14 '19 03:05 rgrinberg

I'll take some time to look into code later, please wait. But after a quick try, the fix seem make bare buffers (e.g. the ones in /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include) slower. The ones in projects looks good.

Yevgnen avatar May 14 '19 03:05 Yevgnen

I'll take some time to look into code later, please wait. But after a quick try, the fix seem make bare buffers (e.g. the ones in /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include) slower. The ones in projects looks good.

Hmm, that definitely shouldn't be the case. In fact, the patch is designed exactly for this use case. Where you should expect a slow down is for bare buffers that are not associated with a file, e.g. dired buffers. This is because there's no more sharing of code to get the root and the filename of the buffer. However, this was insignificant in my measurements

What is your ivy-rich-path-style? My optimization only applies to abbrev and absolute styles.

rgrinberg avatar May 14 '19 05:05 rgrinberg

Sorry for being late.

After I restarting my Emacs, the performance issue seems gone now. There are two other issues or suggestions.

  1. The abbreviate style seems not apply to bare buffers, e.g.
Screen Shot 2019-05-23 at 12 04 47
  1. I would suggest to keep ivy-rich-switch-buffer-project since adding it back by projectile users may require a large block of setq like
(setq ivy-rich-display-transformers-list
      '(counsel-projectile-switch-project
        (:columns ((ivy-rich-counsel-projectile-switch-project-project-name (:width 30 :face success))
                   (ivy-rich-candidate)))
        ivy-switch-buffer
        (:columns ((ivy-rich-switch-buffer-candidate (:width 0.25))
                   (ivy-rich-switch-buffer-size (:width 0.05))
                   (ivy-rich-switch-buffer-indicators (:width 0.05 :face error :align right))
                   (ivy-rich-switch-buffer-major-mode (:width 0.1 :face warning))
                   (ivy-rich-switch-buffer-project (:width 0.15 :face success))
                   (ivy-rich-switch-buffer-path (:width (lambda (x)
                                                          (ivy-rich-switch-buffer-shorten-path x (ivy-rich-minibuffer-width 0.4))))))
                  :predicate
                  (lambda (cand)
                    (get-buffer cand)))
        ...
        counsel-projectile-switch-project
        (:columns
         ((ivy-rich-counsel-projectile-switch-project-project-name (:width 30 :face success))
          (ivy-rich-candidate)))))

which is currently not that elegant... But maybe we can add an other option to control whether to care about project (projectile / project.el /... ) info and return "" when it's nil, i.e., don't what project info. That maybe a good default for both projectile and non projectile users.

Yevgnen avatar May 23 '19 04:05 Yevgnen

I agree that the customization above looks quite unattractive and extremely unlikely that any users would come up with it.

Adding an option should be fine. Although I insist that the default should be performant. The vast majority of users will never look far enough to understand their performance issues with Emacs.

Would it be possible to add some sort of variable for the specification of the projectile column? This way, users will only need to know of this variable rather than the huge specification.

rgrinberg avatar May 24 '19 17:05 rgrinberg

Adding an option (something like ivy-rich-(switch-buffer)?-project-info-p to default nil should make the package performant. Just return "" in the column function when it's nil. Docstring of ivy-rich-display-transformers-list:

COLUMN-FN is a function which takes the completion candidate as single argument and it should return a transformed string. This function should return an empty string "" instead of nil when the transformed string is empty.

Yevgnen avatar May 25 '19 02:05 Yevgnen

Looks like a good idea to me 👍

rgrinberg avatar May 26 '19 08:05 rgrinberg