Tune File Notify Watchers to share directory file descriptors

thejeffphil commented 1 year ago

The same file descriptors will be shared across same server watchers.
When watcher cap unregs, only watcher unique descriptors are rm'd.
When directory created event, add a new watcher for it.
Send eglot--error to eglot-message, like eglot--warn aleady does.
Discussion: https://github.com/joaotavora/eglot/discussions/1226

Note: Eglot is part of Emacs, this PR is just for discussion purposes on https://github.com/joaotavora/eglot/discussions/1226. And if discussion goes well, I will complete necessary required paperwork.

joaotavora commented 1 year ago

This is very interesting, but please explain again what current wasteful behaviour is with a toy example, so that I can understand it.

Then explain how your implementation solves this problem.

jeff-phil commented 1 year ago

Added the "toy example" testing items for demonstration to setup a overly large python virtual environment. Then will load up a python file with eglot, and after each call of eglot-register-capability will output the number of file-notify-descriptors and memory.

Pull the branch: tune-file-watchers
Change to PR-tests-do_not_merge directory
Run the test-python.sh:
- First time this will create venv and pull lots of packages
- Will run emacs -Q --batch -l init.el
- Prompt if want to run with base installed eglot.el, or override with the one in branch

With pyright didChangeWatchedFiles will be requested twice from server, notice the second run doubles file descriptors and adds 15-20MB memory. Loading the patch, file descriptors are not increased due to sharing of descriptors.

Here is how it outputs for me, with first run using normal eglot.el, and second run using updated.

$ ./test-python.sh
Loading project (native compiled elisp)...
Loading eldoc (native compiled elisp)...
Loading seq (native compiled elisp)...
Loading flymake (native compiled elisp)...
Loading xref (native compiled elisp)...
Loading jsonrpc (native compiled elisp)...
Loading external-completion (native compiled elisp)...
Baseline stats...
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
******************************************
Total count file-notify-descriptors: 0
Total MB of memory (rss) Emacs is using: 96.06 MB
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
******************************************
Python Mode stats...
Do you want to load the "override" eglot.el vs. installed package? (y or n) n
[eglot] Connected! Server `EGLOT (python/(python-mode))' now managing `(python-mode)' buffers in project `python'.
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
******************************************
Total count file-notify-descriptors: 3742
Total MB of memory (rss) Emacs is using: 125.12 MB
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
******************************************
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
******************************************
Total count file-notify-descriptors: 7484
Total MB of memory (rss) Emacs is using: 154.38 MB
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
******************************************

$ ./test-python.sh
Loading project (native compiled elisp)...
Loading eldoc (native compiled elisp)...
Loading seq (native compiled elisp)...
Loading flymake (native compiled elisp)...
Loading xref (native compiled elisp)...
Loading jsonrpc (native compiled elisp)...
Loading external-completion (native compiled elisp)...
Baseline stats...
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
******************************************
Total count file-notify-descriptors: 0
Total MB of memory (rss) Emacs is using: 96.22 MB
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
******************************************
Python Mode stats...
Do you want to load the "override" eglot.el vs. installed package? (y or n) y
Loading ../eglot.el (source)...
Loading project (native compiled elisp)...
Loading eldoc (native compiled elisp)...
Loading seq (native compiled elisp)...
Loading flymake (native compiled elisp)...
Loading xref (native compiled elisp)...
Loading jsonrpc (native compiled elisp)...
Loading external-completion (native compiled elisp)...
[eglot] Connected! Server `EGLOT (python/(python-mode))' now managing `(python-mode)' buffers in project `python'.
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
******************************************
Total count file-notify-descriptors: 3742
Total MB of memory (rss) Emacs is using: 127.88 MB
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
******************************************
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
******************************************
Total count file-notify-descriptors: 3742
Total MB of memory (rss) Emacs is using: 140.72 MB
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
******************************************

I have included rust and js LSP's in the update, just for extra info. Rust LSP also sends the didChangeWatchedFiles request twice, but the id is the same which means the file descriptors are not duped. The typescript-language-server does not send the didChangeWatchedFiles more than once. I could not find any other Python LSP's that implemented file watchers to compare there.

Anyway, just wanted to finalize this for you since you requested and so you have it.

joaotavora commented 1 year ago

Sorry, this is too complicated for me to run and decipher. I didn't explain myself, a toy example as I envisioned it couldstart from the MRE (the bit that contains an Emacs -Q call) that was eventually supplied in #1226, and then teach me how I can see in an Eglot events log that the server is over-watching files.

It might not me necessary at this point, as I think I have understood the problem sufficiently well,

But it might still be a nice-to-have. So we could start with this.

mkdir mre
cd mre
pip3 install pyright
touch silly-file.txt
emacs test.py -Q -f toggle-debug-on-error -f eglot
# Now explain to me in English what Emacs actions 
# (say switching to Eglot events buffer and  searching 
# for specific bits of text) to take  # to observe that Eglot is 
# behaving suboptimally.
# 
# You can use minimal Emacs lisp code to express your 
# intentions, or add debug code, like advising eglot-register-capability 
# but if it's anything more than 10 lines I probably
# won't find the time to test it.

jeff-phil commented 1 year ago

Oops, I definitely over thought toy example.

This is probably as tight as I can get it.

mkdir mre
cd mre
python3 -m venv .venv
source .venv/bin/activate 
pip3 install pyright numpy numpydoc
touch test.py
##
emacs -Q

Once inside of emacs, paste this into scratch and eval-buffer:

(require 'package)
(package-initialize)
(let ((default-directory package-user-dir))
  (normal-top-level-add-subdirs-to-load-path))
(require 'eglot)
(add-to-list 'eglot-server-programs '(python-mode . ("pyright-langserver" "--stdio")))
(advice-add 'eglot-register-capability :after #'(lambda(&rest args)(message "Total count file-notify-descriptors: %s" (hash-table-count file-notify-descriptors))))
(find-file "test.py")
(call-interactively 'eglot)
(switch-to-buffer (messages-buffer))

In Messages buffer, you should see something like:

Total count file-notify-descriptors: 654
Total count file-notify-descriptors: 1308

When that number doubles, that means the file notifiers are duplicated.

You can then load the eglot.el from this PR, and restart eglot and you should just see this:

Total count file-notify-descriptors: 654 [2 times]

which means the file notifiers were not doubled.

joaotavora commented 1 year ago

Thanks, this is good and a good test. In fact, it would be easy to make this an automated test

joaotavora commented 1 year ago

I've now pushed changes to the same effect to Emacs master, so we should get these enhancements in the next Eglot release:

joaotavora / eglot

Tune File Notify Watchers to share directory file descriptors #1228