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

Improve buffer listing performance with projectile #57

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rgrinberg
Copy link

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 added 3 commits May 14, 2019 01:19
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.
In most situations, this functions relies on projectile. Unfortunately, getting
the project name with projectile can be quite costly and hence it should not be
done by default.
The relative style requires to always access the project root. This can be quite
costly in conjunction with projectile. 'abbrev on the other hand is much faster
by default.

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
Copy link
Author

rgrinberg commented May 13, 2019

I believe that syl20bnr/spacemacs#10101 can be closed if this PR is accepted. I use macosx and the performance is satisfactory with this PR.

@rgrinberg
Copy link
Author

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.

@CeleritasCelery
Copy link
Contributor

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

@Yevgnen
Copy link
Owner

Yevgnen commented May 14, 2019

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)

@rgrinberg
Copy link
Author

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.

@Yevgnen
Copy link
Owner

Yevgnen commented May 14, 2019

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.

@rgrinberg
Copy link
Author

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.

@Yevgnen
Copy link
Owner

Yevgnen commented May 14, 2019

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

@rgrinberg
Copy link
Author

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.

@Yevgnen
Copy link
Owner

Yevgnen commented May 14, 2019

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.

@rgrinberg
Copy link
Author

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.

@Yevgnen
Copy link
Owner

Yevgnen commented May 23, 2019

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.

@rgrinberg
Copy link
Author

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.

@Yevgnen
Copy link
Owner

Yevgnen commented May 25, 2019

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.

@rgrinberg
Copy link
Author

Looks like a good idea to me 👍

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

Successfully merging this pull request may close these issues.

3 participants