-
Notifications
You must be signed in to change notification settings - Fork 37
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
base: master
Are you sure you want to change the base?
Conversation
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>
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. |
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. |
This is why I switched away from projectile. It lead to too many performance issues, and |
|
Note that your benchmark might not be representative. There are two issues with 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 This may be affected by |
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. |
Have you tried to adjust |
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. |
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 |
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. |
Sorry for being late. After I restarting my Emacs, the performance issue seems gone now. There are two other issues or suggestions.
which is currently not that elegant... But maybe we can add an other option to control whether to care about project ( |
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. |
Adding an option (something like
|
Looks like a good idea to me 👍 |
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:
'abbrev
as it doesn't require the project root in most cases.This commit fixes all the performance problems of #56 for me.