-
Notifications
You must be signed in to change notification settings - Fork 200
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
Emacs freezes (~10s) with big node_modules directory when starting eglot #697
Comments
[ You skip some parts of the template that the template asks you not to skip. You are missing a minimal reproduction recipe. I need to be able to reproduce the error. Please, next time, don't skip the parts that say DO NOT SKIP. Alternatively, open a discussion] Notheless I think this might be related to #645 or #633. Is this project publicly available somewhere? |
@melias122 do you have a tsconfig.json/jsconfig.json file? can you post it? |
@joaotavora, sorry for that. Repository is private, but I will try to make reproducible repo. I will also try
|
@melias122 I wish you could also try the I'd be very thankful if you can test this, since the issuers of the other issues didn't follow through. |
Just tried |
That's an idea... |
|
Yes, lsp has knobs for a lot of things, but asking the user to customize yet another knob in what should be an out-of-box experience is sub-optimal. So if following .gitignore works for the majority of cases, I welcome that simplicity. |
Simplicity is why I love eglot. As far as logic for specific version control systems, wonder if there is some inspiration (or code) to be found in Projectile VCS support. |
Yes, Dmitry Gutov has suggested using project.el for this, so this is
probably what I'll try soon.
…On Wed, May 26, 2021, 16:12 Jarred Ward ***@***.***> wrote:
Simplicity is why I love eglot. As far as logic for specific version
control systems, wonder if there is some inspiration (or code) to be found
in Projectile VCS support
<https://docs.projectile.mx/projectile/2.3/projects.html#version-control-systems>
.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#697 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAC6PQYB75YYKOQRFUO5Q6LTPUFVRANCNFSM45SBZGLA>
.
|
The directory-finding logic is probably a bit slower than using eglot--directories-recursively, but since it honours `.gitignores` and ignores more directories it's much faster overall. And guaranteed to create less watchers. Thanks to Dmitry Gutov <[email protected]> for the idea. * eglot.el (eglot--directories-recursively): Remove.
Pushed another commit making use of project.el for this. It's already a dependency of |
Tried, now emacs pauses for ~2 seconds (which is not bad, but still there is pause), but on 835aa0e, there was no pause. My root .gitignore has:
With empty .gitignore it is also ~2 seconds. Any clue why? |
Try evaluating these: (benchmark 1 '(project-files (project-current))) and (benchmark 1 '(delete-dups (mapcar #'file-name-directory (project-files (project-current))))) ...and tell us the numbers. |
Hmm, I used @dgutov 's suggestion which is faster in some cases but probably slower in others. For example in a full repo of the As I wrote:
I guess, I'll have to some a bit more work to try and get the best of both worlds. |
@dgutov my theory if you have a directory with many many files, calling |
It is slower than OTOH, it's possible that the list of directories returned by the new approach is longer, and you end up spending more time setting up watches. |
It's likely shorter, since it ignores more stuff. In the snowpack (https://www.snowpack.dev/) repo your approach was faster by about 3x than |
It's definitely not the most optimal approach, but it shouldn't be too terrible for a one-time action on most small-to-moderate sized projects.
We can add a new defgeneric, sure. Anyone is welcome to suggest an optimal implementation of it for a Git backend. |
Following up to that, if @melias122 's
I'm not sure about the need for I just think that if If I manage that, I guess, |
But how to match them? What datatype exactly is "glob" pattern, is it a regexp? Oh I see they are actual glob strings, presumably of the My current thinking is to use In the meantime, I'm reverting the last commit. |
Don't forget about the extra overhead of several Lisp calls for every directory you are traversing. So I wouldn't be sure.
A new generic function acting on project instances.
Or of |
I'm reasonably sure that's the case. The number of directories is dramatically smaller than the number of file. So if all other things equal, traversing directories is much faster. The snowpack repo example was especially benevolent to your approach, likely because of the large However my (defun eglot--directories-recursively (&optional dir)
"Because `directory-files-recursively' isn't complete in 26.3."
(cons (setq dir (expand-file-name (or dir default-directory)))
(cl-loop with default-directory = dir
with completion-regexp-list = '("^[^.]")
for f in (file-name-all-completions "" dir)
if (file-directory-p f)
append (eglot--directories-recursively f))))
(defun eglot--directories-recursively-project (&optional dir)
(delete-dups (mapcar #'file-name-directory
(project-files
(or (project-current nil dir)
(cons 'transient dir))))))
(cl-set-difference
(eglot--directories-recursively-project "~/tmp/")
(eglot--directories-recursively "~/tmp/")
:test 'string=) ; nil
(length (eglot--directories-recursively "~/tmp/")); 678
(length (eglot--directories-recursively-project "~/tmp/")) ; 624 not sure why less...
(benchmark 10 '(eglot--directories-recursively-project "~/tmp/")) ; "Elapsed time: 3.594734s (0.419492s in 10 GCs)"
(benchmark 10 '(eglot--directories-recursively "~/tmp/")); "Elapsed time: 1.836761s (0.382498s in 9 GCs)" Another way to go about this might be to simply go into (length (project-files (cons 'transient "~/tmp"))); 678
(benchmark 10 '(project-files (cons 'transient "~/tmp"))); "Elapsed time: 2.580373s" So even
|
Final thought on this:
|
The more ignore entries there are, the slower the
No objections from me, but that's why I asked @melias122 to run a couple of basic benchmarks. There might be something else going on.
A |
Also, when you were testing in |
@dgutov I have |
I think the double star notation is not in all versions of Git. Maybe your
shell is picking up one version and project.el is using another. Try just
"node_modules".
João
…On Thu, May 27, 2021, 13:28 Martin Eliáš ***@***.***> wrote:
@dgutov <https://github.com/dgutov> I have **/node_modules in root
.gitignore and also in adminapp/.gitignore there is node_modules.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#697 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAC6PQ3N3HMFVDKWWYR5URTTPY3FFANCNFSM45SBZGLA>
.
|
I don't know, either seems sufficient to have the directory ignored in my local testing. Even if just |
@joaotavora I tried both @dgutov why would we want to have Looking in
|
That would be a mistake, yes, but it's the only case I know of where Git ignores its own OTOH, you said that
That only affects what Could you maybe You can also try calling |
Hmm, I think I found whats wrong. I noticed different results for I have this in my config (in use-pacage projectile). I am not sure why this is causing troubles:
|
ah! nice. Kill that and we're good :-). Or maybe put it later in the list (what good is it anyway?). |
It's a poor definition: you created a backend, based on Projectile's returned root, which has no ignores aside from the hardcoded standard ones. The file listing, instead of delegating to Projectile, just uses the default definition (based on And it also hijacks the symbol If you really do use it for something, here's a better definition (EDITED): (defun m/projectile-project-find-function (dir)
(let ((root (projectile-project-root dir)))
(and root (cons 'my/projectile root))))
(cl-defmethod project-root ((pr (head my/projectile)))
(cdr pr))
(cl-defmethod project-files ((pr (head my/projectile)) &optional _dirs)
(let ((root (cdr pr)))
(mapcar
(lambda (file)
(concat root file))
(projectile-project-files root))))
(cl-defmethod project-ignores ((pr (head my/projectile)) _dir)
(let ((default-directory (cdr pr)))
(projectile-patterns-to-ignore)))
(with-eval-after-load 'project
(add-to-list 'project-find-functions 'm/projectile-project-find-function)) Feel free to ping Projectile's maintainer in bbatsov/projectile#1591, though. |
…ory watching Previously, given a number of globs, Eglot would try to place system watchers only in those subdirectories that could potentially be matched by a glob. This meant traversing the whole tree, which could be impractical. Just place watchers in every subdirectory of the project (you may run out of watchers). * eglot.el (eglot-register-capability): Simplify. (eglot--files-recursively): Delete. (eglot--directories-recursively): Fix.
…n to "node_modules" directores * eglot.el (eglot--directories-recursively): Fix.
…atchers to skip The directory-finding logic is probably a bit slower than using eglot--directories-recursively, but since it honours `.gitignores` and ignores more directories it's much faster overall. And guaranteed to create less watchers. Thanks to Dmitry Gutov <[email protected]> for the idea. * eglot.el (eglot--directories-recursively): Remove.
…ory watching Previously, given a number of globs, Eglot would try to place system watchers only in those subdirectories that could potentially be matched by a glob. This meant traversing the whole tree, which could be impractical. Just place watchers in every subdirectory of the project (you may run out of watchers). * eglot.el (eglot-register-capability): Simplify. (eglot--files-recursively): Delete. (eglot--directories-recursively): Fix.
…n to "node_modules" directores * eglot.el (eglot--directories-recursively): Fix.
…atchers to skip The directory-finding logic is probably a bit slower than using eglot--directories-recursively, but since it honours `.gitignores` and ignores more directories it's much faster overall. And guaranteed to create less watchers. Thanks to Dmitry Gutov <[email protected]> for the idea. * eglot.el (eglot--directories-recursively): Remove.
Previously, given a number of globs, Eglot would try to place system watchers only in those subdirectories that could potentially be matched by a glob. This meant traversing the whole tree, which could be impractical. Just place watchers in every subdirectory of the project (you may run out of watchers). * eglot.el (eglot-register-capability): Simplify. (eglot--files-recursively): Delete. (eglot--directories-recursively): Fix. #697: joaotavora/eglot#697 #645: joaotavora/eglot#645
* eglot.el (eglot--directories-recursively): Fix. #697: joaotavora/eglot#697 #645: joaotavora/eglot#645
The directory-finding logic is probably a bit slower than using eglot--directories-recursively, but since it honours `.gitignores` and ignores more directories it's much faster overall. And guaranteed to create less watchers. Thanks to Dmitry Gutov <[email protected]> for the idea. * eglot.el (eglot--directories-recursively): Remove. #697: joaotavora/eglot#697
Previously, given a number of globs, Eglot would try to place system watchers only in those subdirectories that could potentially be matched by a glob. This meant traversing the whole tree, which could be impractical. Just place watchers in every subdirectory of the project (you may run out of watchers). * eglot.el (eglot-register-capability): Simplify. (eglot--files-recursively): Delete. (eglot--directories-recursively): Fix. GitHub-reference: fix joaotavora/eglot#697 GitHub-reference: fix joaotavora/eglot#645
* eglot.el (eglot--directories-recursively): Fix. GitHub-reference: per joaotavora/eglot#697 GitHub-reference: per joaotavora/eglot#645
The directory-finding logic is probably a bit slower than using eglot--directories-recursively, but since it honours `.gitignores` and ignores more directories it's much faster overall. And guaranteed to create less watchers. Thanks to Dmitry Gutov <[email protected]> for the idea. * eglot.el (eglot--directories-recursively): Remove. GitHub-reference: per joaotavora/eglot#697
Joao and Dmitry hi, Could you guys please tell: 1, is there a way to see what files are "monitored" by eglot/pyright? (i want to make sure that my settings are correct and only needed files are there (as few as possible for performance reasons)) 2, for following project structure:
What is the best way (for speed/perfromance) to tell eglot/pyright to "watch" only /scripts/ folder? As i understand i can put it into pyrightconfig.json:
or (judging from this thread) into .gitignore
|
That doesn't sound like a project management question, so I'll leave it to Joao. |
Please post the contents of your Eglot events buffer. Or better yet find the message that asks Eglot to watch files, must be some "didChangeWatchedFiles" from server to client. It's not clear what problem you are experiencing. Not all servers ask Eglot to watch files, and it's not clear how that impacts performance, assuming that's what you're worried about. |
1 similar comment
Please post the contents of your Eglot events buffer. Or better yet find the message that asks Eglot to watch files, must be some "didChangeWatchedFiles" from server to client. It's not clear what problem you are experiencing. Not all servers ask Eglot to watch files, and it's not clear how that impacts performance, assuming that's what you're worried about. |
Actually, the server to client message is of type "register Capability" with "didChangeWatchedFiles" being one of its arguments |
@joaotavora upon further checking i think problem is on my side i will try to troubleshoot it, and repost if i still have lagging/freezing issues. Thank you for quick replies. |
Hey I am trying to move from lsp-mode to eglot but I have some issues one of the projects I currently work on. As title says when starting eglot in project containing node_modules (~50k files), emacs freezes for some time. When I remove
node_modules emacs does not freeze. Just to note, lsp-mode does not have this issue.
LSP transcript - M-x eglot-events-buffer (mandatory unless Emacs inoperable)
The text was updated successfully, but these errors were encountered: