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.

Sign up for GitHub

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

Jump to bottom

projectile-project-root is slow when not in project - not caching negative results? #1755

Closed
garyo opened this issue Feb 23, 2022 · 0 comments
Closed

projectile-project-root is slow when not in project - not caching negative results? #1755

garyo opened this issue Feb 23, 2022 · 0 comments
Labels
Good First Issue Improvement

Comments

@garyo
Copy link

garyo commented Feb 23, 2022

Expected behavior

Calling projectile-project-root in a deep subdir that's not in a project should be fast, especially after the first call

Actual behavior

It can be slow; .5 to 1 sec per call.

Steps to reproduce the problem

I have a dir /mnt/c/files/consulting/project/projectA/ containing foo.txt. In that buffer, invoke (projectile-project-root). It takes about half a second on a fast machine.
The reason seems to be that the negative project result does not get cached, so every time it's called it calls projectile-root-top-down, which is slow.

The code in projectile.el has a comment indicating that negative results should be cached as 'none, but that does not seem to be actually happening:

                  ;; set cached to none so is non-nil so we don't try
                  ;; and look it up again

To reproduce:

Start emacs -q -l /tmp/emacs-test-startup.el with this emacs-test-startup.el:

;;; Set up package system -- straight.el
(defvar bootstrap-version)
(or (boundp 'native-comp-deferred-compilation-deny-list)
    (setq native-comp-deferred-compilation-deny-list '()))

(let ((bootstrap-file
       (expand-file-name "straight/repos/straight.el/bootstrap.el" user-emacs-directory))
      (bootstrap-version 5))
  (unless (file-exists-p bootstrap-file)
    (with-current-buffer
        (url-retrieve-synchronously
         "https://raw.githubusercontent.com/raxod502/straight.el/develop/install.el"
         'silent 'inhibit-cookies)
      (goto-char (point-max))
      (eval-print-last-sexp)))
  (load bootstrap-file nil 'nomessage))

;;; Meta-package system: use-package. Auto-installs and configures packages.
(straight-use-package 'use-package)
(defvar straight-use-package-by-default)
(setq straight-use-package-by-default t) ; make use-package use straight

(use-package projectile
  :config
  (projectile-mode))

Then create and find the file /mnt/c/files/consulting/project/projectA/foo.txt. Invoke (projectile-project-root); it should take around half a second. Then invoke it again; it will still take half a second.

If you edebug projectile-project-root and step through it, you can see it never caches the 'none value so each time it tries all the projectile-project-root-functions, and some of them (e.g. projectile-root-top-down, which calls projectile-locate-dominating-file) are slow.

Environment & Version information

Linux Emacs nightly of 2022-01-24, with native comp enabled.
Running in WSL2 on Windows 11 with an AMD Ryzen Threadripper CPU.

Projectile version information

25.0, latest release

Include here the version string displayed by M-x projectile-version.

Projectile nil

(I use straight.el; I have the tag 25.0.)

Emacs version

GNU Emacs 29.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version 3.24.20,
cairo version 1.16.0) of 2022-01-24

Operating system

WSL2, Ubuntu 20.04, on Windows 11
tomheon added a commit to tomheon/projectile that referenced this issue Jun 1, 2022
@tomheon
[Fix bbatsov#1755] Cache failure to find project root
164147a 
When `projectile-project-root` was unable to find a project root for a
directory, it would not cache the negative result, so future invocations would
repeat the (often expensive) search for a project root every time.

With this change, `projectile-project-root` caches failure to find a project
root, when that failure is expected to be permanent, and consults that cache
for speed on subsequent invocations.  "Expected to be permanent" means that
either we're trying to find the project root of a local directory, or we're
successfully connected to a remote directory via TRAMP.  If the directory isn't
local, but we can't connect to it, we consider that a transient failure and
don't cache it.

Under the hood, this change uses the same `projectile-project-root-cache`
that's used to cache successful attempts to find a project root, but with a new
key `projectilerootless-{dir}`.  This should allow cache invalidation to work
as expected.

-----------------

Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

- [X] The commits are consistent with our [contribution guidelines](../blob/master/CONTRIBUTING.md)
- [X] You've added tests (if possible) to cover your change(s)
- [X] All tests are passing ([`eldev test`](https://github.com/doublep/eldev))
- [X] The new code is not generating bytecode or `M-x checkdoc` warnings

Since this is not a user-visible change, I don't believe either of the
following are needed:

- [ ] You've updated the [changelog](../blob/master/CHANGELOG.md) (if adding/changing user-visible functionality)
- [ ] You've updated the readme (if adding/changing user-visible functionality)
tomheon added a commit to tomheon/projectile that referenced this issue Jun 1, 2022
@tomheon
squash! [Fix bbatsov#1755] Cache failure to find project root
e9e2a70
tomheon added a commit to tomheon/projectile that referenced this issue Jun 1, 2022
@tomheon
[Fix bbatsov#1755] Cache failure to find project root
fd723ea 
When `projectile-project-root` was unable to find a project root for a
directory, it would not cache the negative result, so future invocations would
repeat the (often expensive) search for a project root every time.

With this change, `projectile-project-root` caches failure to find a project
root, when that failure is expected to be permanent, and consults that cache
for speed on subsequent invocations.  "Expected to be permanent" means that
either we're trying to find the project root of a local directory, or we're
successfully connected to a remote directory via TRAMP.  If the directory isn't
local, but we can't connect to it, we consider that a transient failure and
don't cache it.

Under the hood, this change uses the same `projectile-project-root-cache`
that's used to cache successful attempts to find a project root, but with a new
key `projectilerootless-{dir}`.  This should allow cache invalidation to work
as expected.

-----------------

Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

- [X] The commits are consistent with our [contribution guidelines](../blob/master/CONTRIBUTING.md)
- [X] You've added tests (if possible) to cover your change(s)
- [X] All tests are passing ([`eldev test`](https://github.com/doublep/eldev))
- [X] The new code is not generating bytecode or `M-x checkdoc` warnings

Since this is not a user-visible change, I don't believe either of the
following are needed:

- [ ] You've updated the [changelog](../blob/master/CHANGELOG.md) (if adding/changing user-visible functionality)
- [ ] You've updated the readme (if adding/changing user-visible functionality)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Good First Issue Improvement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bbatsov @garyo