* bug#70036: 30.0.50; Move file-truename to the C level
@ 2024-03-27 19:08 Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-27 19:44 ` Eli Zaretskii
` (3 more replies)
0 siblings, 4 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-27 19:08 UTC (permalink / raw)
To: 70036
[-- Attachment #1: Type: text/plain, Size: 2405 bytes --]
Hi, all!
During the last couple of weeks I've been studying Eglots performance
and have been noticing a couple of things that I find very
interesting. It seems like `file-truename` is in the hot path due to the
fact that every request to the lsp server has to create the source file
location, and in every response we have to parse the location the
relevant file. `file-truename` is used for this, and its performance
isn't really up to snuff for what it provides, afaict.
Below I've supplied some benchmarks and profile reports along with the
actual patch. Before we discuss the patch itself, I want to get some
answers to the following:
- Is there a reason that this function should be supplied at the lisp
level?
- Does it have to be recursive? It seems to eat up a lot of stack, and
the comments in the file suggest that has been an issue before.
Firstly, I'll show some benchmarks
```
;; Emacs 29 branch
(benchmark-run 10000
(file-truename "~/Work/some/long/path/to/parse/that/is/very/deep/deep/deep/super/duper/deep/deep.el"))
;; (1.810892642 1 0.051070616)
;; With new C implementation
(benchmark-run 10000
(file-truename "~/Work/some/long/path/to/parse/that/is/very/deep/deep/deep/super/duper/deep/deep.el"))
;; (0.018811035 0 0.0)
```
As you can see, the C implementation, though naive for now is two orders
of magnitude faster, and makes a noticeable difference when running an
lsp server in emacs.
As for the patch - it now relies on wordexp to resolve the paths, and I
believe there is no real feature parity with the old variant as for now,
but I haven't seen any issues thus far. If this approach is accepted I
will of course make sure we have feature parity, unless that isn't
wanted.
As for the profiles - it is very clear the performance is better with my
version, as it doesn't really show up in the profiles, but in the
current state `file-truename` seems to eat up around 10-20% of the total
samplings.
And lastly - I've noticed that `redisplay_internal (C function)` shows
up as a _huge_ chunk in emacs 30, but not in emacs 29. Is this a known
issue, or something to look out for? I could open a different bug report
for this if needed.
Below are the profiles and the patch. On my system I needed to `ln -s
lisp/loadup.el .` to make it compile. Not sure if that is due to
differences between old and new `file-truename`, or something else.
Thanks,
Theo
[-- Attachment #2: emacs-29-before-everything --]
[-- Type: application/octet-stream, Size: 11650 bytes --]
65 31% - jsonrpc--process-filter
61 29% - jsonrpc-connection-receive
49 24% - #<compiled -0xbf50d04bcfd4f4b>
29 14% - eglot--hover-info
29 14% - mapconcat
29 14% - eglot--format-markup
14 6% - gfm-view-mode
10 4% - byte-code
4 1% - require
1 0% - defvar
1 0% - define-keymap
1 0% - keymap-set
1 0% - keymap--check
1 0% key-valid-p
6 2% - font-lock-ensure
3 1% - #<compiled -0x19e2d10a955dad87>
3 1% - font-lock-fontify-region
3 1% - c-font-lock-fontify-region
3 1% - font-lock-default-fontify-region
3 1% - font-lock-fontify-keywords-region
2 0% - c-font-lock-declarations
2 0% - c-find-decl-spots
2 0% - #<compiled -0xe9c9c536033cf90>
2 0% - c-forward-decl-or-cast-1
2 0% - c-forward-type
2 0% - c-forward-name
1 0% - c-forward-<>-arglist
1 0% - c-forward-<>-arglist-recur
1 0% - c-forward-type
1 0% c-forward-keyword-clause
1 0% - #<compiled 0xfe79bfcdb7014df>
1 0% c-beginning-of-decl-1
3 1% - font-lock-set-defaults
3 1% - font-lock-compile-keywords
3 1% - mapcar
3 1% - font-lock-compile-keyword
3 1% - eval
3 1% - list
3 1% - progn
2 0% - unless
2 0% - if
2 0% - c-face-name-p
2 0% - face-list
2 0% maphash
4 1% - java-mode
3 1% - c-common-init
2 0% - c-basic-common-init
2 0% - c-set-style
2 0% - mapc
2 0% - #<compiled 0x1170c390a66532e1>
2 0% - c-set-style-1
2 0% - mapcar
2 0% #<compiled 0x1892a0c8da397cc8>
1 0% - c-font-lock-init
1 0% - mapcar
1 0% c-mode-symbol
1 0% - c-init-language-vars-for
1 0% - c-make-keywords-re
1 0% regexp-opt
20 9% - #<compiled 0x138508d9b11649a6>
20 9% - #<compiled 0x3532fdd8350189a>
20 9% - run-hook-with-args
20 9% - eldoc-display-in-echo-area
18 8% - eldoc--message
18 8% - eldoc-minibuffer-message
18 8% - apply
13 6% message
2 0% - eldoc--echo-area-substring
2 0% substitute-command-keys
10 4% - jsonrpc--log-event
8 3% - pp-to-string
8 3% - pp-buffer
5 2% - down-list
5 2% syntax-ppss
2 0% - indent-sexp
2 0% - lisp-indent-calc-next
1 0% calculate-lisp-indent
1 0% up-list
1 0% - jsonrpc--events-buffer-scrollback-size
1 0% - apply
1 0% #<compiled -0x455ec6e2c68d407>
1 0% - #<compiled -0x22ca92508a0e1dc>
1 0% - mapcar
1 0% - #<compiled 0xda6c796629db7f7>
1 0% - eglot--range-region
1 0% eglot--lsp-position-to-point
2 0% - #<compiled -0x1c74af7641681def>
2 0% - kill-buffer
2 0% replace-buffer-in-windows
1 0% generate-new-buffer
60 29% - command-execute
60 29% - call-interactively
59 28% - byte-code
59 28% - read-extended-command
59 28% - read-extended-command-1
59 28% - completing-read
59 28% - completing-read-default
38 18% - read-from-minibuffer
13 6% - command-execute
9 4% - call-interactively
9 4% - funcall-interactively
9 4% - minibuffer-complete-and-exit
9 4% - completion-complete-and-exit
9 4% - completion--complete-and-exit
8 3% - try-completion
8 3% - #<compiled 0x1bf3e3a951e865d9>
8 3% complete-with-action
1 0% test-completion
1 0% - redisplay_internal (C function)
1 0% - eval
1 0% flymake--mode-line-counter
1 0% - funcall-interactively
1 0% - execute-extended-command
1 0% - command-execute
1 0% - call-interactively
1 0% - funcall-interactively
1 0% profiler-stop
55 26% - timer-event-handler
55 26% - apply
51 25% - #<compiled -0xdafdfebeede7b62>
51 25% - eldoc-print-current-symbol-info
51 25% - eldoc--invoke-strategy
51 25% - eldoc-documentation-compose
50 24% - eldoc--documentation-compose-1
50 24% - run-hook-wrapped
50 24% - #<compiled 0xf55a170ff80dfc9>
25 12% - eglot-signature-eldoc-function
18 8% - eglot--TextDocumentPositionParams
18 8% - eglot--TextDocumentIdentifier
18 8% - eglot--path-to-uri
15 7% - file-truename
14 6% - file-truename
14 6% - file-truename
11 5% - file-truename
11 5% - file-truename
11 5% - file-truename
10 4% - file-truename
10 4% - file-truename
8 3% - file-truename
8 3% - file-truename
8 3% - file-truename
5 2% - file-truename
3 1% - file-truename
2 0% - file-truename
1 0% file-truename
1 0% url-hexify-string
1 0% - file-local-name
1 0% file-remote-p
7 3% - jsonrpc-async-request
7 3% - apply
7 3% - jsonrpc--async-request-1
7 3% - jsonrpc-connection-send
7 3% - apply
7 3% - #<compiled -0x4abc9b84fa7c2a4>
6 2% - jsonrpc--log-event
6 2% - pp-to-string
4 1% - pp-buffer
3 1% - down-list
2 0% syntax-ppss
1 0% - indent-sexp
1 0% lisp-indent-calc-next
25 12% - eglot-hover-eldoc-function
11 5% - eglot--highlight-piggyback
7 3% - eglot--TextDocumentPositionParams
7 3% - eglot--TextDocumentIdentifier
7 3% - eglot--path-to-uri
6 2% - file-truename
5 2% - file-truename
4 1% - file-truename
4 1% - file-truename
4 1% - file-truename
3 1% - file-truename
3 1% - file-truename
3 1% - file-truename
2 0% - file-truename
2 0% - file-truename
1 0% file-truename
4 1% - jsonrpc-async-request
4 1% - apply
4 1% - jsonrpc--async-request-1
2 0% - jsonrpc-connection-send
2 0% - apply
2 0% - #<compiled -0x4abc9b84fa7c2a4>
2 0% - jsonrpc--log-event
2 0% - pp-to-string
1 0% pp-buffer
1 0% - #<compiled 0x1547f373ce8e51b2>
1 0% - run-with-timer
1 0% - apply
1 0% - run-at-time
1 0% timer-relative-time
1 0% - jsonrpc--next-request-id
1 0% - apply
1 0% #<compiled -0xdff803ba46c02e5>
10 4% - eglot--TextDocumentPositionParams
10 4% - eglot--TextDocumentIdentifier
10 4% - eglot--path-to-uri
8 3% - file-truename
8 3% - file-truename
8 3% - file-truename
8 3% - file-truename
7 3% - file-truename
7 3% - file-truename
6 2% - file-truename
4 1% - file-truename
3 1% - file-truename
1 0% file-truename
2 0% - url-hexify-string
2 0% - mapconcat
1 0% #<compiled 0x1e6ca0cb85a458bc>
4 1% - jsonrpc-async-request
4 1% - apply
4 1% - jsonrpc--async-request-1
4 1% - jsonrpc-connection-send
4 1% - apply
4 1% - #<compiled -0x4abc9b84fa7c2a4>
3 1% - jsonrpc--log-event
3 1% - pp-to-string
2 0% - pp-buffer
2 0% - indent-sexp
1 0% indent-line-to
1 0% lisp-indent-calc-next
1 0% jsonrpc--json-encode
3 1% #<compiled 0x12c6d7b54a1d6d26>
1 0% - show-paren-function
1 0% - show-paren--default
1 0% syntax-ppss
21 10% - eldoc-pre-command-refresh-echo-area
21 10% - eldoc--message
21 10% - eldoc-minibuffer-message
21 10% apply
3 1% - redisplay_internal (C function)
3 1% - eval
1 0% flymake--mode-line-counter
0 0% + ...
[-- Attachment #3: emacs-30-before --]
[-- Type: application/octet-stream, Size: 16375 bytes --]
209 43% - timer-event-handler
209 43% - apply
187 39% - #<compiled-function 978>
179 37% - jsonrpc-connection-receive
104 21% - #<compiled-function F7B>
104 21% - apply
104 21% - eglot-handle-notification
104 21% - apply
104 21% - #<compiled-function 604>
90 18% - find-buffer-visiting
73 15% - file-truename
61 12% - file-truename
58 12% - file-truename
51 10% - file-truename
46 9% - file-truename
43 9% - file-truename
41 8% - file-truename
36 7% - file-truename
34 7% - file-truename
28 5% - file-truename
22 4% - file-truename
15 3% - file-truename
10 2% - file-truename
5 1% - file-truename
1 0% file-truename
5 1% abbreviate-file-name
8 1% - eglot-uri-to-path
7 1% - url-generic-parse-url
2 0% - #<compiled-function 548>
2 0% kill-buffer
1 0% match-string
1 0% - eglot--trampish-p
1 0% - project-root
1 0% - apply
1 0% - #<compiled-function 5D8>
1 0% gethash
5 1% eglot--make-diag
1 0% expand-file-name
65 13% - jsonrpc--continue
63 13% - #<compiled-function A70>
56 11% - eglot--hover-info
56 11% - eglot--format-markup
40 8% - gfm-view-mode
13 2% - defalias
8 1% - file-name-sans-extension
1 0% file-name-sans-versions
9 1% - byte-code
8 1% - require
2 0% - defalias
2 0% file-name-sans-extension
1 0% - custom-declare-face
1 0% - face-spec-set
1 0% - face-spec-recalc
1 0% - face-attribute
1 0% face-attribute-relative-p
1 0% - custom-declare-variable
1 0% - custom-initialize-reset
1 0% - eval
1 0% - funcall
1 0% - #<compiled-function DF3>
1 0% - executable-find
1 0% locate-file
4 0% require
4 0% - read-only-mode
4 0% - view-mode-enter
1 0% - defalias
1 0% file-name-sans-extension
3 0% read
3 0% - gfm-mode
3 0% - markdown-mode
2 0% - syntax-propertize
2 0% - markdown-syntax-propertize
1 0% - markdown-syntax-propertize-list-items
1 0% - markdown--cur-list-item-bounds
1 0% - markdown-cur-list-item-end
1 0% - markdown-prev-line-blank
1 0% looking-at
1 0% - markdown-syntax-propertize-pre-blocks
1 0% format
1 0% add-hook
1 0% - custom-declare-face
1 0% - face-spec-set
1 0% - make-empty-face
1 0% make-face
13 2% - font-lock-ensure
13 2% - #<compiled-function 46D>
13 2% - font-lock-fontify-region
13 2% - font-lock-default-fontify-region
8 1% - font-lock-fontify-keywords-region
2 0% - markdown-fontify-inline-links
2 0% - markdown-match-generic-links
1 0% - markdown-range-property-any
1 0% number-sequence
1 0% re-search-forward
1 0% - markdown-match-pandoc-metadata
1 0% - markdown-match-generic-metadata
1 0% re-search-forward
1 0% - markdown-match-bold
1 0% - markdown-match-inline-generic
1 0% re-search-forward
1 0% - markdown-fontify-sub-superscripts
1 0% - markdown-search-until-condition
1 0% - apply
1 0% re-search-forward
1 0% - markdown-fontify-plain-uris
1 0% - markdown-match-plain-uris
1 0% - markdown-match-inline-generic
1 0% - markdown-code-block-at-pos
1 0% - markdown-get-enclosing-fenced-block-construct
1 0% cl-find-if
1 0% - markdown-match-code
1 0% - markdown-search-until-condition
1 0% - apply
1 0% re-search-forward
5 1% - font-lock-fontify-syntactically-region
5 1% - treesit-font-lock-fontify-region
4 0% treesit-parser-root-node
1 0% treesit--font-lock-fontify-region-1
3 0% - java-ts-mode
2 0% - treesit-major-mode-setup
2 0% - keymap-set
2 0% - keymap--check
2 0% - key-valid-p
2 0% string-match
1 0% - treesit-ready-p
1 0% treesit-language-available-p
7 1% - #<compiled-function 260>
6 1% - #<compiled-function AF0>
6 1% - run-hook-with-args
5 1% - eldoc-display-in-echo-area
3 0% - eldoc--message
3 0% - eldoc-minibuffer-message
3 0% - message
3 0% redisplay_internal (C function)
1 0% - eldoc-display-in-buffer
1 0% eldoc--format-doc-buffer
1 0% - #<compiled-function C3C>
1 0% - eglot--check-object
1 0% eglot--interface
1 0% - (setf jsonrpc--sync-request-alist)
1 0% gethash
8 1% - apply
6 1% - jsonrpc--event
6 1% - #<compiled-function FBD>
6 1% - apply
6 1% - jsonrpc--log-event
1 0% - jsonrpc--sync-request-alist
1 0% apply
1 0% - jsonrpc--reply
1 0% - jsonrpc-connection-send
1 0% - apply
1 0% - #<compiled-function 201>
1 0% - jsonrpc--json-encode
1 0% json-serialize
1 0% - jsonrpc-convert-from-endpoint
1 0% cl-type-of
1 0% cl--do-remf
4 0% - #<compiled-function 157>
4 0% - kill-buffer
2 0% - replace-buffer-in-windows
1 0% unrecord-window-buffer
1 0% generate-new-buffer
21 4% - #<subr-native-elisp F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_12>
21 4% - eldoc-print-current-symbol-info
21 4% - eldoc--invoke-strategy
21 4% - eldoc-documentation-compose
11 2% - eglot-hover-eldoc-function
5 1% - eglot--TextDocumentPositionParams
5 1% - eglot--TextDocumentIdentifier
5 1% - eglot-path-to-uri
5 1% - file-truename
5 1% - file-truename
5 1% - file-truename
4 0% - file-truename
3 0% - file-truename
3 0% - file-truename
3 0% - file-truename
1 0% - file-truename
1 0% - file-truename
1 0% - file-truename
1 0% - file-truename
1 0% file-truename
5 1% - eglot--highlight-piggyback
5 1% - eglot--TextDocumentPositionParams
5 1% - eglot--TextDocumentIdentifier
5 1% - eglot-path-to-uri
5 1% - file-truename
5 1% - file-truename
4 0% - file-truename
4 0% - file-truename
3 0% - file-truename
2 0% - file-truename
2 0% - file-truename
2 0% - file-truename
1 0% file-truename
1 0% - jsonrpc-async-request
1 0% - jsonrpc--async-request-1
1 0% - jsonrpc-connection-send
1 0% - apply
1 0% - #<compiled-function 201>
1 0% format
10 2% - eglot-signature-eldoc-function
10 2% - eglot--TextDocumentPositionParams
10 2% - eglot--TextDocumentIdentifier
10 2% - eglot-path-to-uri
10 2% - file-truename
10 2% - file-truename
8 1% - file-truename
7 1% - file-truename
7 1% - file-truename
7 1% - file-truename
7 1% - file-truename
7 1% - file-truename
6 1% - file-truename
6 1% - file-truename
6 1% - file-truename
5 1% - file-truename
4 0% - file-truename
3 0% - file-truename
2 0% - file-truename
1 0% tramp-completion-file-name-handler
1 0% - tramp-completion-file-name-handler
1 0% tramp-run-real-handler
1 0% - #<subr-native-elisp F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_9>
1 0% jit-lock-context-fontify
94 19% - redisplay_internal (C function)
8 1% - eval
5 1% - eglot--mode-line-format
3 0% - eglot-project-nickname
1 0% gethash
1 0% cl-type-of
1 0% apply
1 0% - eglot--progress-reporters
1 0% - apply
1 0% #<compiled-function CC3>
2 0% - if
2 0% frame-parameter
1 0% - flymake--mode-line-exception
1 0% - flymake-reporting-backends
1 0% called-interactively-p
85 17% - #<compiled-function FB8>
74 15% native-elisp-load
7 1% - comp-run-async-workers
3 0% - mapcar
3 0% prin1-to-string
2 0% - file-name-base
1 0% file-name-sans-extension
1 0% - write-region
1 0% - select-safe-coding-system
1 0% - find-auto-coding
1 0% auto-coding-alist-lookup
1 0% make-process
2 0% comp-el-to-eln-filename
1 0% delete-file
1 0% comp-accept-and-process-async-output
67 14% - command-execute
65 13% - byte-code
65 13% - read-extended-command
65 13% - read-extended-command-1
65 13% - completing-read-default
65 13% - apply
65 13% - vertico--advice
65 13% - apply
65 13% - #<subr-native-elisp completing-read-default>
19 3% - vertico--exhibit
13 2% - vertico--update
13 2% - vertico--recompute
11 2% - vertico--filter-completions
11 2% - completion-all-completions
11 2% - completion--nth-completion
11 2% - seq-some
11 2% - seq-do
11 2% - mapc
11 2% - #<compiled-function D88>
11 2% - #<compiled-function DBB>
11 2% - orderless-all-completions
11 2% - orderless--filter
11 2% - #<subr-native-elisp F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_56>
11 2% - complete-with-action
11 2% - all-completions
4 0% - #<compiled-function DE6>
2 0% - #<compiled-function 84E>
1 0% commandp
2 0% - vertico-sort-history-length-alpha
1 0% - #<subr-native-elisp F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_14>
1 0% #<primitive-function string-lessp>
3 0% - vertico--arrange-candidates
3 0% - vertico--affixate
3 0% - read-extended-command--affixation
1 0% #<subr-native-elisp F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_61>
3 0% - vertico--display-candidates
2 0% - vertico--resize-window
2 0% window-text-pixel-size
1 0% move-overlay
19 3% - #<compiled-function 12A>
19 3% native-elisp-load
18 3% redisplay_internal (C function)
2 0% - funcall-interactively
2 0% - file-notify-handle-event
2 0% - file-notify--callback-inotify
2 0% - file-notify--handle-event
2 0% - file-notify--call-handler
2 0% - #<compiled-function 79F>
1 0% - #<compiled-function 266>
1 0% - #<compiled-function 08F>
1 0% re-search-forward
1 0% - eglot-path-to-uri
1 0% - file-truename
1 0% - file-truename
1 0% - file-truename
1 0% file-truename
11 2% - apply
11 2% native--compile-async
6 1% - jsonrpc--process-filter
4 0% - jsonrpc--json-read
4 0% json-parse-buffer
1 0% - #<compiled-function 4D6>
1 0% - timer-set-time
1 0% - timer--time-setter
1 0% timerp
1 0% - jsonrpc--expected-bytes
1 0% gethash
2 0% internal-default-process-filter
1 0% + ...
1 0% + eldoc-pre-command-refresh-echo-area
[-- Attachment #4: emacs-30-new-file-truename --]
[-- Type: application/octet-stream, Size: 10595 bytes --]
105 47% - redisplay_internal (C function)
2 0% - redisplay--pre-redisplay-functions
1 0% run-hook-with-args
1 0% - eval
1 0% - if
1 0% frame-parameter
55 25% - command-execute
54 24% - byte-code
54 24% - read-extended-command
54 24% - read-extended-command-1
54 24% - completing-read-default
54 24% - apply
54 24% - vertico--advice
54 24% - apply
54 24% - #<subr-native-elisp completing-read-default>
19 8% - vertico--exhibit
12 5% - vertico--update
12 5% - vertico--recompute
11 5% - vertico--filter-completions
11 5% - completion-all-completions
11 5% - completion--nth-completion
11 5% - seq-some
11 5% - seq-do
11 5% - mapc
11 5% - #<compiled-function 210>
11 5% - #<compiled-function 279>
11 5% - orderless-all-completions
11 5% - orderless--filter
11 5% - #<subr-native-elisp F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_56>
11 5% - complete-with-action
11 5% - all-completions
8 3% - #<compiled-function 9CB>
4 1% - #<compiled-function 5C0>
3 1% commandp
1 0% version-to-list
1 0% - vertico-sort-history-length-alpha
1 0% - #<subr-native-elisp F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_14>
1 0% #<primitive-function string-lessp>
5 2% - vertico--arrange-candidates
4 1% - vertico--affixate
4 1% - read-extended-command--affixation
3 1% #<subr-native-elisp F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_61>
1 0% - vertico--window-width
1 0% get-buffer-window-list
1 0% - vertico--display-count
1 0% vertico--format-count
1 0% - vertico--display-candidates
1 0% - vertico--resize-window
1 0% window-text-pixel-size
18 8% - redisplay_internal (C function)
1 0% redisplay--pre-redisplay-functions
1 0% - timer-event-handler
1 0% - apply
1 0% - show-paren-function
1 0% - show-paren--default
1 0% show-paren--locate-near-paren
1 0% - funcall-interactively
1 0% execute-extended-command
53 24% - timer-event-handler
53 24% - apply
37 16% - #<compiled-function A34>
36 16% - jsonrpc-connection-receive
36 16% - jsonrpc--continue
34 15% - #<compiled-function 1A6>
31 14% - eglot--hover-info
31 14% - eglot--format-markup
20 9% - gfm-view-mode
5 2% - byte-code
4 1% require
1 0% - custom-declare-variable
1 0% - custom-handle-keyword
1 0% custom-add-to-group
4 1% - gfm-mode
4 1% - markdown-mode
4 1% - syntax-propertize
2 0% - markdown-syntax-propertize
1 0% - markdown-syntax-propertize-list-items
1 0% - markdown--cur-list-item-bounds
1 0% markdown-cur-list-item-end
1 0% - markdown-syntax-propertize-pre-blocks
1 0% - markdown-calculate-list-levels
1 0% markdown-search-backward-baseline
2 0% #<compiled-function BA2>
3 1% require
3 1% - read-only-mode
3 1% view-mode-enter
1 0% - custom-declare-face
1 0% - face-spec-set
1 0% - make-empty-face
1 0% - make-face
1 0% - make-face-x-resource-internal
1 0% - set-face-attributes-from-resources
1 0% - set-face-attribute-from-resource
1 0% internal-face-x-get-resource
6 2% - font-lock-ensure
6 2% - #<compiled-function C5E>
6 2% - font-lock-fontify-region
6 2% - font-lock-default-fontify-region
5 2% - font-lock-fontify-keywords-region
1 0% - markdown-match-code
1 0% - markdown-search-until-condition
1 0% - #<compiled-function ECA>
1 0% - markdown-code-block-at-pos
1 0% markdown-get-enclosing-fenced-block-construct
1 0% - markdown-fontify-plain-uris
1 0% - markdown-match-plain-uris
1 0% - markdown-match-inline-generic
1 0% markdown-match-inline-generic
1 0% - markdown-fontify-inline-links
1 0% - markdown-match-generic-links
1 0% markdown-end-of-text-block
1 0% - markdown-match-bold
1 0% markdown-match-inline-generic
1 0% - font-lock-fontify-syntactically-region
1 0% - treesit-font-lock-fontify-region
1 0% treesit--font-lock-fontify-region-1
2 0% - java-ts-mode
1 0% - prog-mode
1 0% - magit-auto-revert-mode-cmhh
1 0% add-hook
1 0% - #<compiled-function 867>
1 0% kill-buffer
1 0% - string-trim
1 0% string-trim-right
3 1% - #<compiled-function 150>
3 1% - #<compiled-function B0C>
3 1% - run-hook-with-args
2 0% - eldoc-display-in-echo-area
2 0% - eldoc--message
2 0% - eldoc-minibuffer-message
2 0% - apply
2 0% - message
2 0% redisplay_internal (C function)
1 0% - eldoc-display-in-buffer
1 0% - eldoc--format-doc-buffer
1 0% - special-mode
1 0% - global-prettify-symbols-mode-cmhh
1 0% add-hook
2 0% - #<compiled-function 752>
1 0% mapc
1 0% - mapcar
1 0% - #<compiled-function 505>
1 0% - eglot-range-region
1 0% - eglot--lsp-position-to-point
1 0% eglot-move-to-utf-16-linepos
1 0% generate-new-buffer
16 7% - #<compiled-function F1D>
16 7% - eldoc-print-current-symbol-info
16 7% - eldoc--invoke-strategy
16 7% - eldoc-documentation-compose
13 5% - eglot-hover-eldoc-function
5 2% - eglot--highlight-piggyback
4 1% - jsonrpc-async-request
4 1% - jsonrpc--async-request-1
4 1% - jsonrpc-connection-send
4 1% - apply
4 1% - #<compiled-function 835>
2 0% - jsonrpc--event
2 0% - #<compiled-function 1E3>
2 0% - apply
2 0% jsonrpc--log-event
1 0% process-send-string
1 0% - eglot--TextDocumentPositionParams
1 0% - eglot--TextDocumentIdentifier
1 0% eglot-path-to-uri
4 1% - jsonrpc-async-request
4 1% - jsonrpc--async-request-1
2 0% - jsonrpc-connection-send
2 0% - apply
2 0% - #<compiled-function 835>
2 0% - jsonrpc--event
2 0% - #<compiled-function 2A0>
2 0% - apply
2 0% jsonrpc--log-event
4 1% - eglot--TextDocumentPositionParams
4 1% - eglot--TextDocumentIdentifier
4 1% - eglot-path-to-uri
3 1% url-generic-parse-url
3 1% - eglot-signature-eldoc-function
2 0% - jsonrpc-async-request
2 0% - jsonrpc--async-request-1
1 0% - jsonrpc-connection-send
1 0% - apply
1 0% - #<compiled-function 835>
1 0% - jsonrpc--json-encode
1 0% json-serialize
1 0% - #<compiled-function FE8>
1 0% - run-with-timer
1 0% - run-at-time
1 0% - timer-activate
1 0% timer--activate
1 0% - eglot--TextDocumentPositionParams
1 0% - eglot--TextDocumentIdentifier
1 0% eglot-path-to-uri
6 2% - eldoc-pre-command-refresh-echo-area
6 2% - eldoc--message
6 2% - eldoc-minibuffer-message
6 2% - apply
6 2% message
1 0% corfu--auto-post-command
0 0% ...
[-- Attachment #5: emacs-Q-30-after-everything --]
[-- Type: application/octet-stream, Size: 10484 bytes --]
122 39% - redisplay_internal (C function)
1 0% file-remote-p
97 31% - command-execute
96 30% - byte-code
96 30% - read-extended-command
96 30% - read-extended-command-1
96 30% - completing-read-default
46 14% redisplay_internal (C function)
16 5% - command-execute
16 5% - funcall-interactively
16 5% - minibuffer-complete
15 4% - completion-in-region
15 4% - completion--in-region
15 4% - #<compiled-function E31>
15 4% - apply
15 4% - #<compiled-function 982>
15 4% - completion--in-region-1
15 4% - completion--do-completion
10 3% - completion-try-completion
10 3% - completion--nth-completion
10 3% - seq-some
10 3% - seq-do
10 3% - mapc
10 3% - #<compiled-function 373>
10 3% - #<compiled-function 1B9>
10 3% - completion-basic-try-completion
10 3% - #<subr-native-elisp F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_56>
10 3% - complete-with-action
10 3% - try-completion
1 0% #<compiled-function 317>
4 1% - minibuffer-completion-help
2 0% - completion-all-completions
2 0% - completion--nth-completion
2 0% - seq-some
2 0% - seq-do
2 0% - mapc
2 0% - #<compiled-function 7FD>
2 0% - #<compiled-function 79E>
2 0% - completion-basic-all-completions
2 0% - completion-pcm--all-completions
2 0% - #<subr-native-elisp F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_56>
2 0% - complete-with-action
2 0% all-completions
2 0% - temp-buffer-window-show
2 0% - display-buffer
2 0% - display-buffer-at-bottom
1 0% - walk-window-tree
1 0% - walk-window-tree-1
1 0% - #<compiled-function 365>
1 0% window-in-direction
1 0% - window--display-buffer
1 0% - #<compiled-function 38B>
1 0% - read-extended-command--affixation
1 0% #<subr-native-elisp F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_61>
1 0% - minibuffer-hide-completions
1 0% - bury-buffer
1 0% - window--delete
1 0% - delete-window
1 0% - window-sizable-p
1 0% - window-sizable
1 0% - window-size-fixed-p
1 0% window--size-fixed-1
1 0% minibuffer--completion-prompt-end
1 0% - timer-event-handler
1 0% - apply
1 0% - show-paren-function
1 0% - show-paren--default
1 0% - show-paren--locate-near-paren
1 0% back-to-indentation
1 0% - funcall-interactively
1 0% execute-extended-command
43 13% - timer-event-handler
43 13% - apply
21 6% - #<compiled-function E78>
21 6% - jsonrpc-connection-receive
19 6% - jsonrpc--continue
17 5% - #<compiled-function A37>
10 3% - #<compiled-function D56>
10 3% - #<compiled-function D92>
10 3% - run-hook-with-args
9 2% - eldoc-display-in-echo-area
9 2% - eldoc--message
9 2% - eldoc-minibuffer-message
9 2% - apply
9 2% - message
7 2% redisplay_internal (C function)
1 0% - eldoc-display-in-buffer
1 0% - eldoc--format-doc-buffer
1 0% - special-mode
1 0% c-leave-cc-mode-mode
7 2% - eglot--hover-info
7 2% - eglot--format-markup
5 1% - font-lock-ensure
5 1% - #<compiled-function C5E>
5 1% - font-lock-fontify-region
5 1% - c-font-lock-fontify-region
5 1% - font-lock-default-fontify-region
4 1% - font-lock-fontify-keywords-region
3 0% - c-font-lock-declarations
3 0% - c-find-decl-spots
3 0% - #<compiled-function F1B>
3 0% - c-forward-decl-or-cast-1
2 0% - c-forward-type
1 0% - c-check-qualified-type
1 0% - c-forward-over-compound-identifier
1 0% c-forward-sws
1 0% - c-forward-name
1 0% - c-forward-<>-arglist
1 0% - c-forward-<>-arglist-recur
1 0% - c-forward-sws
1 0% looking-at
1 0% looking-at
1 0% - c-font-lock-enclosing-decls
1 0% - c-parse-state
1 0% c-beginning-of-macro
1 0% - font-lock-fontify-syntactically-region
1 0% - font-lock-default-fontify-syntactically
1 0% - comment-normalize-vars
1 0% - #<compiled-function A87>
1 0% - kill-buffer
1 0% replace-buffer-in-windows
2 0% - java-mode
1 0% c-init-language-vars-for
1 0% - c-common-init
1 0% - #<subr-native-elisp F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_35>
1 0% - c-unmark-<>-around-region
1 0% #<compiled-function D6E>
2 0% #<compiled-function 3C6>
1 0% - jsonrpc--remove
1 0% slot-value
1 0% - apply
1 0% - jsonrpc--event
1 0% - #<compiled-function 0A3>
1 0% - apply
1 0% jsonrpc--log-event
12 3% - #<compiled-function 4BB>
12 3% - eldoc-print-current-symbol-info
12 3% - eldoc--invoke-strategy
12 3% - eldoc-documentation-compose
12 3% - eglot-hover-eldoc-function
6 1% - eglot--TextDocumentPositionParams
5 1% - eglot--TextDocumentIdentifier
5 1% - eglot-path-to-uri
3 0% url-generic-parse-url
1 0% - eglot--pos-to-lsp-position
1 0% - eglot-utf-16-linepos
1 0% eglot--bol
4 1% - eglot--highlight-piggyback
2 0% - eglot--TextDocumentPositionParams
2 0% - eglot--TextDocumentIdentifier
2 0% - eglot-path-to-uri
2 0% - url-generic-parse-url
1 0% - #<compiled-function D46>
1 0% kill-buffer
2 0% - jsonrpc-async-request
2 0% - jsonrpc--async-request-1
1 0% - jsonrpc-connection-send
1 0% - apply
1 0% - #<compiled-function 510>
1 0% jsonrpc--event
2 0% - jsonrpc-async-request
2 0% - jsonrpc--async-request-1
2 0% - jsonrpc-connection-send
2 0% - apply
2 0% - #<compiled-function 510>
1 0% - jsonrpc--event
1 0% - #<compiled-function 82A>
1 0% - apply
1 0% jsonrpc--log-event
1 0% - jsonrpc--json-encode
1 0% json-serialize
6 1% - #<subr-native-elisp F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_9>
6 1% jit-lock-context-fontify
3 0% - show-paren-function
3 0% - show-paren--default
2 0% - syntax-ppss
1 0% parse-partial-sexp
1 0% syntax-propertize
1 0% show-paren--locate-near-paren
1 0% - blink-cursor-start
1 0% blink-cursor--start-timer
40 12% Automatic GC
4 1% - eldoc-pre-command-refresh-echo-area
4 1% - eldoc--message
4 1% - eldoc-minibuffer-message
4 1% - apply
4 1% message
2 0% - jsonrpc--process-filter
1 0% search-forward-regexp
1 0% - #<compiled-function 79A>
1 0% timer-set-time
1 0% - jit-lock--antiblink-post-command
1 0% syntax--lbp
1 0% - undo-auto--add-boundary
1 0% - undo-auto--boundaries
1 0% add-to-list
0 0% ...
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0001-Move-file-truename-to-the-C-level.patch --]
[-- Type: text/x-diff, Size: 7698 bytes --]
From 183e636eebd9f1653d0cfdacdeba77d2043954af Mon Sep 17 00:00:00 2001
From: Theodor Thornhill <theo@thornhill.no>
Date: Wed, 27 Mar 2024 19:42:56 +0100
Subject: [PATCH] Move file-truename to the C level
---
lisp/files.el | 116 +++++---------------------------------------------
src/fileio.c | 27 ++++++++++++
2 files changed, 38 insertions(+), 105 deletions(-)
diff --git a/lisp/files.el b/lisp/files.el
index 766ed573392..6b9846c2ef4 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -32,6 +32,8 @@
(require 'pcase)
(require 'easy-mmode)) ; For `define-minor-mode'.
+(declare-function file-truename "fileio.c")
+
(defvar font-lock-keywords)
(defgroup backup nil
@@ -1413,111 +1415,15 @@ files--splice-dirname-file
unquoted
(let (file-name-handler-alist) (file-name-quote unquoted)))))
-(defun file-truename (filename &optional counter prev-dirs)
- "Return the truename of FILENAME.
-If FILENAME is not absolute, first expands it against `default-directory'.
-The truename of a file name is found by chasing symbolic links
-both at the level of the file and at the level of the directories
-containing it, until no links are left at any level.
-
-\(fn FILENAME)" ;; Don't document the optional arguments.
- ;; COUNTER and PREV-DIRS are used only in recursive calls.
- ;; COUNTER can be a cons cell whose car is the count of how many
- ;; more links to chase before getting an error.
- ;; PREV-DIRS can be a cons cell whose car is an alist
- ;; of truenames we've just recently computed.
- (cond ((or (string= filename "") (string= filename "~"))
- (setq filename (expand-file-name filename))
- (if (string= filename "")
- (setq filename "/")))
- ((and (string= (substring filename 0 1) "~")
- (string-match "~[^/]*/?" filename))
- (let ((first-part
- (substring filename 0 (match-end 0)))
- (rest (substring filename (match-end 0))))
- (setq filename (concat (expand-file-name first-part) rest)))))
-
- (or counter (setq counter (list 100)))
- (let (done
- ;; For speed, remove the ange-ftp completion handler from the list.
- ;; We know it's not needed here.
- ;; For even more speed, do this only on the outermost call.
- (file-name-handler-alist
- (if prev-dirs file-name-handler-alist
- (let ((tem (copy-sequence file-name-handler-alist)))
- (delq (rassq 'ange-ftp-completion-hook-function tem) tem)))))
- (or prev-dirs (setq prev-dirs (list nil)))
-
- ;; andrewi@harlequin.co.uk - on Windows, there is an issue with
- ;; case differences being ignored by the OS, and short "8.3 DOS"
- ;; name aliases existing for all files. (The short names are not
- ;; reported by directory-files, but can be used to refer to files.)
- ;; It seems appropriate for file-truename to resolve these issues in
- ;; the most natural way, which on Windows is to call the function
- ;; `w32-long-file-name' - this returns the exact name of a file as
- ;; it is stored on disk (expanding short name aliases with the full
- ;; name in the process).
- (if (eq system-type 'windows-nt)
- (unless (string-match "[[*?]" filename)
- ;; If filename exists, use its long name. If it doesn't
- ;; exist, the recursion below on the directory of filename
- ;; will drill down until we find a directory that exists,
- ;; and use the long name of that, with the extra
- ;; non-existent path components concatenated.
- (let ((longname (w32-long-file-name filename)))
- (if longname
- (setq filename longname)))))
-
- ;; If this file directly leads to a link, process that iteratively
- ;; so that we don't use lots of stack.
- (while (not done)
- (setcar counter (1- (car counter)))
- (if (< (car counter) 0)
- (error "Apparent cycle of symbolic links for %s" filename))
- (let ((handler (find-file-name-handler filename 'file-truename)))
- ;; For file name that has a special handler, call handler.
- ;; This is so that ange-ftp can save time by doing a no-op.
- (if handler
- (setq filename (funcall handler 'file-truename filename)
- done t)
- (let ((dir (or (file-name-directory filename) default-directory))
- target dirfile)
- ;; Get the truename of the directory.
- (setq dirfile (directory-file-name dir))
- ;; If these are equal, we have the (or a) root directory.
- (or (string= dir dirfile)
- (and (file-name-case-insensitive-p dir)
- (string-equal-ignore-case dir dirfile))
- ;; If this is the same dir we last got the truename for,
- ;; save time--don't recalculate.
- (if (assoc dir (car prev-dirs))
- (setq dir (cdr (assoc dir (car prev-dirs))))
- (let ((old dir)
- (new (file-name-as-directory (file-truename dirfile counter prev-dirs))))
- (setcar prev-dirs (cons (cons old new) (car prev-dirs)))
- (setq dir new))))
- (if (equal ".." (file-name-nondirectory filename))
- (setq filename
- (directory-file-name (file-name-directory (directory-file-name dir)))
- done t)
- (if (equal "." (file-name-nondirectory filename))
- (setq filename (directory-file-name dir)
- done t)
- ;; Put it back on the file name.
- (setq filename (concat dir (file-name-nondirectory filename)))
- ;; Is the file name the name of a link?
- (setq target (file-symlink-p filename))
- (if target
- ;; Yes => chase that link, then start all over
- ;; since the link may point to a directory name that uses links.
- ;; We can't safely use expand-file-name here
- ;; since target might look like foo/../bar where foo
- ;; is itself a link. Instead, we handle . and .. above.
- (setq filename (files--splice-dirname-file dir target)
- done nil)
- ;; No, we are done!
- (setq done t))))))))
- filename))
+;; (defun file-truename (filename &optional _x _y)
+;; "Return the truename of FILENAME.
+;; If FILENAME is not absolute, first expands it against `default-directory'.
+;; The truename of a file name is found by chasing symbolic links
+;; both at the level of the file and at the level of the directories
+;; containing it, until no links are left at any level.
+
+;; \(fn FILENAME)"
+;; (file-truename-c filename))
(defun file-chase-links (filename &optional limit)
"Chase links in FILENAME until a name that is not a link.
diff --git a/src/fileio.c b/src/fileio.c
index 12da7a9ed3a..05deedead55 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -24,6 +24,8 @@ Copyright (C) 1985-1988, 1993-2024 Free Software Foundation, Inc.
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
+#include <stdlib.h>
+#include <wordexp.h>
#ifdef DARWIN_OS
#include <sys/attr.h>
@@ -367,6 +369,30 @@ restore_point_unwind (Lisp_Object location)
unchain_marker (XMARKER (location));
}
+DEFUN ("file-truename", Ffile_truename, Sfile_truename,
+ 1, 3, 0,
+ doc: /* Return the truename of FILENAME. */)
+ (Lisp_Object filename, Lisp_Object x, Lisp_Object y)
+{
+ Lisp_Object result = filename;
+
+ CHECK_STRING (filename);
+ char *c_filename = SSDATA (filename);
+
+ wordexp_t we;
+ wordexp(c_filename, &we, 0);
+
+ char *truename = realpath(we.we_wordv[0], NULL);
+ wordfree(&we);
+
+ if (!truename)
+ return result;
+
+ result = build_string(truename);
+ free(truename);
+
+ return result;
+}
\f
DEFUN ("find-file-name-handler", Ffind_file_name_handler,
Sfind_file_name_handler, 2, 2, 0,
@@ -6850,6 +6876,7 @@ do (file-exists-p FILENAME) and FILENAME is handled by HANDLER, then
DEFSYM (Qstdout, "stdout");
DEFSYM (Qstderr, "stderr");
+ defsubr (&Sfile_truename);
defsubr (&Sfind_file_name_handler);
defsubr (&Sfile_name_directory);
defsubr (&Sfile_name_nondirectory);
--
2.40.1
^ permalink raw reply related [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-27 19:08 bug#70036: 30.0.50; Move file-truename to the C level Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-03-27 19:44 ` Eli Zaretskii
2024-03-27 21:56 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-27 20:12 ` Felician Nemeth
` (2 subsequent siblings)
3 siblings, 1 reply; 96+ messages in thread
From: Eli Zaretskii @ 2024-03-27 19:44 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: 70036
> Date: Wed, 27 Mar 2024 20:08:54 +0100
> From: Theodor Thornhill via "Bug reports for GNU Emacs,
> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>
> During the last couple of weeks I've been studying Eglots performance
> and have been noticing a couple of things that I find very
> interesting. It seems like `file-truename` is in the hot path due to the
> fact that every request to the lsp server has to create the source file
> location, and in every response we have to parse the location the
> relevant file. `file-truename` is used for this, and its performance
> isn't really up to snuff for what it provides, afaict.
>
> Below I've supplied some benchmarks and profile reports along with the
> actual patch. Before we discuss the patch itself, I want to get some
> answers to the following:
>
> - Is there a reason that this function should be supplied at the lisp
> level?
No, we could have it implemented in C. It just never was needed,
until now, and the processing there is not trivial, to say the least.
> - Does it have to be recursive?
No, it doesn't.
> Firstly, I'll show some benchmarks
>
> ```
> ;; Emacs 29 branch
>
> (benchmark-run 10000
> (file-truename "~/Work/some/long/path/to/parse/that/is/very/deep/deep/deep/super/duper/deep/deep.el"))
> ;; (1.810892642 1 0.051070616)
>
>
> ;; With new C implementation
>
> (benchmark-run 10000
> (file-truename "~/Work/some/long/path/to/parse/that/is/very/deep/deep/deep/super/duper/deep/deep.el"))
> ;; (0.018811035 0 0.0)
> ```
>
> As you can see, the C implementation, though naive for now is two orders
> of magnitude faster, and makes a noticeable difference when running an
> lsp server in emacs.
Yes, but comparing a partial implementation is not very useful, since
the complete one could be much more expensive.
> As for the patch - it now relies on wordexp to resolve the paths, and I
> believe there is no real feature parity with the old variant as for now,
> but I haven't seen any issues thus far. If this approach is accepted I
> will of course make sure we have feature parity, unless that isn't
> wanted.
We cannot rely on wordexp and we cannot rely on realpath: both are not
portable enough.
> + CHECK_STRING (filename);
> + char *c_filename = SSDATA (filename);
> +
> + wordexp_t we;
> + wordexp(c_filename, &we, 0);
> +
> + char *truename = realpath(we.we_wordv[0], NULL);
> + wordfree(&we);
> +
> + if (!truename)
> + return result;
> +
> + result = build_string(truename);
You cannot pass Lisp strings to libc functions like that: you need to
do 2 things first:
. call expand-file-name
. encode the file name with ENCODE_FILE
This is needed because relative file names in Emacs are relative to
the current buffer's directory, not relative to the current directory
of the Emacs process, and because file names with non-ASCII characters
need to be encoded to match the encoding expected by file-related APIs
in libc. Likewise, when you get a file name from a libc function, you
need to decode it with DECODE_FILE, before you create a Lisp string
from it
> + free(truename);
IMO, this should be xfree, not free. And for that to work, we need to
call realpath with 2nd argument non-NULL, but pointing to a buffer we
allocated with xmalloc, or maybe a stack-based buffer. (But since we
cannot rely on realpath, this could be a moot point.)
Thanks.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-27 19:08 bug#70036: 30.0.50; Move file-truename to the C level Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-27 19:44 ` Eli Zaretskii
@ 2024-03-27 20:12 ` Felician Nemeth
2024-03-27 21:43 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 9:22 ` Ihor Radchenko
2024-04-18 15:32 ` bug#70036: a fix that João Távora
3 siblings, 1 reply; 96+ messages in thread
From: Felician Nemeth @ 2024-03-27 20:12 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: 70036
> I've been studying Eglots performance and have been noticing a couple
> of things that I find very interesting. It seems like `file-truename`
> is in the hot path
I think Eglot repeatedly calls file-truename with the same argument (or
with an argument from a small set of filenames.)
I wonder whether it would make sense for Eglot to cache the result of
file-truename. Do you think caching would make Eglot faster than it
currently is? (Would it still be worth moving file-truename to the C
level?)
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-27 20:12 ` Felician Nemeth
@ 2024-03-27 21:43 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 6:03 ` Eli Zaretskii
0 siblings, 1 reply; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-27 21:43 UTC (permalink / raw)
To: Felician Nemeth; +Cc: 70036
Felician Nemeth <felician.nemeth@gmail.com> writes:
>> I've been studying Eglots performance and have been noticing a couple
>> of things that I find very interesting. It seems like `file-truename`
>> is in the hot path
>
> I think Eglot repeatedly calls file-truename with the same argument (or
> with an argument from a small set of filenames.)
This is true, to some extent. For the requests, eglot calls it
directly. For some handlers it is called through the function
`find-buffer-visiting`, so I'd say it's advisable to keep the function
itself fast.
>
> I wonder whether it would make sense for Eglot to cache the result of
> file-truename. Do you think caching would make Eglot faster than it
> currently is? (Would it still be worth moving file-truename to the C
> level?)
I think it would be faster, yes. This was my first thought as well, but
I quickly threw the first attempts on that away due to the fact that the
processing almost solely lies in that function, and I'd suppose it would
make other parts of emacs faster too. Adding complications to eglot
would likely make the source more complicated rather than better on the
whole. But that's only my opinion. I'd like to try to replicate the
function 1 to 1 in C.
What do you think?
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-27 19:44 ` Eli Zaretskii
@ 2024-03-27 21:56 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 1:14 ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 6:22 ` Eli Zaretskii
0 siblings, 2 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-27 21:56 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 70036
Eli Zaretskii <eliz@gnu.org> writes:
>> Date: Wed, 27 Mar 2024 20:08:54 +0100
>> From: Theodor Thornhill via "Bug reports for GNU Emacs,
>> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> During the last couple of weeks I've been studying Eglots performance
>> and have been noticing a couple of things that I find very
>> interesting. It seems like `file-truename` is in the hot path due to the
>> fact that every request to the lsp server has to create the source file
>> location, and in every response we have to parse the location the
>> relevant file. `file-truename` is used for this, and its performance
>> isn't really up to snuff for what it provides, afaict.
>>
>> Below I've supplied some benchmarks and profile reports along with the
>> actual patch. Before we discuss the patch itself, I want to get some
>> answers to the following:
>>
>> - Is there a reason that this function should be supplied at the lisp
>> level?
>
> No, we could have it implemented in C. It just never was needed,
> until now, and the processing there is not trivial, to say the least.
Yeah, the source is complicated, but it seems most of it is to gradually
chop the path shorter and shorter.
>
>> - Does it have to be recursive?
>
> No, it doesn't.
>
I guess making it iterable is something worth checking out anyway, so
I'll look into that a little further.
>> Firstly, I'll show some benchmarks
>>
[...]
>>
>> As you can see, the C implementation, though naive for now is two orders
>> of magnitude faster, and makes a noticeable difference when running an
>> lsp server in emacs.
>
> Yes, but comparing a partial implementation is not very useful, since
> the complete one could be much more expensive.
>
No doubt. The most interesting part of that benchmark is maybe to see
that the current version is very slow, not that my function is very
fast. I'd guess that even if I'd lose an order of magnitude keeping
feature parity we're better off.
>> As for the patch - it now relies on wordexp to resolve the paths, and I
>> believe there is no real feature parity with the old variant as for now,
>> but I haven't seen any issues thus far. If this approach is accepted I
>> will of course make sure we have feature parity, unless that isn't
>> wanted.
>
> We cannot rely on wordexp and we cannot rely on realpath: both are not
> portable enough.
>
OK - for my education on the portability argument. Is that because of
Emacs support targets like haiku and old versions of windows, or
something else inherent in these functions?
>> + CHECK_STRING (filename);
>> + char *c_filename = SSDATA (filename);
>> +
>> + wordexp_t we;
>> + wordexp(c_filename, &we, 0);
>> +
>> + char *truename = realpath(we.we_wordv[0], NULL);
>> + wordfree(&we);
>> +
>> + if (!truename)
>> + return result;
>> +
>> + result = build_string(truename);
>
> You cannot pass Lisp strings to libc functions like that: you need to
> do 2 things first:
>
> . call expand-file-name
> . encode the file name with ENCODE_FILE
>
> This is needed because relative file names in Emacs are relative to
> the current buffer's directory, not relative to the current directory
> of the Emacs process, and because file names with non-ASCII characters
> need to be encoded to match the encoding expected by file-related APIs
> in libc. Likewise, when you get a file name from a libc function, you
> need to decode it with DECODE_FILE, before you create a Lisp string
> from it
>
>> + free(truename);
>
> IMO, this should be xfree, not free. And for that to work, we need to
> call realpath with 2nd argument non-NULL, but pointing to a buffer we
> allocated with xmalloc, or maybe a stack-based buffer. (But since we
> cannot rely on realpath, this could be a moot point.)
>
> Thanks.
Thanks for the pointers here. I'll take note of them and investigate
further.
Another much simpler way to improve Eglot performance her could be to
allow for the relevant functions to execute through handlers, to not
break other parts of Emacs. For example `find-buffer-visiting` could
allow to run through a simpler function that merely expands and looks up
the current file, considering that the LSP server likely reports on
files that are already existing, and likely most symlink shenanigans
aren't an issue here. Just thinking out loudly on this.
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-27 21:56 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-03-28 1:14 ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 3:05 ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 7:03 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 6:22 ` Eli Zaretskii
1 sibling, 2 replies; 96+ messages in thread
From: Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-28 1:14 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036
Theodor Thornhill <theo@thornhill.no> writes:
> OK - for my education on the portability argument. Is that because of
> Emacs support targets like haiku and old versions of windows, or
> something else inherent in these functions?
Haiku supports realpath, but not wordexp. We must reimplement virtually
all POSIX functions of this nature on Windows, as the versions provided
by system C libraries, if they exist at all, are inadequate for some
reason or another. Furthermore, these functions are absent from a
number of other operating systems, such as OpenBSD and Android 14.0, and
on others (Alpine, I believe) wordexp is severely inefficient for being
implemented as a wrapper around /bin/sh.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-28 1:14 ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-03-28 3:05 ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 7:04 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 7:03 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 1 reply; 96+ messages in thread
From: Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-28 3:05 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036
Po Lu <luangruo@yahoo.com> writes:
> Haiku supports realpath, but not wordexp. We must reimplement virtually
> all POSIX functions of this nature on Windows, as the versions provided
> by system C libraries, if they exist at all, are inadequate for some
> reason or another. Furthermore, these functions are absent from a
> number of other operating systems, such as OpenBSD and Android 14.0, and
> on others (Alpine, I believe) wordexp is severely inefficient for being
> implemented as a wrapper around /bin/sh.
BTW, after reading the subject of this bug report, I don't believe there
is a place for wordexp at all. wordexp is no replacement for
Fexpand_file_name, but rather a means of expanding input in the manner
of a shell, which input might include arithmetic expressions, shell
command substitutions, and other expressions valid as shell input. If
Fexpand_file_name alone will not suffice, TRT is to replicate the
mechanics of file-truename in whole, not cutting corners with these
admittedly tempting functions.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-27 21:43 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-03-28 6:03 ` Eli Zaretskii
2024-03-28 7:10 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 96+ messages in thread
From: Eli Zaretskii @ 2024-03-28 6:03 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: 70036, felician.nemeth
> Cc: 70036@debbugs.gnu.org
> Date: Wed, 27 Mar 2024 22:43:25 +0100
> From: Theodor Thornhill via "Bug reports for GNU Emacs,
> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>
> Felician Nemeth <felician.nemeth@gmail.com> writes:
>
> >> I've been studying Eglots performance and have been noticing a couple
> >> of things that I find very interesting. It seems like `file-truename`
> >> is in the hot path
> >
> > I think Eglot repeatedly calls file-truename with the same argument (or
> > with an argument from a small set of filenames.)
>
> This is true, to some extent.
Can someone explain why Eglot needs to call file-truename in the first
place?
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-27 21:56 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 1:14 ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-03-28 6:22 ` Eli Zaretskii
2024-03-28 7:03 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 1 reply; 96+ messages in thread
From: Eli Zaretskii @ 2024-03-28 6:22 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: 70036
> From: Theodor Thornhill <theo@thornhill.no>
> Cc: 70036@debbugs.gnu.org
> Date: Wed, 27 Mar 2024 22:56:41 +0100
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> As for the patch - it now relies on wordexp to resolve the paths, and I
> >> believe there is no real feature parity with the old variant as for now,
> >> but I haven't seen any issues thus far. If this approach is accepted I
> >> will of course make sure we have feature parity, unless that isn't
> >> wanted.
> >
> > We cannot rely on wordexp and we cannot rely on realpath: both are not
> > portable enough.
>
> OK - for my education on the portability argument. Is that because of
> Emacs support targets like haiku and old versions of windows, or
> something else inherent in these functions?
Platforms other than GNU/Linux are the main concern, yes. Another
aspect to consider is whether the function is available in Gnulib --
if it is, we can import the Gnulib implementation and use it on
platforms which don't provide it natively; in that case, using such a
function is okay (but the MS-Windows port will likely need to provide
its own implementation, because Emacs uses Unicode APIs to access and
process file names, something Gnulib doesn't do on Windows).
In this case, realpath is available in Gnulib, but wordexp isn't,
AFAICT. And I'm not even sure we want to use wordexp here, because
(reading its docs) it does stuff we don't want to do in file-truename.
Why did you need to call it here?
> Another much simpler way to improve Eglot performance her could be to
> allow for the relevant functions to execute through handlers, to not
> break other parts of Emacs. For example `find-buffer-visiting` could
> allow to run through a simpler function that merely expands and looks up
> the current file, considering that the LSP server likely reports on
> files that are already existing, and likely most symlink shenanigans
> aren't an issue here. Just thinking out loudly on this.
AFAIR, find-buffer-visiting was significantly sped up recently (see
bug#66117), so if you did your benchmarks with Emacs before commit
b7a737ef49 on master, perhaps do it again with the latest master
branch.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-28 6:22 ` Eli Zaretskii
@ 2024-03-28 7:03 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 0 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-28 7:03 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 70036
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Theodor Thornhill <theo@thornhill.no>
>> Cc: 70036@debbugs.gnu.org
>> Date: Wed, 27 Mar 2024 22:56:41 +0100
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> >> As for the patch - it now relies on wordexp to resolve the paths, and I
>> >> believe there is no real feature parity with the old variant as for now,
>> >> but I haven't seen any issues thus far. If this approach is accepted I
>> >> will of course make sure we have feature parity, unless that isn't
>> >> wanted.
>> >
>> > We cannot rely on wordexp and we cannot rely on realpath: both are not
>> > portable enough.
>>
>> OK - for my education on the portability argument. Is that because of
>> Emacs support targets like haiku and old versions of windows, or
>> something else inherent in these functions?
>
> Platforms other than GNU/Linux are the main concern, yes. Another
> aspect to consider is whether the function is available in Gnulib --
> if it is, we can import the Gnulib implementation and use it on
> platforms which don't provide it natively; in that case, using such a
> function is okay (but the MS-Windows port will likely need to provide
> its own implementation, because Emacs uses Unicode APIs to access and
> process file names, something Gnulib doesn't do on Windows).
>
> In this case, realpath is available in Gnulib, but wordexp isn't,
> AFAICT. And I'm not even sure we want to use wordexp here, because
> (reading its docs) it does stuff we don't want to do in file-truename.
> Why did you need to call it here?
>
Thanks! I'll admit Fexpand-file-name likely does what we want in this
case, so I'll look into this.
>> Another much simpler way to improve Eglot performance her could be to
>> allow for the relevant functions to execute through handlers, to not
>> break other parts of Emacs. For example `find-buffer-visiting` could
>> allow to run through a simpler function that merely expands and looks up
>> the current file, considering that the LSP server likely reports on
>> files that are already existing, and likely most symlink shenanigans
>> aren't an issue here. Just thinking out loudly on this.
>
> AFAIR, find-buffer-visiting was significantly sped up recently (see
> bug#66117), so if you did your benchmarks with Emacs before commit
> b7a737ef49 on master, perhaps do it again with the latest master
> branch.
These reports are made using some commit from yesterday, so these
speedups should be included. `find-buffer-visiting` is used inside of
the `publishDiagnostics` handler, which sometimes receives file paths
for the whole project, so this really becomes a bottleneck for servers
that provide project wide diags.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-28 1:14 ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 3:05 ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-03-28 7:03 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 0 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-28 7:03 UTC (permalink / raw)
To: Po Lu; +Cc: Eli Zaretskii, 70036
Po Lu <luangruo@yahoo.com> writes:
> Theodor Thornhill <theo@thornhill.no> writes:
>
>> OK - for my education on the portability argument. Is that because of
>> Emacs support targets like haiku and old versions of windows, or
>> something else inherent in these functions?
>
> Haiku supports realpath, but not wordexp. We must reimplement virtually
> all POSIX functions of this nature on Windows, as the versions provided
> by system C libraries, if they exist at all, are inadequate for some
> reason or another. Furthermore, these functions are absent from a
> number of other operating systems, such as OpenBSD and Android 14.0, and
> on others (Alpine, I believe) wordexp is severely inefficient for being
> implemented as a wrapper around /bin/sh.
Thanks for the input!
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-28 3:05 ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-03-28 7:04 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 0 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-28 7:04 UTC (permalink / raw)
To: Po Lu; +Cc: Eli Zaretskii, 70036
Po Lu <luangruo@yahoo.com> writes:
> Po Lu <luangruo@yahoo.com> writes:
>
>> Haiku supports realpath, but not wordexp. We must reimplement virtually
>> all POSIX functions of this nature on Windows, as the versions provided
>> by system C libraries, if they exist at all, are inadequate for some
>> reason or another. Furthermore, these functions are absent from a
>> number of other operating systems, such as OpenBSD and Android 14.0, and
>> on others (Alpine, I believe) wordexp is severely inefficient for being
>> implemented as a wrapper around /bin/sh.
>
> BTW, after reading the subject of this bug report, I don't believe there
> is a place for wordexp at all. wordexp is no replacement for
> Fexpand_file_name, but rather a means of expanding input in the manner
> of a shell, which input might include arithmetic expressions, shell
> command substitutions, and other expressions valid as shell input. If
> Fexpand_file_name alone will not suffice, TRT is to replicate the
> mechanics of file-truename in whole, not cutting corners with these
> admittedly tempting functions.
I agree, and believe you are right. I think Fexpand_file_name should be
enough. I'll look into it!
Thanks!
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-28 6:03 ` Eli Zaretskii
@ 2024-03-28 7:10 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 8:52 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 11:55 ` Felician Nemeth
0 siblings, 2 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-28 7:10 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 70036, felician.nemeth
Eli Zaretskii <eliz@gnu.org> writes:
>> Cc: 70036@debbugs.gnu.org
>> Date: Wed, 27 Mar 2024 22:43:25 +0100
>> From: Theodor Thornhill via "Bug reports for GNU Emacs,
>> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> Felician Nemeth <felician.nemeth@gmail.com> writes:
>>
>> >> I've been studying Eglots performance and have been noticing a couple
>> >> of things that I find very interesting. It seems like `file-truename`
>> >> is in the hot path
>> >
>> > I think Eglot repeatedly calls file-truename with the same argument (or
>> > with an argument from a small set of filenames.)
>>
>> This is true, to some extent.
>
> Can someone explain why Eglot needs to call file-truename in the first
> place?
I tried quickly just replacing file-truename with expand-file-name, and
from some quick testing, it seems to work, and of course remove
file-truenames slow performance in the profiles:
```
@@ -1085,7 +1089,7 @@ eglot-uri-to-path
(defun eglot-path-to-uri (path)
"Convert PATH, a file name, to LSP URI string and return it."
- (let ((truepath (file-truename path)))
+ (let ((truepath (expand-file-name path)))
(if (and (url-type (url-generic-parse-url path))
;; It might be MS Windows path which includes a drive
;; letter that looks like a URL scheme (bug#59338)
```
This wouldn't help for the usage in find-buffer-visiting, though. But
this one could more easily be replaced by reworking the diagnostics
handler. We could store the last received diagnostics in the server
object, and do a quick lookup from known buffers there.
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-28 7:10 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-03-28 8:52 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 11:55 ` Felician Nemeth
1 sibling, 0 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-28 8:52 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 70036, felician.nemeth, João Távora
Theodor Thornhill <theo@thornhill.no> writes:
> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Cc: 70036@debbugs.gnu.org
>>> Date: Wed, 27 Mar 2024 22:43:25 +0100
>>> From: Theodor Thornhill via "Bug reports for GNU Emacs,
>>> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>>
>>> Felician Nemeth <felician.nemeth@gmail.com> writes:
>>>
>>> >> I've been studying Eglots performance and have been noticing a couple
>>> >> of things that I find very interesting. It seems like `file-truename`
>>> >> is in the hot path
>>> >
>>> > I think Eglot repeatedly calls file-truename with the same argument (or
>>> > with an argument from a small set of filenames.)
>>>
>>> This is true, to some extent.
>>
>> Can someone explain why Eglot needs to call file-truename in the first
>> place?
>
[...]
> handler. We could store the last received diagnostics in the server
> object, and do a quick lookup from known buffers there.
>
> Theo
It seems to be there from the very early days, like in 2018. My guess is
that we simply never noticed the performance issues here due to other
stuff like json parsing etc.
CC'd Joao to get some perspective whether or not it is intentional or
needed at all.
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-27 19:08 bug#70036: 30.0.50; Move file-truename to the C level Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-27 19:44 ` Eli Zaretskii
2024-03-27 20:12 ` Felician Nemeth
@ 2024-03-28 9:22 ` Ihor Radchenko
2024-03-28 10:59 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-18 15:32 ` bug#70036: a fix that João Távora
3 siblings, 1 reply; 96+ messages in thread
From: Ihor Radchenko @ 2024-03-28 9:22 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: 70036
[-- Attachment #1: Type: text/plain, Size: 1556 bytes --]
Theodor Thornhill via "Bug reports for GNU Emacs, the Swiss army knife
of text editors" <bug-gnu-emacs@gnu.org> writes:
> Firstly, I'll show some benchmarks
>
> ```
> ;; Emacs 29 branch
>
> (benchmark-run 10000
> (file-truename "~/Work/some/long/path/to/parse/that/is/very/deep/deep/deep/super/duper/deep/deep.el"))
> ;; (1.810892642 1 0.051070616)
> ...
> Below are the profiles and the patch. On my system I needed to `ln -s
> lisp/loadup.el .` to make it compile. Not sure if that is due to
> differences between old and new `file-truename`, or something else.
The profiles look fishy. For example, in emacs-30-before
73 15% - file-truename
73 is ~0.073 seconds, which cannot be right if you profiled the above
`benchmark-run'.
I tried to record the profiles on my side, using the above
`benchmark-run' call, and what I am seeing is that most of the CPU time
is already spend in C subrs:
1285 17% + file-name-nondirectory
1250 16% Automatic GC
1182 15% + file-symlink-p
1060 14% + file-name-case-insensitive-p
495 6% + find-file-name-handler
451 6% + file-name-as-directory
425 5% + file-name-directory
401 5% - directory-file-name
398 5% + setq
I am attaching my profile, as saved via M-x
profiler-report-write-profile. You can view it via M-x
profile-report-find-profile
At least, the number of calls to `file-name-nondirectory' can be
trivially reduced in the `file-truename' code - it is called up to three
times in a row.
[-- Attachment #2: profile-emacs-master.eld --]
[-- Type: application/octet-stream, Size: 260996 bytes --]
[-- Attachment #3: Type: text/plain, Size: 224 bytes --]
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-28 9:22 ` Ihor Radchenko
@ 2024-03-28 10:59 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 11:18 ` Ihor Radchenko
0 siblings, 1 reply; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-28 10:59 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: 70036
Ihor Radchenko <yantar92@posteo.net> writes:
> Theodor Thornhill via "Bug reports for GNU Emacs, the Swiss army knife
> of text editors" <bug-gnu-emacs@gnu.org> writes:
>
>> Firstly, I'll show some benchmarks
>>
>> ```
>> ;; Emacs 29 branch
>>
>> (benchmark-run 10000
>> (file-truename "~/Work/some/long/path/to/parse/that/is/very/deep/deep/deep/super/duper/deep/deep.el"))
>> ;; (1.810892642 1 0.051070616)
>> ...
>> Below are the profiles and the patch. On my system I needed to `ln -s
>> lisp/loadup.el .` to make it compile. Not sure if that is due to
>> differences between old and new `file-truename`, or something else.
>
> The profiles look fishy. For example, in emacs-30-before
>
Not sure I understand what you mean. I tried it again, but this time the
call is 100000 times and in an existing file on my system which is
deeply nested. I run emacs with `emacs -Q` from a build on
`30b1b0d7cd8e4d46a601e9737350cda970f6bab0`.
the relevant part from the profile this time:
```
15478 72% - command-execute
15440 72% - funcall-interactively
15439 72% - eval-last-sexp
15439 72% - #<compiled-function 0C4>
15439 72% - elisp--eval-last-sexp
15436 71% - eval
15436 71% - benchmark-call
15434 71% - #<lambda E8B>
15434 71% - file-truename
13536 63% - file-truename
12224 57% - file-truename
10970 51% - file-truename
9673 45% - file-truename
8468 39% - file-truename
7420 34% - file-truename
6374 29% - file-truename
5286 24% - file-truename
4275 19% - file-truename
3313 15% - file-truename
2454 11% - file-truename
1652 7% - file-truename
984 4% - file-truename
376 1% file-truename
1 0% time-since
1 0% execute-extended-command
```
To me this looks like it spends a lot of time in recursive calls
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-28 10:59 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-03-28 11:18 ` Ihor Radchenko
2024-03-28 11:41 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 96+ messages in thread
From: Ihor Radchenko @ 2024-03-28 11:18 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: 70036
Theodor Thornhill <theo@thornhill.no> writes:
> Not sure I understand what you mean. I tried it again, but this time the
> call is 100000 times and in an existing file on my system which is
> deeply nested. I run emacs with `emacs -Q` from a build on
> `30b1b0d7cd8e4d46a601e9737350cda970f6bab0`.
>
> the relevant part from the profile this time:
> ```
> 15478 72% - command-execute
> 15440 72% - funcall-interactively
> 15439 72% - eval-last-sexp
> 15439 72% - #<compiled-function 0C4>
> 15439 72% - elisp--eval-last-sexp
> 15436 71% - eval
> 15436 71% - benchmark-call
> 15434 71% - #<lambda E8B>
> 15434 71% - file-truename
> 13536 63% - file-truename
> 12224 57% - file-truename
> ...
> To me this looks like it spends a lot of time in recursive calls
No. It is just that your `file-truename' is native-compiled and the
profiler is unable to get the details from inside native subr code.
You can re-evaluate the defun to reveal the details in the profiler
output. Or use Emacs compiled without native-compilation support.
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-28 11:18 ` Ihor Radchenko
@ 2024-03-28 11:41 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 11:51 ` Ihor Radchenko
0 siblings, 1 reply; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-28 11:41 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: 70036
Ihor Radchenko <yantar92@posteo.net> writes:
> Theodor Thornhill <theo@thornhill.no> writes:
>
>> Not sure I understand what you mean. I tried it again, but this time the
>> call is 100000 times and in an existing file on my system which is
>> deeply nested. I run emacs with `emacs -Q` from a build on
>> `30b1b0d7cd8e4d46a601e9737350cda970f6bab0`.
>>
>> the relevant part from the profile this time:
>> ```
>> 15478 72% - command-execute
>> 15440 72% - funcall-interactively
>> 15439 72% - eval-last-sexp
>> 15439 72% - #<compiled-function 0C4>
>> 15439 72% - elisp--eval-last-sexp
>> 15436 71% - eval
>> 15436 71% - benchmark-call
>> 15434 71% - #<lambda E8B>
>> 15434 71% - file-truename
>> 13536 63% - file-truename
>> 12224 57% - file-truename
>> ...
>> To me this looks like it spends a lot of time in recursive calls
>
> No. It is just that your `file-truename' is native-compiled and the
> profiler is unable to get the details from inside native subr code.
>
> You can re-evaluate the defun to reveal the details in the profiler
> output. Or use Emacs compiled without native-compilation support.
>
> --
> Ihor Radchenko // yantar92,
> Org mode contributor,
> Learn more about Org mode at <https://orgmode.org/>.
> Support Org development at <https://liberapay.com/org-mode>,
> or support my work at <https://liberapay.com/yantar92>
I'm sorry, but I don't see the real difference here. Yes, the profile is
more detailed this way, but it doesn't change the fact that
file-truename is slow, does it?
The question to me is whether or not this is an acceptable performance
hit to take for eglot given what it's trying to do, and my opinion is
no. Whether or not it should be moved to C is open to suggestion. I'm
preparing a patch that only targets Eglot by removing reliance on this
function.
Theo
```
17839 63% - command-execute
17794 63% - funcall-interactively
17793 63% - eval-last-sexp
17793 63% - #<compiled-function EF0>
17793 63% - elisp--eval-last-sexp
17791 63% - eval
17791 63% - benchmark-call
17788 63% - #<lambda 219>
17783 63% - file-truename
17576 62% - let
17546 62% - while
17529 62% - let
17358 61% - if
17356 61% - let
16365 58% - or
15969 56% - if
15964 56% - let
15951 56% - file-name-as-directory
15824 56% - file-truename
15788 56% - let
15768 56% - while
15712 55% - let
15579 55% - if
15577 55% - let
7224 25% - or
3429 12% - if
3325 11% - let
3059 10% - file-name-as-directory
1866 6% - file-truename
1501 5% - let
1276 4% - while
1121 3% - let
1004 3% find-file-name-handler
18 0% if
49 0% - setcar
26 0% 1-
37 0% - if
23 0% <
36 0% not
97 0% - if
46 0% - eq
8 0% quote
7 0% or
218 0% - cond
112 0% - and
95 0% - string=
73 0% substring
82 0% - or
57 0% string=
17 0% or
123 0% - setcar
83 0% - cons
```
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-28 11:41 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-03-28 11:51 ` Ihor Radchenko
2024-03-28 12:47 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 13:52 ` Eli Zaretskii
0 siblings, 2 replies; 96+ messages in thread
From: Ihor Radchenko @ 2024-03-28 11:51 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: 70036
Theodor Thornhill <theo@thornhill.no> writes:
> I'm sorry, but I don't see the real difference here. Yes, the profile is
> more detailed this way, but it doesn't change the fact that
> file-truename is slow, does it?
It does not, but it is important for your suggestion to move
`file-truename' to the C level.
If the slow parts of `file-truename' are the calls to C subroutines,
rewriting `file-truename' in C will not help much with the performance.
(Unless you try hard and move parts of the used subroutines into the C
implementation of `file-truename', but that will involve copy-pasting
parts of the existing code and cannot be a good idea without very strong
justification)
> The question to me is whether or not this is an acceptable performance
> hit to take for eglot given what it's trying to do, and my opinion is
> no. Whether or not it should be moved to C is open to suggestion. I'm
> preparing a patch that only targets Eglot by removing reliance on this
> function.
If your aim is improving eglot performance specifically, sure.
If your aim is improving `file-truename' performance in general, a more
detailed analysis can be helpful.
> 17839 63% - command-execute
> 17794 63% - funcall-interactively
> 17793 63% - eval-last-sexp
> 17793 63% - #<compiled-function EF0>
> 17793 63% - elisp--eval-last-sexp
> 17791 63% - eval
> 17791 63% - benchmark-call
> 17788 63% - #<lambda 219>
> 17783 63% - file-truename
> 17576 62% - let
> ...
When you have recursive calls, it is generally more useful to view
reversed calltree (B) in the profiler report. It will accumulate calls
to the same function together, regardless on how deep in the call stack
these calls are made.
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-28 7:10 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 8:52 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-03-28 11:55 ` Felician Nemeth
2024-03-28 12:08 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 1 reply; 96+ messages in thread
From: Felician Nemeth @ 2024-03-28 11:55 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036
Theodor Thornhill <theo@thornhill.no> writes:
> This wouldn't help for the usage in find-buffer-visiting, though. But
> this one could more easily be replaced by reworking the diagnostics
> handler. We could store the last received diagnostics in the server
> object, and do a quick lookup from known buffers there.
eglot-handle-notification:textDocument/publishDiagnostics,
eglot--xref-make-match, and eglot--apply-workspace-edit call
find-buffer-visiting. It seems to me only the first case might be
really time sensitive. Theo, can you email me the relevant messages
that your server sends to Emacs? Does the server send lots of similar
diagnostics messages frequently?
Thanks,
Felicián
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-28 11:55 ` Felician Nemeth
@ 2024-03-28 12:08 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-30 9:46 ` Felician Nemeth
0 siblings, 1 reply; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-28 12:08 UTC (permalink / raw)
To: Felician Nemeth; +Cc: Eli Zaretskii, 70036
[-- Attachment #1: Type: text/plain, Size: 1188 bytes --]
Felician Nemeth <felician.nemeth@gmail.com> writes:
> Theodor Thornhill <theo@thornhill.no> writes:
>
>> This wouldn't help for the usage in find-buffer-visiting, though. But
>> this one could more easily be replaced by reworking the diagnostics
>> handler. We could store the last received diagnostics in the server
>> object, and do a quick lookup from known buffers there.
>
> eglot-handle-notification:textDocument/publishDiagnostics,
> eglot--xref-make-match, and eglot--apply-workspace-edit call
> find-buffer-visiting. It seems to me only the first case might be
> really time sensitive. Theo, can you email me the relevant messages
> that your server sends to Emacs? Does the server send lots of similar
> diagnostics messages frequently?
>
> Thanks,
> Felicián
I'll try to include such a report a little later today. But yes, it
happens a lot. Consider the following patch, though. It eliminates the
issues for publishDiagnostics. It can easily be extended to the other
places where find-buffer-visiting is used.
The publishDiagnostics is sent on change from the server, so that isn't
directly initiated by Eglot.
What do you think?
Theo
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-use-file-truepath-in-Eglot-bug-70036.patch --]
[-- Type: text/x-diff, Size: 5902 bytes --]
From d43fe85eae05baa07dd4c3e873a1b94542d97ea9 Mon Sep 17 00:00:00 2001
From: Theodor Thornhill <theo@thornhill.no>
Date: Thu, 28 Mar 2024 12:56:23 +0100
Subject: [PATCH] Don't use file-truepath in Eglot (bug#70036)
`file-truepath' is slow because of recursive calls and being implemented
in lisp. It seems to not be needed in eglot, but it is used behind the
scenes in `find-buffer-visiting', thus appearing in profiles. Moving
the implementation to a hash map will yield similar performance
benefits, but wouldn't require us to rewrite `file-truename' in C.
* lisp/progmodes/eglot.el (eglot-lsp-server): Convert 'managed-buffers'
to a hashmap.
(eglot-uri-to-path): Don't use file-truepath, as it is too slow to be
included in the hot path.
(eglot--on-shutdown): Use buffers from buffer map.
(eglot--managed-mode): Add buffer to map, rather than list. Also remove
it from the map on deactivation.
(eglot-handle-notification): Expose server and get buffer from the
buffer map.
---
lisp/progmodes/eglot.el | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 7d2f1a55165..c6dbca6fc6a 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1053,8 +1053,8 @@ eglot-lsp-server
:documentation "Map (DIR -> (WATCH ID1 ID2...)) for `didChangeWatchedFiles'."
:initform (make-hash-table :test #'equal) :accessor eglot--file-watches)
(managed-buffers
- :initform nil
- :documentation "List of buffers managed by server."
+ :documentation "Map (PATH -> BUFFER) for buffers managed by server."
+ :initform (make-hash-table :test #'equal)
:accessor eglot--managed-buffers)
(saved-initargs
:documentation "Saved initargs for reconnection purposes."
@@ -1085,12 +1085,12 @@ eglot-uri-to-path
(defun eglot-path-to-uri (path)
"Convert PATH, a file name, to LSP URI string and return it."
- (let ((truepath (file-truename path)))
+ (let ((expanded-path (expand-file-name path)))
(if (and (url-type (url-generic-parse-url path))
;; It might be MS Windows path which includes a drive
;; letter that looks like a URL scheme (bug#59338)
(not (and (eq system-type 'windows-nt)
- (file-name-absolute-p truepath))))
+ (file-name-absolute-p expanded-path))))
;; Path is already a URI, so forward it to the LSP server
;; untouched. The server should be able to handle it, since
;; it provided this URI to clients in the first place.
@@ -1098,11 +1098,11 @@ eglot-path-to-uri
(concat "file://"
;; Add a leading "/" for local MS Windows-style paths.
(if (and (eq system-type 'windows-nt)
- (not (file-remote-p truepath)))
+ (not (file-remote-p expanded-path)))
"/")
(url-hexify-string
;; Again watch out for trampy paths.
- (directory-file-name (file-local-name truepath))
+ (directory-file-name (file-local-name expanded-path))
eglot--uri-path-allowed-chars)))))
(defun eglot-range-region (range &optional markers)
@@ -1187,7 +1187,7 @@ eglot--servers-by-xrefed-file
(defun eglot--on-shutdown (server)
"Called by jsonrpc.el when SERVER is already dead."
;; Turn off `eglot--managed-mode' where appropriate.
- (dolist (buffer (eglot--managed-buffers server))
+ (dolist (buffer (map-values (eglot--managed-buffers server)))
(let (;; Avoid duplicate shutdowns (github#389)
(eglot-autoshutdown nil))
(eglot--when-live-buffer buffer (eglot--managed-mode-off))))
@@ -1992,7 +1992,11 @@ eglot--managed-mode
(add-hook 'eldoc-documentation-functions #'eglot-signature-eldoc-function
nil t)
(eldoc-mode 1))
- (cl-pushnew (current-buffer) (eglot--managed-buffers (eglot-current-server))))
+
+ (let ((buffer (current-buffer)))
+ (puthash (expand-file-name (buffer-file-name buffer))
+ buffer
+ (eglot--managed-buffers (eglot-current-server)))))
(t
(remove-hook 'after-change-functions #'eglot--after-change t)
(remove-hook 'before-change-functions #'eglot--before-change t)
@@ -2020,10 +2024,10 @@ eglot--managed-mode
(let ((server eglot--cached-server))
(setq eglot--cached-server nil)
(when server
- (setf (eglot--managed-buffers server)
- (delq (current-buffer) (eglot--managed-buffers server)))
+ (remhash (expand-file-name (buffer-file-name (current-buffer)))
+ (eglot--managed-buffers server))
(when (and eglot-autoshutdown
- (null (eglot--managed-buffers server)))
+ (null (map-values (eglot--managed-buffers server))))
(eglot-shutdown server)))))))
(defun eglot--managed-mode-off ()
@@ -2346,7 +2350,7 @@ eglot-handle-notification
(remhash token (eglot--progress-reporters server))))))))))
(cl-defmethod eglot-handle-notification
- (_server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
+ (server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
&allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
"Handle notification publishDiagnostics."
(cl-flet ((eglot--diag-type (sev)
@@ -2357,7 +2361,7 @@ eglot-handle-notification
(mess (source code message)
(concat source (and code (format " [%s]" code)) ": " message)))
(if-let* ((path (expand-file-name (eglot-uri-to-path uri)))
- (buffer (find-buffer-visiting path)))
+ (buffer (gethash path (eglot--managed-buffers server))))
(with-current-buffer buffer
(cl-loop
initially
--
2.40.1
^ permalink raw reply related [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-28 11:51 ` Ihor Radchenko
@ 2024-03-28 12:47 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 13:52 ` Eli Zaretskii
1 sibling, 0 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-28 12:47 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: 70036
Ihor Radchenko <yantar92@posteo.net> writes:
> Theodor Thornhill <theo@thornhill.no> writes:
>
>> I'm sorry, but I don't see the real difference here. Yes, the profile is
>> more detailed this way, but it doesn't change the fact that
>> file-truename is slow, does it?
>
> It does not, but it is important for your suggestion to move
> `file-truename' to the C level.
>
> If the slow parts of `file-truename' are the calls to C subroutines,
> rewriting `file-truename' in C will not help much with the performance.
> (Unless you try hard and move parts of the used subroutines into the C
> implementation of `file-truename', but that will involve copy-pasting
> parts of the existing code and cannot be a good idea without very strong
> justification)
>
Yeah, I agree with this.
>> The question to me is whether or not this is an acceptable performance
>> hit to take for eglot given what it's trying to do, and my opinion is
>> no. Whether or not it should be moved to C is open to suggestion. I'm
>> preparing a patch that only targets Eglot by removing reliance on this
>> function.
>
> If your aim is improving eglot performance specifically, sure.
> If your aim is improving `file-truename' performance in general, a more
> detailed analysis can be helpful.
>
And this.
Wrt eglot specifically or not - I'm not sure whether or not this affects
other parts of emacs. My guess is that it will, but I haven't
investigated that yet.
>> 17839 63% - command-execute
>> 17794 63% - funcall-interactively
>> 17793 63% - eval-last-sexp
>> 17793 63% - #<compiled-function EF0>
>> 17793 63% - elisp--eval-last-sexp
>> 17791 63% - eval
>> 17791 63% - benchmark-call
>> 17788 63% - #<lambda 219>
>> 17783 63% - file-truename
>> 17576 62% - let
>> ...
>
> When you have recursive calls, it is generally more useful to view
> reversed calltree (B) in the profiler report. It will accumulate calls
> to the same function together, regardless on how deep in the call stack
> these calls are made.
>
TIL - thanks :)
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-28 11:51 ` Ihor Radchenko
2024-03-28 12:47 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-03-28 13:52 ` Eli Zaretskii
1 sibling, 0 replies; 96+ messages in thread
From: Eli Zaretskii @ 2024-03-28 13:52 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: 70036, theo
> Cc: 70036@debbugs.gnu.org
> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Thu, 28 Mar 2024 11:51:06 +0000
>
> If the slow parts of `file-truename' are the calls to C subroutines,
> rewriting `file-truename' in C will not help much with the performance.
That is only true if the C implementation uses the same C subroutines,
or their bodies. If, instead, the C implementation uses something
like 'realpath', which produces the fully resolved file name ion one
go, then the profile from the Lisp implementation is not really
relevant.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-28 12:08 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-03-30 9:46 ` Felician Nemeth
2024-03-30 11:18 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-30 12:45 ` Eli Zaretskii
0 siblings, 2 replies; 96+ messages in thread
From: Felician Nemeth @ 2024-03-30 9:46 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036
Theodor Thornhill <theo@thornhill.no> writes:
> Felician Nemeth <felician.nemeth@gmail.com> writes:
>> Theo, can you email me the relevant messages that your server sends
>> to Emacs? Does the server send lots of similar diagnostics messages
>> frequently?
> I'll try to include such a report a little later today.
Thanks, that would be helpful.
> [2. text/x-diff; 0001-Don-t-use-file-truepath-in-Eglot-bug-70036.patch]...
I think using find-buffer-visiting instead of get-file-buffer and
file-truename instead of expand-file-name in Eglot is problematic.
Let's say we have these files:
/project/a.c
/project/a.h -> /other/a.h
Eglot will communicate these file names to the LSP server: /project/a.c
and /other/a.h. Then the server cannot "associate" a.h with a.c.
Additionally, a.h will be outside of the LSP workspace.
This indeed confuses clangd a bit: it only takes into account the
changes of buffer a.h when it is saved. (Because it assumes
/project/a.h is not opened in the editor.)
------
Regarding the patch itself, cache invalidation is missing from it. The
user might kill a buffer or save it under a different name with
write-file. Changing (PATH -> BUFFER) to (PATH -> (BUFFER, FILENAME))
would probably work. Eglot should save the current buffer-file-name
when it inserts a new item into the hash of managed-buffers. And when
it retrieves an item, it should verify whether the buffer-file-name is
the same as the saved file-name.
Can file-truepath change while buffer-file-name remains the same? Yes,
but I think Eglot could ignore those rare cases, or handle it elsewhere.
(For example, it could update the cache entry after a buffer is saved.)
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-30 9:46 ` Felician Nemeth
@ 2024-03-30 11:18 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-30 12:45 ` Eli Zaretskii
1 sibling, 0 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-30 11:18 UTC (permalink / raw)
To: Felician Nemeth; +Cc: Eli Zaretskii, 70036
[-- Attachment #1: Type: text/plain, Size: 3007 bytes --]
Felician Nemeth <felician.nemeth@gmail.com> writes:
> Theodor Thornhill <theo@thornhill.no> writes:
>> Felician Nemeth <felician.nemeth@gmail.com> writes:
>
>>> Theo, can you email me the relevant messages that your server sends
>>> to Emacs? Does the server send lots of similar diagnostics messages
>>> frequently?
>
>> I'll try to include such a report a little later today.
>
> Thanks, that would be helpful.
>
It's a little hard scraping the logs for company related data, so I
haven't done this yet, but the profiles should be pretty expanatory if
you do some edits after applying the below patch.
>> [2. text/x-diff; 0001-Don-t-use-file-truepath-in-Eglot-bug-70036.patch]...
>
> I think using find-buffer-visiting instead of get-file-buffer and
> file-truename instead of expand-file-name in Eglot is problematic.
> Let's say we have these files:
>
> /project/a.c
> /project/a.h -> /other/a.h
>
> Eglot will communicate these file names to the LSP server: /project/a.c
> and /other/a.h. Then the server cannot "associate" a.h with a.c.
> Additionally, a.h will be outside of the LSP workspace.
>
> This indeed confuses clangd a bit: it only takes into account the
> changes of buffer a.h when it is saved. (Because it assumes
> /project/a.h is not opened in the editor.)
So in other words this is already a bug in eglot?
>
> ------
>
> Regarding the patch itself, cache invalidation is missing from it. The
> user might kill a buffer or save it under a different name with
> write-file. Changing (PATH -> BUFFER) to (PATH -> (BUFFER, FILENAME))
> would probably work. Eglot should save the current buffer-file-name
> when it inserts a new item into the hash of managed-buffers. And when
> it retrieves an item, it should verify whether the buffer-file-name is
> the same as the saved file-name.
>
> Can file-truepath change while buffer-file-name remains the same? Yes,
> but I think Eglot could ignore those rare cases, or handle it elsewhere.
> (For example, it could update the cache entry after a buffer is saved.)
Actually, cache invalidation is there, at least for killing a
buffer. Major modes are disabled on killing buffer, and it is removed as
a part of the teardown. Saving to a new buffer isn't handled properly
yet, but this looks like something not really supported by Eglot yet
anyway. If I resave a buffer with C-x C-w its content will be placed in
a new buffer, but old one will not be deleted, and the new one will not
be registered with the eglot server before running M-x revert-buffer
anyways. So that seems like a different issue, really.
Please check out the new patch. I find no issues diverging from the
current behavior using this one. It seems to solve all the performance
issues I stated, and now completely new stuff shows up in the profile,
which to me sounds like the bottlenecks have moved, suggesting we get a
nice performance upgrade.
What do you think? I'll test this for a while and install to master in a
few days if nothing comes up :-)
Theo
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-use-file-truepath-in-Eglot-bug-70036.patch --]
[-- Type: text/x-diff, Size: 8181 bytes --]
From 26e5f3cc8e767215f7c50800a6d713702b8fa144 Mon Sep 17 00:00:00 2001
From: Theodor Thornhill <theo@thornhill.no>
Date: Thu, 28 Mar 2024 12:56:23 +0100
Subject: [PATCH] Don't use file-truepath in Eglot (bug#70036)
`file-truepath' is slow because of recursive calls and being implemented
in lisp. It seems to not be needed in eglot, but it is used behind the
scenes in `find-buffer-visiting', thus appearing in profiles. Moving
the implementation to a hash map will yield similar performance
benefits, but wouldn't require us to rewrite `file-truename' in C.
* lisp/progmodes/eglot.el (eglot-lsp-server): Convert 'managed-buffers'
to a hashmap.
(eglot-uri-to-path): Don't use file-truepath, as it is too slow to be
included in the hot path.
(eglot--on-shutdown): Use buffers from buffer map.
(eglot--managed-mode): Add buffer to map, rather than list. Also remove
it from the map on deactivation.
(eglot-handle-notification): Expose server and get buffer from the
buffer map.
---
lisp/progmodes/eglot.el | 42 +++++++++++++++++++++++------------------
1 file changed, 24 insertions(+), 18 deletions(-)
diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index f247c43203c..7f4284bf09d 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1053,8 +1053,8 @@ eglot-lsp-server
:documentation "Map (DIR -> (WATCH ID1 ID2...)) for `didChangeWatchedFiles'."
:initform (make-hash-table :test #'equal) :accessor eglot--file-watches)
(managed-buffers
- :initform nil
- :documentation "List of buffers managed by server."
+ :documentation "Map (PATH -> BUFFER) for buffers managed by server."
+ :initform (make-hash-table :test #'equal)
:accessor eglot--managed-buffers)
(saved-initargs
:documentation "Saved initargs for reconnection purposes."
@@ -1085,12 +1085,12 @@ eglot-uri-to-path
(defun eglot-path-to-uri (path)
"Convert PATH, a file name, to LSP URI string and return it."
- (let ((truepath (file-truename path)))
+ (let ((expanded-path (expand-file-name path)))
(if (and (url-type (url-generic-parse-url path))
;; It might be MS Windows path which includes a drive
;; letter that looks like a URL scheme (bug#59338)
(not (and (eq system-type 'windows-nt)
- (file-name-absolute-p truepath))))
+ (file-name-absolute-p expanded-path))))
;; Path is already a URI, so forward it to the LSP server
;; untouched. The server should be able to handle it, since
;; it provided this URI to clients in the first place.
@@ -1098,11 +1098,11 @@ eglot-path-to-uri
(concat "file://"
;; Add a leading "/" for local MS Windows-style paths.
(if (and (eq system-type 'windows-nt)
- (not (file-remote-p truepath)))
+ (not (file-remote-p expanded-path)))
"/")
(url-hexify-string
;; Again watch out for trampy paths.
- (directory-file-name (file-local-name truepath))
+ (directory-file-name (file-local-name expanded-path))
eglot--uri-path-allowed-chars)))))
(defun eglot-range-region (range &optional markers)
@@ -1187,7 +1187,7 @@ eglot--servers-by-xrefed-file
(defun eglot--on-shutdown (server)
"Called by jsonrpc.el when SERVER is already dead."
;; Turn off `eglot--managed-mode' where appropriate.
- (dolist (buffer (eglot--managed-buffers server))
+ (dolist (buffer (map-values (eglot--managed-buffers server)))
(let (;; Avoid duplicate shutdowns (github#389)
(eglot-autoshutdown nil))
(eglot--when-live-buffer buffer (eglot--managed-mode-off))))
@@ -1992,7 +1992,11 @@ eglot--managed-mode
(add-hook 'eldoc-documentation-functions #'eglot-signature-eldoc-function
nil t)
(eldoc-mode 1))
- (cl-pushnew (current-buffer) (eglot--managed-buffers (eglot-current-server))))
+
+ (let ((buffer (current-buffer)))
+ (puthash (expand-file-name (buffer-file-name buffer))
+ buffer
+ (eglot--managed-buffers (eglot-current-server)))))
(t
(remove-hook 'after-change-functions #'eglot--after-change t)
(remove-hook 'before-change-functions #'eglot--before-change t)
@@ -2020,10 +2024,10 @@ eglot--managed-mode
(let ((server eglot--cached-server))
(setq eglot--cached-server nil)
(when server
- (setf (eglot--managed-buffers server)
- (delq (current-buffer) (eglot--managed-buffers server)))
+ (remhash (expand-file-name (buffer-file-name (current-buffer)))
+ (eglot--managed-buffers server))
(when (and eglot-autoshutdown
- (null (eglot--managed-buffers server)))
+ (null (map-values (eglot--managed-buffers server))))
(eglot-shutdown server)))))))
(defun eglot--managed-mode-off ()
@@ -2346,7 +2350,7 @@ eglot-handle-notification
(remhash token (eglot--progress-reporters server))))))))))
(cl-defmethod eglot-handle-notification
- (_server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
+ (server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
&allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
"Handle notification publishDiagnostics."
(cl-flet ((eglot--diag-type (sev)
@@ -2357,7 +2361,7 @@ eglot-handle-notification
(mess (source code message)
(concat source (and code (format " [%s]" code)) ": " message)))
(if-let* ((path (expand-file-name (eglot-uri-to-path uri)))
- (buffer (find-buffer-visiting path)))
+ (buffer (gethash path (eglot--managed-buffers server))))
(with-current-buffer buffer
(cl-loop
initially
@@ -2842,7 +2846,7 @@ eglot--xref-make-match
Try to visit the target file for a richer summary line."
(pcase-let*
((file (eglot-uri-to-path uri))
- (visiting (or (find-buffer-visiting file)
+ (visiting (or (gethash file (eglot--managed-buffers (eglot-current-server)))
(gethash uri eglot--temp-location-buffers)))
(collect (lambda ()
(eglot--widening
@@ -3542,13 +3546,14 @@ eglot--propose-changes-as-diff
(with-current-buffer (get-buffer-create "*EGLOT proposed server changes*")
(buffer-disable-undo (current-buffer))
(let ((inhibit-read-only t)
- (target (current-buffer)))
+ (target (current-buffer))
+ (managed-buffers (eglot--managed-buffers (eglot-current-server))))
(diff-mode)
(erase-buffer)
(pcase-dolist (`(,path ,edits ,_) prepared)
(with-temp-buffer
(let* ((diff (current-buffer))
- (existing-buf (find-buffer-visiting path))
+ (existing-buf (gethash path (gethash path managed-buffers)))
(existing-buf-label (prin1-to-string existing-buf)))
(with-temp-buffer
(if existing-buf
@@ -3583,7 +3588,8 @@ eglot--apply-workspace-edit
(eglot--dbind ((VersionedTextDocumentIdentifier) uri version)
textDocument
(list (eglot-uri-to-path uri) edits version)))
- documentChanges)))
+ documentChanges))
+ (managed-buffers (eglot--managed-buffers (eglot-current-server))))
(unless (and changes documentChanges)
;; We don't want double edits, and some servers send both
;; changes and documentChanges. This unless ensures that we
@@ -3591,7 +3597,7 @@ eglot--apply-workspace-edit
(cl-loop for (uri edits) on changes by #'cddr
do (push (list (eglot-uri-to-path uri) edits) prepared)))
(cl-flet ((notevery-visited-p ()
- (cl-notevery #'find-buffer-visiting
+ (cl-notevery (lambda (p) (gethash p managed-buffers))
(mapcar #'car prepared)))
(accept-p ()
(y-or-n-p
--
2.40.1
^ permalink raw reply related [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-30 9:46 ` Felician Nemeth
2024-03-30 11:18 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-03-30 12:45 ` Eli Zaretskii
2024-03-31 12:57 ` Felician Nemeth
1 sibling, 1 reply; 96+ messages in thread
From: Eli Zaretskii @ 2024-03-30 12:45 UTC (permalink / raw)
To: Felician Nemeth; +Cc: 70036, theo
> From: Felician Nemeth <felician.nemeth@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, 70036@debbugs.gnu.org
> Date: Sat, 30 Mar 2024 10:46:00 +0100
>
> I think using find-buffer-visiting instead of get-file-buffer and
> file-truename instead of expand-file-name in Eglot is problematic.
> Let's say we have these files:
>
> /project/a.c
> /project/a.h -> /other/a.h
>
> Eglot will communicate these file names to the LSP server: /project/a.c
> and /other/a.h. Then the server cannot "associate" a.h with a.c.
> Additionally, a.h will be outside of the LSP workspace.
>
> This indeed confuses clangd a bit: it only takes into account the
> changes of buffer a.h when it is saved. (Because it assumes
> /project/a.h is not opened in the editor.)
Can LSP servers resolve symlinks? If they can, then expand-file-name
is TRT, AFAIU.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-30 12:45 ` Eli Zaretskii
@ 2024-03-31 12:57 ` Felician Nemeth
2024-03-31 13:32 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 96+ messages in thread
From: Felician Nemeth @ 2024-03-31 12:57 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 70036, theo
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Felician Nemeth <felician.nemeth@gmail.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>, 70036@debbugs.gnu.org
>> Date: Sat, 30 Mar 2024 10:46:00 +0100
>>
>> I think using find-buffer-visiting instead of get-file-buffer and
>> file-truename instead of expand-file-name in Eglot is problematic.
>> Let's say we have these files:
>>
>> /project/a.c
>> /project/a.h -> /other/a.h
>>
>> Eglot will communicate these file names to the LSP server: /project/a.c
>> and /other/a.h. Then the server cannot "associate" a.h with a.c.
>> Additionally, a.h will be outside of the LSP workspace.
>>
>> This indeed confuses clangd a bit: it only takes into account the
>> changes of buffer a.h when it is saved. (Because it assumes
>> /project/a.h is not opened in the editor.)
>
> Can LSP servers resolve symlinks? If they can, then expand-file-name
> is TRT, AFAIU.
The LSP specification does not talk about symlinks. The servers I used
let the operating system resolve symlinks for them.
All in all, I think Eglot should switch to use expand-file-name as well.
It should also use get-file-buffer instead of the more complicated
caching mechanism proposed previously. Theo?
Nevertheless, Eglot will continue to handle poorly the case when there
is a symlink in a project that points outside of the project.
Hopefully, this can be potentially solved later with the idea behind
eglot--servers-by-xrefed-file.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: 30.0.50; Move file-truename to the C level
2024-03-31 12:57 ` Felician Nemeth
@ 2024-03-31 13:32 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 0 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-31 13:32 UTC (permalink / raw)
To: Felician Nemeth; +Cc: Eli Zaretskii, 70036
[-- Attachment #1: Type: text/html, Size: 2571 bytes --]
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-03-27 19:08 bug#70036: 30.0.50; Move file-truename to the C level Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
` (2 preceding siblings ...)
2024-03-28 9:22 ` Ihor Radchenko
@ 2024-04-18 15:32 ` João Távora
2024-04-18 15:39 ` João Távora
` (2 more replies)
3 siblings, 3 replies; 96+ messages in thread
From: João Távora @ 2024-04-18 15:32 UTC (permalink / raw)
To: Theodor Thornhill, Felician Nemeth; +Cc: Eli Zaretskii, 70036
So I've read up on the bug report and I had a close look at the Eglot
usage profiles (not the micro-benchmarks, those are reasonably
irrelevant in what concerns Eglot). I see these kinds of things in
Theodor's profiles
18 8% - eglot--TextDocumentPositionParams
18 8% - eglot--TextDocumentIdentifier
18 8% - eglot--path-to-uri
15 7% - file-truename
14 6% - file-truename
14 6% - file-truename
11 5% - file-truename
11 5% - file-truename
11 5% - file-truename
10 4% - file-truename
10 4% - file-truename
8 3% - file-truename
8 3% - file-truename
8 3% - file-truename
5 2% - file-truename
3 1% - file-truename
2 0% - file-truename
1 0% file-truename
I could reproduce this, but never even close to the amount of ~7-8%.
Best I could get was 1% and I had to work pretty hard for it. If I
invoke completion or something heavier like that, it completely
dominates the profile.
25 1% - eglot--TextDocumentPositionParams
23 1% - eglot--TextDocumentIdentifier
23 1% - eglot-path-to-uri
13 0% - file-truename
13 0% - file-truename
13 0% - file-truename
13 0% file-truename
Maybe that's because file-truename is a recursive function that becomes
slower as the path it's asked to analyse becomes longer (obviously,
there can be a symlink at every junction).
So let's assume for the sake of argument that it's frequent for
users to have very long file names with very many components
or that the 1% overhead on the average path length is a real
performance problem for people.
If so, I think this simpler patch after my sig is all we need, as it
completely clears the profile of any "file-truename". I have reverted
the earlier patch and pushed a patch very similar to the one after my sig.
I'm interested in more details of exactly what constitutes actual
usage, i.e. what did you do while recording a cpu or mem profile.
As to the symlink-outside-the-project issue that Felicián raised,
I was not aware of that bug. It seems a separate bug at any rate.
I've also noticed that the Eglot automated tests are failing.
That too is unexpected. I will have a look at that, too.
João
commit 23d6517b2499089f775640b88958774ac2c91049
Author: João Távora <joaotavora@gmail.com>
Date: Thu Apr 18 08:03:10 2024 -0500
Better way to fix bug#70036
Cache eglot--cached-tdi per buffer, like we do with some many
other variables already, to avoid frequent expensive
recomputation of a value that requires potentially many
'file-truename' calls.
diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 806265544d6..6196aaff7a3 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -2517,12 +2517,17 @@ eglot-handle-request
(t (setq success :json-false)))
`(:success ,success)))
+(defvar-local eglot--cached-tdi nil
+ "A cached LSP TextDocumentIdentifier URI string.")
+
(defun eglot--TextDocumentIdentifier ()
"Compute TextDocumentIdentifier object for current buffer."
- `(:uri ,(eglot-path-to-uri (or buffer-file-name
- (ignore-errors
- (buffer-file-name
- (buffer-base-buffer)))))))
+ `(:uri ,(or eglot--cached-tdi
+ (setq eglot--cached-tdi
+ (eglot-path-to-uri (or buffer-file-name
+ (ignore-errors
+ (buffer-file-name
+ (buffer-base-buffer)))))))))
(defvar-local eglot--versioned-identifier 0)
--
João Távora
^ permalink raw reply related [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 15:32 ` bug#70036: a fix that João Távora
@ 2024-04-18 15:39 ` João Távora
2024-04-18 15:40 ` Ihor Radchenko
2024-04-18 15:49 ` Eli Zaretskii
2 siblings, 0 replies; 96+ messages in thread
From: João Távora @ 2024-04-18 15:39 UTC (permalink / raw)
To: Theodor Thornhill, Felician Nemeth; +Cc: Eli Zaretskii, 70036
[Sorry I botched the subject line in my last mail, this is what I
get for multitasking million things. I meant to call it:
"a fix that doesn't break existing use cases"]
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 15:32 ` bug#70036: a fix that João Távora
2024-04-18 15:39 ` João Távora
@ 2024-04-18 15:40 ` Ihor Radchenko
2024-04-18 15:45 ` João Távora
2024-04-18 15:49 ` Eli Zaretskii
2 siblings, 1 reply; 96+ messages in thread
From: Ihor Radchenko @ 2024-04-18 15:40 UTC (permalink / raw)
To: João Távora
Cc: Felician Nemeth, Eli Zaretskii, 70036, Theodor Thornhill
João Távora <joaotavora@gmail.com> writes:
> 18 8% - eglot--TextDocumentPositionParams
> ...
> 25 1% - eglot--TextDocumentPositionParams
> 23 1% - eglot--TextDocumentIdentifier
> 23 1% - eglot-path-to-uri
> 13 0% - file-truename
Be aware that 13 is roughly in milliseconds (with the default value of
`profiler-sampling-interval'). The numbers you are seeing in this
profile are close to the "noise" levels.
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 15:40 ` Ihor Radchenko
@ 2024-04-18 15:45 ` João Távora
0 siblings, 0 replies; 96+ messages in thread
From: João Távora @ 2024-04-18 15:45 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: Felician Nemeth, Eli Zaretskii, 70036, Theodor Thornhill
On Thu, Apr 18, 2024 at 4:39 PM Ihor Radchenko <yantar92@posteo.net> wrote:
>
> João Távora <joaotavora@gmail.com> writes:
>
> > 18 8% - eglot--TextDocumentPositionParams
> > ...
> > 25 1% - eglot--TextDocumentPositionParams
> > 23 1% - eglot--TextDocumentIdentifier
> > 23 1% - eglot-path-to-uri
> > 13 0% - file-truename
>
> Be aware that 13 is roughly in milliseconds (with the default value of
> `profiler-sampling-interval'). The numbers you are seeing in this
> profile are close to the "noise" levels.
Correct, and that was precisely my point. I had to work hard just
to get these numbers to show on the profile with average-length
pathnames. But it was enough to verify that I apply my caching
patch, they go away completely.
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 15:32 ` bug#70036: a fix that João Távora
2024-04-18 15:39 ` João Távora
2024-04-18 15:40 ` Ihor Radchenko
@ 2024-04-18 15:49 ` Eli Zaretskii
2024-04-18 16:11 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-18 16:12 ` João Távora
2 siblings, 2 replies; 96+ messages in thread
From: Eli Zaretskii @ 2024-04-18 15:49 UTC (permalink / raw)
To: João Távora; +Cc: felician.nemeth, 70036, theo
> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 18 Apr 2024 16:32:33 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, 70036@debbugs.gnu.org
>
> So I've read up on the bug report and I had a close look at the Eglot
> usage profiles (not the micro-benchmarks, those are reasonably
> irrelevant in what concerns Eglot). I see these kinds of things in
> Theodor's profiles
>
> 18 8% - eglot--TextDocumentPositionParams
> 18 8% - eglot--TextDocumentIdentifier
> 18 8% - eglot--path-to-uri
> 15 7% - file-truename
> 14 6% - file-truename
> 14 6% - file-truename
> 11 5% - file-truename
> 11 5% - file-truename
> 11 5% - file-truename
> 10 4% - file-truename
> 10 4% - file-truename
> 8 3% - file-truename
> 8 3% - file-truename
> 8 3% - file-truename
> 5 2% - file-truename
> 3 1% - file-truename
> 2 0% - file-truename
> 1 0% file-truename
>
>
> I could reproduce this, but never even close to the amount of ~7-8%.
> Best I could get was 1% and I had to work pretty hard for it. If I
> invoke completion or something heavier like that, it completely
> dominates the profile.
>
> 25 1% - eglot--TextDocumentPositionParams
> 23 1% - eglot--TextDocumentIdentifier
> 23 1% - eglot-path-to-uri
> 13 0% - file-truename
> 13 0% - file-truename
> 13 0% - file-truename
> 13 0% file-truename
>
> Maybe that's because file-truename is a recursive function that becomes
> slower as the path it's asked to analyse becomes longer (obviously,
> there can be a symlink at every junction).
Profiles can mislead and they can lie. It is much easier to time the
old and the new code doing the same jobs, and compare the times.
> If so, I think this simpler patch after my sig is all we need, as it
> completely clears the profile of any "file-truename". I have reverted
> the earlier patch and pushed a patch very similar to the one after my sig.
This new code should also be timed and compared to the other two
versions, before we make the final decision on this.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 15:49 ` Eli Zaretskii
@ 2024-04-18 16:11 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-18 16:15 ` João Távora
2024-04-18 16:21 ` Eli Zaretskii
2024-04-18 16:12 ` João Távora
1 sibling, 2 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-18 16:11 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 70036, felician.nemeth, João Távora
[-- Attachment #1: Type: text/html, Size: 9317 bytes --]
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 15:49 ` Eli Zaretskii
2024-04-18 16:11 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-18 16:12 ` João Távora
2024-04-18 16:24 ` Eli Zaretskii
1 sibling, 1 reply; 96+ messages in thread
From: João Távora @ 2024-04-18 16:12 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: felician.nemeth, 70036, theo
On Thu, Apr 18, 2024 at 4:49 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Date: Thu, 18 Apr 2024 16:32:33 +0100
> > Cc: Eli Zaretskii <eliz@gnu.org>, 70036@debbugs.gnu.org
> >
> > So I've read up on the bug report and I had a close look at the Eglot
> > usage profiles (not the micro-benchmarks, those are reasonably
> > irrelevant in what concerns Eglot). I see these kinds of things in
> > Theodor's profiles
> >
> > 18 8% - eglot--TextDocumentPositionParams
> > 18 8% - eglot--TextDocumentIdentifier
> > 18 8% - eglot--path-to-uri
> > 15 7% - file-truename
> > 14 6% - file-truename
> > 14 6% - file-truename
> > 11 5% - file-truename
> > 11 5% - file-truename
> > 11 5% - file-truename
> > 10 4% - file-truename
> > 10 4% - file-truename
> > 8 3% - file-truename
> > 8 3% - file-truename
> > 8 3% - file-truename
> > 5 2% - file-truename
> > 3 1% - file-truename
> > 2 0% - file-truename
> > 1 0% file-truename
> >
> >
> > I could reproduce this, but never even close to the amount of ~7-8%.
> > Best I could get was 1% and I had to work pretty hard for it. If I
> > invoke completion or something heavier like that, it completely
> > dominates the profile.
> >
> > 25 1% - eglot--TextDocumentPositionParams
> > 23 1% - eglot--TextDocumentIdentifier
> > 23 1% - eglot-path-to-uri
> > 13 0% - file-truename
> > 13 0% - file-truename
> > 13 0% - file-truename
> > 13 0% file-truename
> >
> > Maybe that's because file-truename is a recursive function that becomes
> > slower as the path it's asked to analyse becomes longer (obviously,
> > there can be a symlink at every junction).
>
> Profiles can mislead and they can lie.
Theo's profiles attached to this issue were the best I could find.
If the manner in which they were collected was enough to make a
decision in a certainway, they should be good enough to make a decision
in another way.
But I fully agree we should have a more constrained test case.
> It is much easier to time the
> old and the new code doing the same jobs, and compare the times.
So... that's what I did. As I wrote, I first reproduced (a fraction of)
Theo's findings with the old code (before his patch). Then in the latest
master his patch, added my new patch, and verified that the newest code
no longer reproduces those findings.
Do you have a concrete idea of what this "job" is? I could only gather
a moderately useful idea from Theo's profiles, but it wasn't vague
by any means. It seems his "timer-event-handler" which is usually
doing the work for Eglot's at-point documentation job, is spending
about a quarter of its time in file-truename. That's typical
when just browsing code with the cursor and reading documentation.
> > If so, I think this simpler patch after my sig is all we need, as it
> > completely clears the profile of any "file-truename". I have reverted
> > the earlier patch and pushed a patch very similar to the one after my sig.
>
> This new code should also be timed and compared to the other two
> versions, before we make the final decision on this.
Of course. Let's have Theo do this comparison and perhaps describe
in more detail the conditions in which he collected his profiles
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 16:11 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-18 16:15 ` João Távora
2024-04-18 16:29 ` Eli Zaretskii
2024-04-18 16:21 ` Eli Zaretskii
1 sibling, 1 reply; 96+ messages in thread
From: João Távora @ 2024-04-18 16:15 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036, felician.nemeth
On Thu, Apr 18, 2024 at 5:11 PM Theodor Thornhill <theo@thornhill.no> wrote:
> > Profiles can mislead and they can lie. It is much easier to time the
> > old and the new code doing the same jobs, and compare the times.
> >
> >> If so, I think this simpler patch after my sig is all we need, as it
> >> completely clears the profile of any "file-truename". I have reverted
> >> the earlier patch and pushed a patch very similar to the one after my sig.
>
> That's unfortunate.
For you maybe. From my standpoint noone else besides you has complained
about these problems, and I am still Eglot maintainer and must protect
against regressions.
> > This new code should also be timed and compared to the other two
> > versions, before we make the final decision on this.
>
> This is a very unfortunate change. This completely misses the point and
> reverts any perf gains from my previous patch... I think your
> conclusions are too quick, and actions likewise.
>
> I'd argue that long paths are a way more common occurrence than
> symlinking, and the places you touched in your "better way" ignores the
> performance critical parts.
>
> The function you are suggesting isn't the hotspot. Most notable
> publishDiagnostics is, which now is killed again.
Then you should publish details for a reproducible experiment
and we can take it from there.
The experiments I ran were the ones I could gather from a reading
of your profiles. The patch I used was very effective in solving
the performance effects.
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 16:11 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-18 16:15 ` João Távora
@ 2024-04-18 16:21 ` Eli Zaretskii
1 sibling, 0 replies; 96+ messages in thread
From: Eli Zaretskii @ 2024-04-18 16:21 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: 70036, felician.nemeth, joaotavora
> Date: Thu, 18 Apr 2024 18:11:36 +0200
> From: Theodor Thornhill <theo@thornhill.no>
> Cc: João Távora <joaotavora@gmail.com>,
> felician.nemeth@gmail.com, 70036@debbugs.gnu.org
>
> >> If so, I think this simpler patch after my sig is all we need, as it
> >> completely clears the profile of any "file-truename". I have reverted
> >> the earlier patch and pushed a patch very similar to the one after my sig.
>
> That's unfortunate.
>
> >
> > This new code should also be timed and compared to the other two
> > versions, before we make the final decision on this.
>
>
> This is a very unfortunate change. This completely misses the point and
> reverts any perf gains from my previous patch... I think your
> conclusions are too quick, and actions likewise.
>
> I'd argue that long paths are a way more common occurrence than
> symlinking, and the places you touched in your "better way" ignores the
> performance critical parts.
Please show timing data which compares the possible variants. We must
have quantitative data to have a firm basis for this discussion. I
don't yet have an opinion what we should do about this issue, and I
cannot form an opinion unless I see some real-life measurements. I
asked João to present such measurements as well.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 16:12 ` João Távora
@ 2024-04-18 16:24 ` Eli Zaretskii
2024-04-18 16:33 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-18 17:27 ` João Távora
0 siblings, 2 replies; 96+ messages in thread
From: Eli Zaretskii @ 2024-04-18 16:24 UTC (permalink / raw)
To: João Távora; +Cc: felician.nemeth, 70036, theo
> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 18 Apr 2024 17:12:34 +0100
> Cc: theo@thornhill.no, felician.nemeth@gmail.com, 70036@debbugs.gnu.org
>
> > Profiles can mislead and they can lie.
>
> Theo's profiles attached to this issue were the best I could find.
> If the manner in which they were collected was enough to make a
> decision in a certainway, they should be good enough to make a decision
> in another way.
>
> But I fully agree we should have a more constrained test case.
>
> > It is much easier to time the
> > old and the new code doing the same jobs, and compare the times.
>
> So... that's what I did.
If you timed this code with the likes of benchmark-run, please show
the timings. Profiles are not the best instrument for this kind of
decisions.
> Do you have a concrete idea of what this "job" is?
No, sorry. If nothing else comes to mind, I think Theo had such a
scenario.
> Of course. Let's have Theo do this comparison and perhaps describe
> in more detail the conditions in which he collected his profiles
Sure, it doesn't matter who does the measurements, as long as we have
them.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 16:15 ` João Távora
@ 2024-04-18 16:29 ` Eli Zaretskii
2024-04-18 17:22 ` João Távora
0 siblings, 1 reply; 96+ messages in thread
From: Eli Zaretskii @ 2024-04-18 16:29 UTC (permalink / raw)
To: João Távora; +Cc: felician.nemeth, 70036, theo
> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 18 Apr 2024 17:15:52 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, felician.nemeth@gmail.com, 70036@debbugs.gnu.org
>
> On Thu, Apr 18, 2024 at 5:11 PM Theodor Thornhill <theo@thornhill.no> wrote:
>
> > > Profiles can mislead and they can lie. It is much easier to time the
> > > old and the new code doing the same jobs, and compare the times.
> > >
> > >> If so, I think this simpler patch after my sig is all we need, as it
> > >> completely clears the profile of any "file-truename". I have reverted
> > >> the earlier patch and pushed a patch very similar to the one after my sig.
> >
> > That's unfortunate.
>
> For you maybe.
For me also.
And please try to be kinder. Theo might have made a mistake (or not),
but he is still a valued contributor, so please treat him with respect
he deserves.
> From my standpoint noone else besides you has complained about these
> problems, and I am still Eglot maintainer and must protect against
> regressions.
Yes, but that doesn't mean you should rush to revert others' changes
while the discussion is still on-going and the conclusions are not yet
in sight. Please don't, it hurts people and makes them less willing
to contribute. No catastrophe would have happened if you waited for a
while before making the changes.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 16:24 ` Eli Zaretskii
@ 2024-04-18 16:33 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-18 16:36 ` Eli Zaretskii
2024-04-18 17:26 ` João Távora
2024-04-18 17:27 ` João Távora
1 sibling, 2 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-18 16:33 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 70036, felician.nemeth, João Távora
[-- Attachment #1: Type: text/html, Size: 1967 bytes --]
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 16:33 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-18 16:36 ` Eli Zaretskii
2024-04-18 17:26 ` João Távora
1 sibling, 0 replies; 96+ messages in thread
From: Eli Zaretskii @ 2024-04-18 16:36 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: 70036, felician.nemeth, joaotavora
> Date: Thu, 18 Apr 2024 18:33:36 +0200
> From: Theodor Thornhill <theo@thornhill.no>
> Cc: João Távora <joaotavora@gmail.com>,
> felician.nemeth@gmail.com, 70036@debbugs.gnu.org
>
> I'll make a simple recipe this evening. It will require an lsp server install, likely, but I'll do my best to keep it
> contained.
Thanks, comparative timings are the best means to judging our
possibilities.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 16:29 ` Eli Zaretskii
@ 2024-04-18 17:22 ` João Távora
2024-04-18 17:53 ` Eli Zaretskii
0 siblings, 1 reply; 96+ messages in thread
From: João Távora @ 2024-04-18 17:22 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: felician.nemeth, 70036, theo
On Thu, Apr 18, 2024 at 5:30 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > > That's unfortunate.
> >
> > For you maybe.
>
> For me also.
>
> And please try to be kinder. Theo might have made a mistake (or not),
> but he is still a valued contributor, so please treat him with respect
> he deserves.
I'm not being unkind at all.
So let's skip the morals. I've told you I'm invested in fixing Theo's
performance problems, but I must learn more about them obviously.
But I'm not going to keep a regression in just to be nice to
people. I'm sure Theo can understand that. He's done lots of
useful contributions and will do more, but this one was a mistake.
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 16:33 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-18 16:36 ` Eli Zaretskii
@ 2024-04-18 17:26 ` João Távora
1 sibling, 0 replies; 96+ messages in thread
From: João Távora @ 2024-04-18 17:26 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036, felician.nemeth
On Thu, Apr 18, 2024 at 5:33 PM Theodor Thornhill <theo@thornhill.no> wrote:
> I'll make a simple recipe this evening. It will require an lsp server install, likely, but I'll do my best to keep it contained.
OK, but it would be preferable if this performance situation could
be reproduced with any language server in a project of certain
characteristics.
So if you say this happens with a project with a very deep directory hierarchy,
it's ideal if you could describe a generic project so I can reproduce
results with
clangd, basedpyright, gopls, rust-analyzer or any easy-to-get server. jdtls
is notoriously hard to install and get running, so I hope that's not the one
you'll pick.
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 16:24 ` Eli Zaretskii
2024-04-18 16:33 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-18 17:27 ` João Távora
1 sibling, 0 replies; 96+ messages in thread
From: João Távora @ 2024-04-18 17:27 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: felician.nemeth, 70036, theo
On Thu, Apr 18, 2024 at 5:24 PM Eli Zaretskii <eliz@gnu.org> wrote:
> If you timed this code with the likes of benchmark-run, please show
> the timings. Profiles are not the best instrument for this kind of
> decisions.
I disagree completely, but whatever. It's microbenchmarks that are
useless, because they don't have any context. But if you want
some of these, here you go. I benchmarked eglot--TextDocumentIdentifier
because that's the only reasonable thing I could gather from the
profile.
;; before Theodor's patch
(benchmark-run 10000 (eglot--TextDocumentIdentifier)) ;; (1.716975213
21 0.8623904230000008)
;; after Theodor's patch
(benchmark-run 10000 (eglot--TextDocumentIdentifier)) ;; (0.651663934
11 0.46295383499999687)
;; after I reverted Theodor's patch and added my patch
(benchmark-run 10000 (eglot--TextDocumentIdentifier)) ;; (0.000280462
0 0.0) yes, this is just reading a cached variable.
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 17:22 ` João Távora
@ 2024-04-18 17:53 ` Eli Zaretskii
2024-04-18 20:21 ` João Távora
0 siblings, 1 reply; 96+ messages in thread
From: Eli Zaretskii @ 2024-04-18 17:53 UTC (permalink / raw)
To: João Távora; +Cc: felician.nemeth, 70036, theo
> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 18 Apr 2024 18:22:09 +0100
> Cc: theo@thornhill.no, felician.nemeth@gmail.com, 70036@debbugs.gnu.org
>
> On Thu, Apr 18, 2024 at 5:30 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > > > That's unfortunate.
> > >
> > > For you maybe.
> >
> > For me also.
> >
> > And please try to be kinder. Theo might have made a mistake (or not),
> > but he is still a valued contributor, so please treat him with respect
> > he deserves.
>
> I'm not being unkind at all.
I suggest to get an objective opinion of that from an uninvolved
party.
> So let's skip the morals.
No, let's not. Let's try being kind and cooperative and respectful to
the work of others, exactly like you expect us to respect you and
yours.
> I've told you I'm invested in fixing Theo's performance problems,
> but I must learn more about them obviously. But I'm not going to
> keep a regression in just to be nice to people.
If -- and it's stil an "if" -- it was a regression, it was there for
quite some time, so nothing serious would have happened if we'd leave
it there for a few more hours or days.
> I'm sure Theo can understand that.
He obviously felt hurt, so "understand" is not an appropriate word
here.
> He's done lots of useful contributions and will do more, but this
> one was a mistake.
We shall see. For now, I reserve my judgment until we see
measurements.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 17:53 ` Eli Zaretskii
@ 2024-04-18 20:21 ` João Távora
[not found] ` <874jbycrd7.fsf@dick>
` (3 more replies)
0 siblings, 4 replies; 96+ messages in thread
From: João Távora @ 2024-04-18 20:21 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: felician.nemeth, 70036, theo
On Thu, Apr 18, 2024 at 6:53 PM Eli Zaretskii <eliz@gnu.org> wrote:
> I suggest to get an objective opinion of that from an uninvolved
> party.
I suggest you drop your sarcasm not because it hurts me or anything
but just because you're not very good at it.
> > So let's skip the morals.
>
> No, let's not.
Really, you want to go at it again? Well let me just add a pinch of
salt to your typical condescension lecture, getting some facts straight
about what happened here.
* I had to take a long timeout from Eglot maintenance for personal
reasons managed to get maintainers for Flymake and Jsonrpc, but I am
still Eglot maintainer.
* I notice some discussions going on and I let them proceed with
little input.
* I regain some capacity some months later, notice a user report
on GitHub alerts that there are some very new things in Eglot that
didn't ask for explicit greenlighting as was usual during the last 7
years I'm maintaining Eglot. Fair enough, life went on without me.
* I post to Emacs-devel about these. I take care to thank everyone
for their efforts.
* One of the changes is Stefan Monnier's. It's broken, he fixes it
immediately in the usual good spirits.
* Another one of the changes is Theodor's. It's a good change and
I highlight it as such, merely request a change to the doc.
* The other change is also Theodor's. I commend it for its clarity
and implementation, but ultimately notice a serious bug that affects
me and anyone else working a super-common C/C++ servers and symlinks.
* I go and read the full bug report and notice that Theodor has been
"studying Eglots performance" and has come to the conclusion that
there is a hotspot of performance problems. He posts in that email
what to this time is the only actual hard data we have. That
data suggests that file-truename is slow, and that Eglot uses it
a lot in eglot--TextDocumentIdentifier.
It doesn't suggest anything other than it, nothing about
textDocument/publishDiagnostics. There's 0 data about that
and it never showed up in my profile.
* Theodor suggests rewriting file-truneame in C, you decline or raise
doubts. I don't follow the reasons, but fair enough, the discussion
pivots to changing Eglot.
* I reproduce Theodor's experimental findings.
* I find a patch to address that hotspot that is several times
faster than Theodor's solution (doesn't really matter though).
* I ask Theodor to revert the patch and start afresh. He understands
but declines.
* I weigh the pros and cons, especially the time I have to address this
and other Eglot recent problems (tests are borked, have to see what happened)
Since I am Eglot maintainer and revert it myself, just like I've
reverted my and others own perfectly well-intentioned and laboriously
crafted commits many times and it hurts a bit to do it, but not that much.
It's a no-brainer to revert something like this to me. There is so far
very little hard evidence of the effect this is having on the general
population, it's a very uncommon report. This code has been there
for 7 years and while there have been performance bugs, this was never
one of it. This is why I write "one-off" and "uncommon". Absolutely
accurate. Ultimately, I see that very little testing has been done around some
rather obvious use cases of removing symlink-smarts from Eglot. The "my
servers happen not to have this" just isn't an acceptable argument to me.
Sure if Eglot had behaved like this all along from the beginning, displaying
duplicated references in some servers, maybe it would be different, but
that's a different world. Bottom line, Eglot worked correctly three weeks
ago and now has a new bug introduced by a performance fix based on very
little hard data, and a single very scarse report. It's a no brainer
to revert.
Still, for the very little data that there is available, I do take care
to put in a much simpler fix that completely fixes the issue - as far as
I or anyone reading the available data can see it.
Your last question to Theodor on this thread before the change was merge
good but the answer was completely misinterpreted. You ask if the servers
resolve symlinks and Theodor answers this:
> The LSP specification does not talk about symlinks. The
> servers I used let the operating system resolve symlinks for them.
Well, maybe at that point if one would have noticed that there are
hundreds of servers arounds, this could have taken a different turn.
> > I've told you I'm invested in fixing Theo's performance problems,
> > but I must learn more about them obviously. But I'm not going to
> > keep a regression in just to be nice to people.
>
> If -- and it's stil an "if" -- it was a regression,
Did you not read my experiment with the three-file project and the
clangd server?
https://lists.gnu.org/archive/html/emacs-devel/2024-04/msg00350.html
Is anything unclear? If you've seen it but don't believe my results
maybe you should try Eglot yourself: seeing is believing, they say.
> it was there for quite some time
So was the supposed relevant performance problem. Only that time is
measured in years and this is measured in weeks.
> , so nothing serious would have happened if we'd leave
> it there for a few more hours or days.
And nothing serious will happen after I reverted it, will there?
I have no interest in delaying a responsible decision just for the
sake of appeasing feelings that someone else says are there. If
Theodor is worried about a specific performance problem I have some
time this week and the next one to help fix it, and I'm confident I will.
For the record my repositories at $DAYJOB with very long path names
_and_ very symlinks. So I'm personally interested in fixing any
performance problems and not opening new ones.
> > I'm sure Theo can understand that.
>
> He obviously felt hurt, so "understand" is not an appropriate word
> here.
I'm confident he will understand this, I can't be sure of course.
But I know he wrote "could absolutely do that [revert]" though he preferred
not to do it. That at least shows that it's not an absurd proposition.
And _I_ understand that he doesn't want to, of course.
I don't know if Theodor felt hurt, and even if I suspected something, I
wouldn't write about it in public simply because I personally find it
in poor taste to speculate or moralize about others people's feelings
secondhand.
I await Theo's reproducible experiment so that we can work on what's
really important.
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
[not found] ` <874jbycrd7.fsf@dick>
@ 2024-04-18 21:26 ` João Távora
2024-04-18 21:37 ` João Távora
0 siblings, 1 reply; 96+ messages in thread
From: João Távora @ 2024-04-18 21:26 UTC (permalink / raw)
To: dick; +Cc: theo, Eli Zaretskii, 70036, felician.nemeth
On Thu, Apr 18, 2024 at 10:07 PM dick <dick.r.chiang@gmail.com> wrote:
> Speaking of dicks, you are one, and your code is always dogshit, so your
> much heralded leavetaking of the emacs project can't come soon enough.
My dear dick, you on the other hand, are such a lovely wonderful human being
who writes such great code and is so courageous.. Thank you for
gracing me with
your attention. Means a lot to me. Love xoxo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 20:21 ` João Távora
[not found] ` <874jbycrd7.fsf@dick>
@ 2024-04-18 21:32 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-18 22:06 ` João Távora
2024-04-19 6:45 ` Eli Zaretskii
2024-04-19 0:57 ` Yuan Fu
2024-04-22 22:11 ` Dmitry Gutov
3 siblings, 2 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-18 21:32 UTC (permalink / raw)
To: João Távora; +Cc: Eli Zaretskii, 70036, felician.nemeth
[-- Attachment #1: Type: text/html, Size: 95068 bytes --]
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 21:26 ` João Távora
@ 2024-04-18 21:37 ` João Távora
2024-04-19 9:17 ` Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 96+ messages in thread
From: João Távora @ 2024-04-18 21:37 UTC (permalink / raw)
To: dick; +Cc: theo, Eli Zaretskii, 70036, felician.nemeth
On Thu, Apr 18, 2024 at 10:26 PM João Távora <joaotavora@gmail.com> wrote:
>
> On Thu, Apr 18, 2024 at 10:07 PM dick <dick.r.chiang@gmail.com> wrote:
>
> > Speaking of dicks, you are one, and your code is always dogshit, so your
> > much heralded leavetaking of the emacs project can't come soon enough.
>
> My dear dick, you on the other hand, are such a lovely wonderful human being
> who writes such great code and is so courageous.. Thank you for
> gracing me with
> your attention. Means a lot to me. Love xoxo
And I say this even without having the faintest idea of who you are,
imagine if I did :-)
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 21:32 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-18 22:06 ` João Távora
2024-04-18 23:59 ` João Távora
` (2 more replies)
2024-04-19 6:45 ` Eli Zaretskii
1 sibling, 3 replies; 96+ messages in thread
From: João Távora @ 2024-04-18 22:06 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036, felician.nemeth
On Thu, Apr 18, 2024 at 10:32 PM Theodor Thornhill <theo@thornhill.no> wrote:
>
> Im having some email client issues. I hope this mail renders readable.
> > It doesn't suggest anything other than it, nothing about
> > textDocument/publishDiagnostics. There's 0 data about that
> > and it never showed up in my profile.
>
> I don't remember exactly right now, but I believe I mentioned many times
> the performance of file-truename directly, and find-buffer-visiting
> which uses file-truename indirectly.
Yes, but that's not the same as saying there's data about it. Your
profiles don't show find-buffer-visiting as a hotspot. Or did I miss
any?
> Did I decline? I've lost track, but I believe I said something in the
> lines of "let's discuss it first", but nevermind.
Well, that's declining. Politely, but declining :)
> > Still, for the very little data that there is available, I do take care
> > to put in a much simpler fix that completely fixes the issue - as far as
> > I or anyone reading the available data can see it.
>
> With this I disagree - but I guess I either have miscommunicated or have
> provided unclear profiles or something inbetween.
I scanned the bug thread twice and couldn't find any other profiles.
There are mentions of textDocument/publishDiagnostics, but no actual
profile data or information of how to see the performance problem.
Maybe _I_ missed something. But I see you have now sent a perfect
reproduction, so it doesn't matter.
> > And nothing serious will happen after I reverted it, will there?
>
> Not serious, but now we have a performance regression ;-)
As far as we can tell, *you* have one. Noone else has -- as far as I
can tell from the 7 years that code has been there. So I wouldn't
really call this a "regression", but that doesn't mean I won't try
to solve it. I'm confident it can be done. Incidentally I work on
deep repos *with* symlinks, so I'm personally interested in the solution
too.
> I'm fine. I do think you do come off strongly though, and it is not
> always easy to judge if it is of good faith.
Just think: what reason do I have to be in "bad faith"? I just invited
you to collaborate in the GitHub repo and I want you to keep learning
and improving Eglot. I just think you did a commit -- in perfectly
good faith -- that clashes directly with what is a tenet of Eglot:
correctness and backward compatibility. I also think there are other
much better ways to fix the problem (the greatest of these ideas is
yours!!! rewrite it is C).
> And I don't think Eli moralizes on my behalf.
I don't know why it's done and I don't care, but I think it helps noone.
> 1. Install gopls
> 2. Make some directory you can wipe out after the test and cd into it
> 3. git clone git@github.com:theothornhill/gin.git foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/gin
> 4. cd foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/gin
> 5. open fs.go in emacs and make sure some go mode is available. Go-ts-mode for example
> 6. M-x profiler-start
> 7. M-x eglot
> 8. Wait 10-20 seconds. Do no actions other than let the lsp settle.
> 9. M-x profiler-stop
> 10. M-x profiler-report
> 11. Rinse repeat with both or all variants of emacs with/without the
> latest eglot changes.
> I'll add my profiles, and let some metrics talk. This is from emacs -Q
> from your "better fix" and the commit before your revert. Please try on
> your systems and report back.
Great recipe.
> By the way. I have a hunch the reason this is so apparent now is because
> of the faster json serde recently added to emacs. Not sure, but feels
> like it. Json serde is almost gone from the profiles.
What is Json serde? Is it Json SERialization/DEserialization?
That commit by Hermann made it in? That's great!! So Eglot is probably
faster overall. So I guess your bug report is about not missing
a further optimization opportunity. Fine.
Anyway is this the hotspot I should be trying to optimize?
> 9 4% - find-buffer-visiting
I still think the cleanest solution is to write file-truename
in C. But if that can't be done, it doesn't seem terribly hard
to get rid of find-buffer-visiting in publishDiagnostics and
still remain symlink-correct.
That's because every interesting result of find-buffer-visiting
is a buffer for which we've already issued a 'didOpen', which
in turn means we've already called file-truename once for it.
If we cache that result (the basic idea behind the "better fix")
it should be possible to find the buffer quicker just by iterating
the list. That's what I will try to do, using your recipe as
guide.
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 22:06 ` João Távora
@ 2024-04-18 23:59 ` João Távora
2024-04-19 6:09 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 5:58 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 6:56 ` Eli Zaretskii
2 siblings, 1 reply; 96+ messages in thread
From: João Távora @ 2024-04-18 23:59 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036, felician.nemeth
[-- Attachment #1: Type: text/plain, Size: 4856 bytes --]
On Thu, Apr 18, 2024 at 11:06 PM João Távora <joaotavora@gmail.com> wrote:
> Anyway is this the hotspot I should be trying to optimize?
>
> > 9 4% - find-buffer-visiting
Alright, i've reproduced this
33 5% - eglot-handle-notification
33 5% - apply
33 5% - #<compiled-function B79>
29 4% - find-buffer-visiting
29 4% - file-truename
29 4% - file-truename
25 4% - file-truename
25 4% - file-truename
25 4% - file-truename
25 4% - file-truename
25 4% + file-truename
But I have to say, I wouldn't call this a severe performance penalty.
I followed your instructions very closely and invoked Emacs like this:
emacs -Q foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/gin/fs.go -f
go-ts-mode
The directory is ~/tmp/theo/foo/bar... so it's a pretty long path with
many directories all in all.
But I didn't have to wait 10 seconds for the LSP to settle down! It was
pretty snappy on my 2018 Lenovo T480 running Archlinux. And if I
profile anything other than the initial M-x eglot (which normally happens
only once in a work session), I don't find any file-truename in the profile.
So my perception is that it must have spent around 4% of 1 second in
file-truename.
Anyway the reason this shows in this profile is because this project
with this particular server sends a lot of publishDiagnostics upfront.
That's OK. Gopls is a very good server. I think I see a fix. But can
you qualitatively describe the Eglot experience. Do you notice any
input lag or something like that? With this project? I didn't feel _any_
lag.
Super snappy. Maybe the JSON serde kicking in?
Anyway, the idea I suggested earlier is in the patch after my sig.
Let's think: LSP's publishDiagnostics coming from the server deals
in URIs, right? And we inform the LSP server about URIs, too, right?
So the URI is always LSP's idea of the resource identifier (and it likes
to have truename URI).
My last "better fix" commit records this URI in the buffer as a buffer
local variable eglot--cached-tdi and it has to do that for every didOpen.
So, to find an open buffer pertaining to a certain LSP's publishDiagnostics
it suffices in theory to go through all the buffers that have a non-nil
cached
URI and compare that.
No need to convert from URI to file names, not for this job at least!
I tried this and it worked fine.
When I do that, the profile is completely free of those 4% that
bothered you.
I'm still testing this for edge cases and will sleep on it, but it seems
promisingly simple at least. I can't run unit tests right now, because
a recent adventurous commit by Stefan Monnier broke them all :-)
but I'm confident that will be fixed soon...
I hope you can try this patch.
João
diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 90a607075d3..38a16b15e4c 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -2381,6 +2381,9 @@ eglot-handle-notification
(lambda ()
(remhash token (eglot--progress-reporters
server))))))))))
+(defvar-local eglot--cached-tdi nil
+ "A cached LSP TextDocumentIdentifier URI string.")
+
(cl-defmethod eglot-handle-notification
(_server (_method (eql textDocument/publishDiagnostics)) &key uri
diagnostics
&allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
@@ -2391,9 +2394,14 @@ eglot-handle-notification
((= sev 2) 'eglot-warning)
(t 'eglot-note)))
(mess (source code message)
- (concat source (and code (format " [%s]" code)) ": "
message)))
+ (concat source (and code (format " [%s]" code)) ": "
message))
+ (find-it (uri)
+ (cl-loop for b in (buffer-list)
+ when (with-current-buffer b
+ (equal eglot--cached-tdi uri))
+ return b)))
(if-let* ((path (expand-file-name (eglot-uri-to-path uri)))
- (buffer (find-buffer-visiting path)))
+ (buffer (find-it uri)))
(with-current-buffer buffer
(cl-loop
initially
@@ -2518,9 +2526,6 @@ eglot-handle-request
(t (setq success :json-false)))
`(:success ,success)))
-(defvar-local eglot--cached-tdi nil
- "A cached LSP TextDocumentIdentifier URI string.")
-
(defun eglot--TextDocumentIdentifier ()
"Compute TextDocumentIdentifier object for current buffer."
`(:uri ,(or eglot--cached-tdi
[-- Attachment #2: Type: text/html, Size: 6018 bytes --]
^ permalink raw reply related [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 20:21 ` João Távora
[not found] ` <874jbycrd7.fsf@dick>
2024-04-18 21:32 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 0:57 ` Yuan Fu
2024-04-19 1:20 ` João Távora
2024-04-22 22:11 ` Dmitry Gutov
3 siblings, 1 reply; 96+ messages in thread
From: Yuan Fu @ 2024-04-19 0:57 UTC (permalink / raw)
To: João Távora; +Cc: theo, Eli Zaretskii, 70036, Felician Nemeth
> On Apr 18, 2024, at 1:21 PM, João Távora <joaotavora@gmail.com> wrote:
>
> On Thu, Apr 18, 2024 at 6:53 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
>> I suggest to get an objective opinion of that from an uninvolved
>> party.
>
> I suggest you drop your sarcasm not because it hurts me or anything
> but just because you're not very good at it.
I wouldn’t consider it sarcasm, I’m sure Eli does mean that he’s an uninvolved party, in the sense that he’s neither Theo nor you.
>
>>> So let's skip the morals.
>>
>> No, let's not.
>
> Really, you want to go at it again? Well let me just add a pinch of
> salt to your typical condescension lecture, getting some facts straight
> about what happened here.
I’m absolutely sure that you don’t want to hurt anyone on this list, and I’m sure you have way too many Emacs work to take care of in the limited personal time you got. But, I can see that to an average person in the English-speaking culture, your words can sound a bit harsh. Theo is an adult, yes, but why not be extra nice to each other when we can? To me, taking some extra care for our fellow Emacs developers in this cold world is definitely well worth it, even if it means writing longer sentences :-)
I hope you don’t take Eli’s reply as sarcasm plus lecture, because I can see that he definitely doesn’t mean it. He’s trying to maintain a nice, friendly environment for Emacs developers, which is part of the job of a maintainer. He’s succinctness in phrasing probably didn’t help here, but given the amount of messages he needs to reply everyday, I can’t really blame him ;-)
And please don’t take this as a criticism, it’s understandable to feel what you felt in the conversation. I just want to share a more benefit-of-the-doubt-y view from a third-party. I’ll be very sad if you, Eli, and any fellow developers ends up in heated arguments and bad mood because of misunderstandings and miscommunication.
Yuan
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 0:57 ` Yuan Fu
@ 2024-04-19 1:20 ` João Távora
0 siblings, 0 replies; 96+ messages in thread
From: João Távora @ 2024-04-19 1:20 UTC (permalink / raw)
To: Yuan Fu; +Cc: theo, Eli Zaretskii, 70036, Felician Nemeth
[-- Attachment #1: Type: text/plain, Size: 999 bytes --]
> > On Apr 18, 2024, at 1:21 PM, João Távora <joaotavora@gmail.com> wrote:
>
> And please don’t take this as a criticism
It's fine Yuan. I don't. My relationship with Eli is past mending, and it
doesn't
need to be mended.
I'm in this thread since this afternoon to understand and fix the
performance
problem without introducing a functional regression and I'm well on my way
to doing all those three things together with the person who I so violated
by reverting one of their commits among many.
Then I will fix Eglot's unit tests, or Stefan will, and then I will be
gone, keep
a closer eye on these Eglot proposals since I notice that some would-be
maintainers don't (yet) have a good grasp on tests, server variability, etc.
I think not many Emacs old-timers are using Eglot yet and that's crucial.
But the situation is improving slowly so eventually that too will come
sooner or later and then I will leave you more definitely and wish
you lots of success.
João
[-- Attachment #2: Type: text/html, Size: 1473 bytes --]
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 22:06 ` João Távora
2024-04-18 23:59 ` João Távora
@ 2024-04-19 5:58 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 7:52 ` João Távora
2024-04-19 6:56 ` Eli Zaretskii
2 siblings, 1 reply; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-19 5:58 UTC (permalink / raw)
To: João Távora; +Cc: Eli Zaretskii, 70036, felician.nemeth
>> I'm fine. I do think you do come off strongly though, and it is not
>> always easy to judge if it is of good faith.
>
> Just think: what reason do I have to be in "bad faith"? I just invited
> you to collaborate in the GitHub repo and I want you to keep learning
> and improving Eglot. I just think you did a commit -- in perfectly
> good faith -- that clashes directly with what is a tenet of Eglot:
> correctness and backward compatibility. I also think there are other
> much better ways to fix the problem (the greatest of these ideas is
> yours!!! rewrite it is C).
>
I agree - that's why I want to focus on improving the code, same as
you. I just want you to understand that being mindful on how you come
across can't really hurt. There are enough conflicts around these days,
let's not make code be one.
>> 1. Install gopls
>> 2. Make some directory you can wipe out after the test and cd into it
>> 3. git clone git@github.com:theothornhill/gin.git foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/gin
>> 4. cd foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/gin
>> 5. open fs.go in emacs and make sure some go mode is available. Go-ts-mode for example
>> 6. M-x profiler-start
>> 7. M-x eglot
>> 8. Wait 10-20 seconds. Do no actions other than let the lsp settle.
>> 9. M-x profiler-stop
>> 10. M-x profiler-report
>> 11. Rinse repeat with both or all variants of emacs with/without the
>> latest eglot changes.
>> I'll add my profiles, and let some metrics talk. This is from emacs -Q
>> from your "better fix" and the commit before your revert. Please try on
>> your systems and report back.
>
> Great recipe.
>
Good!
>> By the way. I have a hunch the reason this is so apparent now is because
>> of the faster json serde recently added to emacs. Not sure, but feels
>> like it. Json serde is almost gone from the profiles.
>
> What is Json serde? Is it Json SERialization/DEserialization?
That's right
> That commit by Hermann made it in? That's great!! So Eglot is probably
> faster overall. So I guess your bug report is about not missing
> a further optimization opportunity. Fine.
Yeah, that's right. I assume that when json serde disappeared as a
bottleneck others emerge. This is the obvious one to me right now.
>
> Anyway is this the hotspot I should be trying to optimize?
>
>> 9 4% - find-buffer-visiting
>
> I still think the cleanest solution is to write file-truename
> in C. But if that can't be done, it doesn't seem terribly hard
> to get rid of find-buffer-visiting in publishDiagnostics and
> still remain symlink-correct.
I agree, and I'm still experimenting on that. But I need to make sure I
have good test coverage on the current behavior before I start making
the C one. The initial version, while fast, was just to prove that
file-truename should be improved.
>
> That's because every interesting result of find-buffer-visiting
> is a buffer for which we've already issued a 'didOpen', which
> in turn means we've already called file-truename once for it.
> If we cache that result (the basic idea behind the "better fix")
> it should be possible to find the buffer quicker just by iterating
> the list. That's what I will try to do, using your recipe as
> guide.
>
> João
Sure. That will make the change you make similar in spirit to mine, yet
with a different implementation. I'm fine with that.
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 23:59 ` João Távora
@ 2024-04-19 6:09 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 6:26 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 8:01 ` João Távora
0 siblings, 2 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-19 6:09 UTC (permalink / raw)
To: João Távora; +Cc: Eli Zaretskii, 70036, felician.nemeth
João Távora <joaotavora@gmail.com> writes:
> On Thu, Apr 18, 2024 at 11:06 PM João Távora <joaotavora@gmail.com> wrote:
>
>> Anyway is this the hotspot I should be trying to optimize?
>>
>> > 9 4% - find-buffer-visiting
>
> Alright, i've reproduced this
>
> 33 5% - eglot-handle-notification
> 33 5% - apply
> 33 5% - #<compiled-function B79>
> 29 4% - find-buffer-visiting
> 29 4% - file-truename
> 29 4% - file-truename
> 25 4% - file-truename
> 25 4% - file-truename
> 25 4% - file-truename
> 25 4% - file-truename
> 25 4% + file-truename
>
> But I have to say, I wouldn't call this a severe performance penalty.
> I followed your instructions very closely and invoked Emacs like this:
>
> emacs -Q foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/gin/fs.go -f
> go-ts-mode
>
> The directory is ~/tmp/theo/foo/bar... so it's a pretty long path with
> many directories all in all.
Great!
>
> But I didn't have to wait 10 seconds for the LSP to settle down! It was
> pretty snappy on my 2018 Lenovo T480 running Archlinux. And if I
> profile anything other than the initial M-x eglot (which normally happens
> only once in a work session), I don't find any file-truename in the profile.
>
This is true to some extent, but varies a lot from server to server, as
you've also noted earlier. I have an even stronger computer, a 2023 P1
gen 5, I believe, running ubuntu. Some servers send _all_ diagnostics on
every keystroke.
> So my perception is that it must have spent around 4% of 1 second in
> file-truename.
>
> Anyway the reason this shows in this profile is because this project
> with this particular server sends a lot of publishDiagnostics upfront.
> That's OK. Gopls is a very good server. I think I see a fix. But can
> you qualitatively describe the Eglot experience. Do you notice any
> input lag or something like that? With this project? I didn't feel _any_
> lag.
> Super snappy. Maybe the JSON serde kicking in?
>
In this particular project I don't notice any input lag. But on every
java project at work I absolutely do. With my fix I don't. So if we
solve this in any particular way that will be a huge benefit for
everyone using java and other either suboptimal lsp servers or languages
in general.
> Anyway, the idea I suggested earlier is in the patch after my sig.
>
> Let's think: LSP's publishDiagnostics coming from the server deals
> in URIs, right? And we inform the LSP server about URIs, too, right?
> So the URI is always LSP's idea of the resource identifier (and it likes
> to have truename URI).
Sure, like in my key/value store. (apart from symlinking)
>
> My last "better fix" commit records this URI in the buffer as a buffer
> local variable eglot--cached-tdi and it has to do that for every didOpen.
>
> So, to find an open buffer pertaining to a certain LSP's publishDiagnostics
> it suffices in theory to go through all the buffers that have a non-nil
> cached
> URI and compare that.
>
> No need to convert from URI to file names, not for this job at least!
> I tried this and it worked fine.
>
> When I do that, the profile is completely free of those 4% that
> bothered you.
>
> I'm still testing this for edge cases and will sleep on it, but it seems
> promisingly simple at least. I can't run unit tests right now, because
> a recent adventurous commit by Stefan Monnier broke them all :-)
> but I'm confident that will be fixed soon...
>
> I hope you can try this patch.
I will :)
>
> João
>
> diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
> index 90a607075d3..38a16b15e4c 100644
> --- a/lisp/progmodes/eglot.el
> +++ b/lisp/progmodes/eglot.el
> @@ -2381,6 +2381,9 @@ eglot-handle-notification
> (lambda ()
> (remhash token (eglot--progress-reporters
> server))))))))))
>
> +(defvar-local eglot--cached-tdi nil
> + "A cached LSP TextDocumentIdentifier URI string.")
> +
> (cl-defmethod eglot-handle-notification
> (_server (_method (eql textDocument/publishDiagnostics)) &key uri
> diagnostics
> &allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
> @@ -2391,9 +2394,14 @@ eglot-handle-notification
> ((= sev 2) 'eglot-warning)
> (t 'eglot-note)))
> (mess (source code message)
> - (concat source (and code (format " [%s]" code)) ": "
> message)))
> + (concat source (and code (format " [%s]" code)) ": "
> message))
> + (find-it (uri)
> + (cl-loop for b in (buffer-list)
Could we rather use eglot--managed-buffers, like in my patch? there
shouldn't be a need to loop through say 200 buffers that are unrelated
to the project in question, right? Apart from this I agree, and will try
it.
Thanks,
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 6:09 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 6:26 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 8:06 ` João Távora
2024-04-19 8:01 ` João Távora
1 sibling, 1 reply; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-19 6:26 UTC (permalink / raw)
To: João Távora; +Cc: Eli Zaretskii, 70036, felician.nemeth
>
> Could we rather use eglot--managed-buffers, like in my patch? there
> shouldn't be a need to loop through say 200 buffers that are unrelated
> to the project in question, right? Apart from this I agree, and will try
> it.
Like this?
I can confirm with yours and this patch the latency is down again, so
thanks.
I'll test a little the difference with/without eglot--managed-buffers.
Theo
diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 90a607075d3..3578aed92b4 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -2381,8 +2381,11 @@ eglot-handle-notification
(lambda ()
(remhash token (eglot--progress-reporters server))))))))))
+(defvar-local eglot--cached-tdi nil
+ "A cached LSP TextDocumentIdentifier URI string.")
+
(cl-defmethod eglot-handle-notification
- (_server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
+ (server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
&allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
"Handle notification publishDiagnostics."
(cl-flet ((eglot--diag-type (sev)
@@ -2391,9 +2394,14 @@ eglot-handle-notification
((= sev 2) 'eglot-warning)
(t 'eglot-note)))
(mess (source code message)
- (concat source (and code (format " [%s]" code)) ": " message)))
+ (concat source (and code (format " [%s]" code)) ": " message))
+ (find-it (uri)
+ (cl-loop for b in (eglot--managed-buffers server)
+ when (with-current-buffer b
+ (equal eglot--cached-tdi uri))
+ return b)))
(if-let* ((path (expand-file-name (eglot-uri-to-path uri)))
- (buffer (find-buffer-visiting path)))
+ (buffer (find-it uri)))
(with-current-buffer buffer
(cl-loop
initially
@@ -2518,9 +2526,6 @@ eglot-handle-request
(t (setq success :json-false)))
`(:success ,success)))
-(defvar-local eglot--cached-tdi nil
- "A cached LSP TextDocumentIdentifier URI string.")
-
(defun eglot--TextDocumentIdentifier ()
"Compute TextDocumentIdentifier object for current buffer."
`(:uri ,(or eglot--cached-tdi
Thanks,
Theo
^ permalink raw reply related [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 21:32 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-18 22:06 ` João Távora
@ 2024-04-19 6:45 ` Eli Zaretskii
2024-04-19 7:38 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 12:54 ` João Távora
1 sibling, 2 replies; 96+ messages in thread
From: Eli Zaretskii @ 2024-04-19 6:45 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: 70036, felician.nemeth, joaotavora
> Date: Thu, 18 Apr 2024 23:32:00 +0200
> From: Theodor Thornhill <theo@thornhill.no>
> Cc: Eli Zaretskii <eliz@gnu.org>, felician.nemeth@gmail.com,
> 70036@debbugs.gnu.org
>
> I'll add two profiles, one is profile-fast and profile-slow. You can
> guess which is which :)
>
> STEPS:
> I've made an intentionally broken commit into a golang repo I just
> forked, to get some publishDiagnostics quickly.
>
> (for good measure evaluate eglot.el before running profiles)
>
> 1. Install gopls
> 2. Make some directory you can wipe out after the test and cd into it
> 3. git clone git@github.com:theothornhill/gin.git foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/gin
> 4. cd foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/gin
> 5. open fs.go in emacs and make sure some go mode is available. Go-ts-mode for example
> 6. M-x profiler-start
> 7. M-x eglot
> 8. Wait 10-20 seconds. Do no actions other than let the lsp settle.
> 9. M-x profiler-stop
> 10. M-x profiler-report
> 11. Rinse repeat with both or all variants of emacs with/without the
> latest eglot changes.
> I'll add my profiles, and let some metrics talk.
Thanks, but that is not what I asked to provide, for us to make a
decision in this case. I asked to provide results of a benchmark-run
or similar measure of the run time. Profiles, by contrast, are much
harder to interpret when the issue is the overall time it takes to
perform some operations.
Could you please show benchmark times of the old code (before your
changes), the code after your changes, and the current code in Git
(after João reverted your change and installed his own improvements)?
This will allow us to see the times (both processing and GC) of each
variant, and will allow to compare their performance.
Thanks.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 22:06 ` João Távora
2024-04-18 23:59 ` João Távora
2024-04-19 5:58 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 6:56 ` Eli Zaretskii
2024-04-19 7:51 ` Ihor Radchenko
2024-04-19 8:27 ` João Távora
2 siblings, 2 replies; 96+ messages in thread
From: Eli Zaretskii @ 2024-04-19 6:56 UTC (permalink / raw)
To: João Távora; +Cc: felician.nemeth, 70036, theo
> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 18 Apr 2024 23:06:47 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, felician.nemeth@gmail.com, 70036@debbugs.gnu.org
>
> > > Still, for the very little data that there is available, I do take care
> > > to put in a much simpler fix that completely fixes the issue - as far as
> > > I or anyone reading the available data can see it.
> >
> > With this I disagree - but I guess I either have miscommunicated or have
> > provided unclear profiles or something inbetween.
>
> I scanned the bug thread twice and couldn't find any other profiles.
> There are mentions of textDocument/publishDiagnostics, but no actual
> profile data or information of how to see the performance problem.
>
> Maybe _I_ missed something. But I see you have now sent a perfect
> reproduction, so it doesn't matter.
There was no need to have any profiles to demonstrate that
file-truename is significantly slower than expand-file-name. It
should be clear just by looking at what file-truename does and how it
does that. Therefore, once I was told that resolving symlinks in
Emacs is unnecessary, replacing file-truename with expand-file-name
became a no-brainer, regardless of how many or how few percents of CPU
time it takes: it's just a waste of CPU cycles.
It's only now, that we decided symlinks _should_ be resolved by Emacs,
that quantitative performance of the code and the fraction of it due
to file-truename becomes relevant, and must be measured and compared
with alternatives.
> I still think the cleanest solution is to write file-truename
> in C.
I explained in that past discussion why this is not simple. So if
simpler solutions exist, we should prefer them.
> But if that can't be done, it doesn't seem terribly hard
> to get rid of find-buffer-visiting in publishDiagnostics and
> still remain symlink-correct.
find-buffer-visiting was made much faster lately, but that speedup
AFAIR shows up only if the session has a lot of buffers, so trying
these benchmarks in "emacs -Q" will not typically show the effect, and
could even obscure the file-truename effect as well, because the
number of calls to file-truename will be much smaller. But if calling
find-buffer-visiting from Eglot can be avoided, that is of course even
better.
> That's because every interesting result of find-buffer-visiting
> is a buffer for which we've already issued a 'didOpen', which
> in turn means we've already called file-truename once for it.
> If we cache that result (the basic idea behind the "better fix")
> it should be possible to find the buffer quicker just by iterating
> the list. That's what I will try to do, using your recipe as
> guide.
Thanks.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 6:45 ` Eli Zaretskii
@ 2024-04-19 7:38 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 12:54 ` João Távora
1 sibling, 0 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-19 7:38 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 70036, felician.nemeth, joaotavora
Eli Zaretskii <eliz@gnu.org> writes:
>> Date: Thu, 18 Apr 2024 23:32:00 +0200
>> From: Theodor Thornhill <theo@thornhill.no>
>> Cc: Eli Zaretskii <eliz@gnu.org>, felician.nemeth@gmail.com,
>> 70036@debbugs.gnu.org
>>
>> I'll add two profiles, one is profile-fast and profile-slow. You can
>> guess which is which :)
>>
>> STEPS:
>> I've made an intentionally broken commit into a golang repo I just
>> forked, to get some publishDiagnostics quickly.
>>
>> (for good measure evaluate eglot.el before running profiles)
>>
>> 1. Install gopls
>> 2. Make some directory you can wipe out after the test and cd into it
>> 3. git clone git@github.com:theothornhill/gin.git foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/gin
>> 4. cd foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/gin
>> 5. open fs.go in emacs and make sure some go mode is available. Go-ts-mode for example
>> 6. M-x profiler-start
>> 7. M-x eglot
>> 8. Wait 10-20 seconds. Do no actions other than let the lsp settle.
>> 9. M-x profiler-stop
>> 10. M-x profiler-report
>> 11. Rinse repeat with both or all variants of emacs with/without the
>> latest eglot changes.
>> I'll add my profiles, and let some metrics talk.
>
> Thanks, but that is not what I asked to provide, for us to make a
> decision in this case. I asked to provide results of a benchmark-run
> or similar measure of the run time. Profiles, by contrast, are much
> harder to interpret when the issue is the overall time it takes to
> perform some operations.
>
> Could you please show benchmark times of the old code (before your
> changes), the code after your changes, and the current code in Git
> (after João reverted your change and installed his own improvements)?
> This will allow us to see the times (both processing and GC) of each
> variant, and will allow to compare their performance.
>
> Thanks.
I'll try.
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 6:56 ` Eli Zaretskii
@ 2024-04-19 7:51 ` Ihor Radchenko
2024-04-19 10:51 ` Eli Zaretskii
2024-04-19 8:27 ` João Távora
1 sibling, 1 reply; 96+ messages in thread
From: Ihor Radchenko @ 2024-04-19 7:51 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: theo, 70036, felician.nemeth, João Távora
Eli Zaretskii <eliz@gnu.org> writes:
>> I still think the cleanest solution is to write file-truename
>> in C.
>
> I explained in that past discussion why this is not simple. So if
> simpler solutions exist, we should prefer them.
There are some easy things that can be done to improve `file-truename'
performance somewhat. For example, the number of calls to
`file-name-nondirectory' can be trivially reduced in the `file-truename'
code - it is called up to three times in a row.
(see my earlier message in https://yhetil.org/emacs-bugs/87jzlmd831.fsf@localhost/)
Will it be of interest?
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 5:58 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 7:52 ` João Távora
2024-04-19 9:14 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 96+ messages in thread
From: João Távora @ 2024-04-19 7:52 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036, felician.nemeth
On Fri, Apr 19, 2024 at 6:59 AM Theodor Thornhill <theo@thornhill.no> wrote:
> I agree - that's why I want to focus on improving the code, same as
> you. I just want you to understand that being mindful on how you come
> across can't really hurt.
You're right of course. I do try to be mindful, but maybe I made a mistake.
IIUC it was the part where I said this was a "one-off and uncommon" report
that you felt was unkind and harsh. Because you wrote that. I absolutely
think we miscommunicated: I wasn't trying to say you have a bizarre project
setup, I was simply trying to make a statement about statistics and
the relative importance.
> There are enough conflicts around these days,
In Emacs or in the world? If the former I'd rather not know about
them.
> let's not make code be one.
Right. So reverting another person's code isn't that big of a deal.
You "reverted" bits of my code when doing your commits, and that's
fine.
> Yeah, that's right. I assume that when json serde disappeared as a
> bottleneck others emerge. This is the obvious one to me right now.
When my hotspot for a given reasonable profile is 4%, I'm feeling
fairly good about my code. But yes, of course, it can be addressed.
> Sure. That will make the change you make similar in spirit to mine, yet
> with a different implementation. I'm fine with that.
With the difference that my change also still ultimately uses truename,
just very frugally. So it still understands symlinks, doesn't
mislead the server and doesn't expose the bug I demonstrated.
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 6:09 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 6:26 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 8:01 ` João Távora
2024-04-19 9:10 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 1 reply; 96+ messages in thread
From: João Távora @ 2024-04-19 8:01 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036, felician.nemeth
On Fri, Apr 19, 2024 at 7:09 AM Theodor Thornhill <theo@thornhill.no> wrote:
> you've also noted earlier. I have an even stronger computer, a 2023 P1
> gen 5, I believe, running ubuntu. Some servers send _all_ diagnostics on
> every keystroke.
That could be, but what servers are these?
Thought it would never be "on every keystroke" though. As you probably
know, Eglot bunches them up in didChange notifications, and you can
can even be the size of these bunches.
> > So my perception is that it must have spent around 4% of 1 second in
> > file-truename.
> >
> > Anyway the reason this shows in this profile is because this project
> > with this particular server sends a lot of publishDiagnostics upfront.
> > That's OK. Gopls is a very good server. I think I see a fix. But can
> > you qualitatively describe the Eglot experience. Do you notice any
> > input lag or something like that? With this project? I didn't feel _any_
> > lag.
> > Super snappy. Maybe the JSON serde kicking in?
> >
>
> In this particular project I don't notice any input lag. But on every
> java project at work I absolutely do.
But that could be down to other reasons Theodor. We would need data
for that server too. Last time I tried jdtls (on a docker-based TRAMP
setup, described somewhere in the mailing list), it was snappy.
If you make a simple recipe like this one I can absolutely
try though. The difficulty in that one was TRAMP.
> > Anyway, the idea I suggested earlier is in the patch after my sig.
> >
> > Let's think: LSP's publishDiagnostics coming from the server deals
> > in URIs, right? And we inform the LSP server about URIs, too, right?
> > So the URI is always LSP's idea of the resource identifier (and it likes
> > to have truename URI).
>
> Sure, like in my key/value store. (apart from symlinking)
The other difference is it uses buffer-local variables as the
natural "per-buffer" storage method.
> Could we rather use eglot--managed-buffers, like in my patch? there
> shouldn't be a need to loop through say 200 buffers that are unrelated
> to the project in question, right? Apart from this I agree, and will try
> it.
Of course, that's a good improvement.
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 6:26 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 8:06 ` João Távora
2024-04-19 9:05 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 96+ messages in thread
From: João Távora @ 2024-04-19 8:06 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036, felician.nemeth
On Fri, Apr 19, 2024 at 7:26 AM Theodor Thornhill <theo@thornhill.no> wrote:
> Like this?
>
> I can confirm with yours and this patch the latency is down again, so
> thanks.
I'm very happy it works!
> I'll test a little the difference with/without eglot--managed-buffers.
Yes, do that change, and if you want (_if_ you want) you can clean up
the patch, add some in-code comments explaining the matter, replace
cl-loop with your preferred thing, rename the slightly cryptic
eglot--cached-tdi, maybe set it more explicitly on in eglot--signal-didOpen.
And maybe write a test for the symlink situation, perhaps using my
small experiment, so that it becomes less likely to be broken again
in the future.
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 6:56 ` Eli Zaretskii
2024-04-19 7:51 ` Ihor Radchenko
@ 2024-04-19 8:27 ` João Távora
2024-04-19 8:49 ` João Távora
2024-04-19 11:01 ` Eli Zaretskii
1 sibling, 2 replies; 96+ messages in thread
From: João Távora @ 2024-04-19 8:27 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: felician.nemeth, 70036, theo
On Fri, Apr 19, 2024 at 7:56 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > Maybe _I_ missed something. But I see you have now sent a perfect
> > reproduction, so it doesn't matter.
>
> There was no need to have any profiles to demonstrate that
> file-truename is significantly slower than expand-file-name.
Of course. And there wasn't any need for profiles OR benchmark
to understand that file-truename and expand-file-name do different
things, so comparing one to the other when that difference in behavior
is crucial for the caller is very similar to comparing apple and
oranges.
> It's only now, that we decided symlinks _should_ be resolved by Emacs,
I _think_, but can't be 100% sure, that I explicitly decided that 6 years
ago, I just didn't document it explicitly beyond typing in "file-truename".
Git archeology brings me to a commit in 2018 where I was reorganizing
code, and file-truename was already there. I definitely knew about
expand-file-name in 2018 though.
> > I still think the cleanest solution is to write file-truename
> > in C.
>
> I explained in that past discussion why this is not simple. So if
> simpler solutions exist, we should prefer them.
Fair enough. Shifting complexity around is what we do. But
having a performant file-truename is a strategically investment, it's
a very common filesystem primitive that users expect to be as fast
as it can be made. Common Lisp has TRUENAME, Python has 'realpath()',
etc. We could compare (here benchmarks are definitely the best
method)
> > But if that can't be done, it doesn't seem terribly hard
> > to get rid of find-buffer-visiting in publishDiagnostics and
> > still remain symlink-correct.
>
> find-buffer-visiting was made much faster lately, but that speedup
> AFAIR shows up only if the session has a lot of buffers, so trying
> these benchmarks in "emacs -Q" will not typically show the effect, and
> could even obscure the file-truename effect as well, because the
> number of calls to file-truename will be much smaller.
I'm not sure what test you are suggesting. If f-b-v performs _better_
in "lots of buffers" situation, then we should measure Eglot's performance
in the plausible _worse_ case of few buffers, no? And that's what
Theo proposed. And in the possible but not exactly super-common case
where you have an extreme 15-long directory chain, find-buffer-visiting
was observed to weigh in at 4%, all because of its file-truename call.
> But if calling
> find-buffer-visiting from Eglot can be avoided, that is of course even
> better.
Yes, that's what my latest patch does. But ideally it would be cleaner
(IMHO) to have a fast usable find-buffer-visiting by speeding
up its underlying file-truename.
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 8:27 ` João Távora
@ 2024-04-19 8:49 ` João Távora
2024-04-19 11:12 ` Eli Zaretskii
2024-04-19 11:01 ` Eli Zaretskii
1 sibling, 1 reply; 96+ messages in thread
From: João Távora @ 2024-04-19 8:49 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: felician.nemeth, 70036, theo
On Fri, Apr 19, 2024 at 9:27 AM João Távora <joaotavora@gmail.com> wrote:
> > It's only now, that we decided symlinks _should_ be resolved by Emacs,
>
> I _think_, but can't be 100% sure, that I explicitly decided that 6 years
> ago, I just didn't document it explicitly beyond typing in "file-truename".
> Git archeology brings me to a commit in 2018 where I was reorganizing
> code, and file-truename was already there. I definitely knew about
> expand-file-name in 2018 though.
The fact that I chose find-buffer-visiting over get-file-buffer around
the same time is another hint...
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 8:06 ` João Távora
@ 2024-04-19 9:05 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 0 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-19 9:05 UTC (permalink / raw)
To: João Távora; +Cc: Eli Zaretskii, 70036, felician.nemeth
João Távora <joaotavora@gmail.com> writes:
> On Fri, Apr 19, 2024 at 7:26 AM Theodor Thornhill <theo@thornhill.no> wrote:
>
>> Like this?
>>
>> I can confirm with yours and this patch the latency is down again, so
>> thanks.
>
> I'm very happy it works!
>
>> I'll test a little the difference with/without eglot--managed-buffers.
>
> Yes, do that change, and if you want (_if_ you want) you can clean up
> the patch, add some in-code comments explaining the matter, replace
> cl-loop with your preferred thing, rename the slightly cryptic
> eglot--cached-tdi, maybe set it more explicitly on in eglot--signal-didOpen.
> And maybe write a test for the symlink situation, perhaps using my
> small experiment, so that it becomes less likely to be broken again
> in the future.
>
> João
Sure, will do!
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 8:01 ` João Távora
@ 2024-04-19 9:10 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 9:22 ` João Távora
0 siblings, 1 reply; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-19 9:10 UTC (permalink / raw)
To: João Távora; +Cc: Eli Zaretskii, 70036, felician.nemeth
João Távora <joaotavora@gmail.com> writes:
> On Fri, Apr 19, 2024 at 7:09 AM Theodor Thornhill <theo@thornhill.no> wrote:
>
>> you've also noted earlier. I have an even stronger computer, a 2023 P1
>> gen 5, I believe, running ubuntu. Some servers send _all_ diagnostics on
>> every keystroke.
>
> That could be, but what servers are these?
>
> Thought it would never be "on every keystroke" though. As you probably
> know, Eglot bunches them up in didChange notifications, and you can
> can even be the size of these bunches.
>
Absolutely, it could be. But with these latest changes the felt lag is
gone.
>> > So my perception is that it must have spent around 4% of 1 second in
>> > file-truename.
>> >
>> > Anyway the reason this shows in this profile is because this project
>> > with this particular server sends a lot of publishDiagnostics upfront.
>> > That's OK. Gopls is a very good server. I think I see a fix. But can
>> > you qualitatively describe the Eglot experience. Do you notice any
>> > input lag or something like that? With this project? I didn't feel _any_
>> > lag.
>> > Super snappy. Maybe the JSON serde kicking in?
>> >
>>
>> In this particular project I don't notice any input lag. But on every
>> java project at work I absolutely do.
>
> But that could be down to other reasons Theodor. We would need data
> for that server too. Last time I tried jdtls (on a docker-based TRAMP
> setup, described somewhere in the mailing list), it was snappy.
>
> If you make a simple recipe like this one I can absolutely
> try though. The difficulty in that one was TRAMP.
>
I can try, but these repros are hard, because it is hard to find an open
source java project where I easily can construct similar scenarios. And
also time.
>
>> > Anyway, the idea I suggested earlier is in the patch after my sig.
>> >
>> > Let's think: LSP's publishDiagnostics coming from the server deals
>> > in URIs, right? And we inform the LSP server about URIs, too, right?
>> > So the URI is always LSP's idea of the resource identifier (and it likes
>> > to have truename URI).
>>
>> Sure, like in my key/value store. (apart from symlinking)
>
> The other difference is it uses buffer-local variables as the
> natural "per-buffer" storage method.
>
Yes, sure. Not arguing there. Just noticing that what we're doing here
isn't too different.
>> Could we rather use eglot--managed-buffers, like in my patch? there
>> shouldn't be a need to loop through say 200 buffers that are unrelated
>> to the project in question, right? Apart from this I agree, and will try
>> it.
>
> Of course, that's a good improvement.
>
> João
:thumbsup:
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 7:52 ` João Távora
@ 2024-04-19 9:14 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 0 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-19 9:14 UTC (permalink / raw)
To: João Távora; +Cc: Eli Zaretskii, 70036, felician.nemeth
João Távora <joaotavora@gmail.com> writes:
> On Fri, Apr 19, 2024 at 6:59 AM Theodor Thornhill <theo@thornhill.no> wrote:
>
>> I agree - that's why I want to focus on improving the code, same as
>> you. I just want you to understand that being mindful on how you come
>> across can't really hurt.
>
> You're right of course. I do try to be mindful, but maybe I made a mistake.
> IIUC it was the part where I said this was a "one-off and uncommon" report
> that you felt was unkind and harsh. Because you wrote that. I absolutely
> think we miscommunicated: I wasn't trying to say you have a bizarre project
> setup, I was simply trying to make a statement about statistics and
> the relative importance.
>
No worries. Water under the bridge.
>> There are enough conflicts around these days,
>
> In Emacs or in the world? If the former I'd rather not know about
> them.
>
In the world.
>> let's not make code be one.
>
> Right. So reverting another person's code isn't that big of a deal.
> You "reverted" bits of my code when doing your commits, and that's
> fine.
>
>> Yeah, that's right. I assume that when json serde disappeared as a
>> bottleneck others emerge. This is the obvious one to me right now.
>
> When my hotspot for a given reasonable profile is 4%, I'm feeling
> fairly good about my code. But yes, of course, it can be addressed.
>
Yes, Eglot is pretty fast. Doesn't mean it can't improve, though :)
>> Sure. That will make the change you make similar in spirit to mine, yet
>> with a different implementation. I'm fine with that.
>
> With the difference that my change also still ultimately uses truename,
> just very frugally. So it still understands symlinks, doesn't
> mislead the server and doesn't expose the bug I demonstrated.
Yep, I think we agree and have for some time. I wasn't really arguing
from your first mail, either.
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 21:37 ` João Távora
@ 2024-04-19 9:17 ` Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 0 replies; 96+ messages in thread
From: Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-19 9:17 UTC (permalink / raw)
To: João Távora; +Cc: felician.nemeth, Eli Zaretskii, 70036, theo, dick
João Távora <joaotavora@gmail.com> writes:
>> > Speaking of dicks, you are one, and your code is always dogshit, so your
>> > much heralded leavetaking of the emacs project can't come soon enough.
>>
>> My dear dick, you on the other hand, are such a lovely wonderful human being
>> who writes such great code and is so courageous.. Thank you for
>> gracing me with
>> your attention. Means a lot to me. Love xoxo
>
> And I say this even without having the faintest idea of who you are,
> imagine if I did :-)
Oh, that's the person who is blacklisted on debbugs.gnu.org because of
sending offending messages.
> João
Best regards, Michael.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 9:10 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 9:22 ` João Távora
0 siblings, 0 replies; 96+ messages in thread
From: João Távora @ 2024-04-19 9:22 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036, felician.nemeth
On Fri, Apr 19, 2024 at 10:10 AM Theodor Thornhill <theo@thornhill.no> wrote:
>
> João Távora <joaotavora@gmail.com> writes:
>
> > On Fri, Apr 19, 2024 at 7:09 AM Theodor Thornhill <theo@thornhill.no> wrote:
> > But that could be down to other reasons Theodor. We would need data
> > for that server too. Last time I tried jdtls (on a docker-based TRAMP
> > setup, described somewhere in the mailing list), it was snappy.
> >
> > If you make a simple recipe like this one I can absolutely
> > try though. The difficulty in that one was TRAMP.
> >
>
> I can try, but these repros are hard, because it is hard to find an open
> source java project where I easily can construct similar scenarios. And
> also time.
Yes, it's hard, indeed. But it's very valuable. IOW, we shouldn't make
design decisions without gathering hard data and reproducible experiments,
like we did here. There are lots and lots of litte factors so at least
two people reproducing the same results in a plausible agreed-upon scenario
(again, like we did here), is important.
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 7:51 ` Ihor Radchenko
@ 2024-04-19 10:51 ` Eli Zaretskii
2024-04-30 11:30 ` Ihor Radchenko
0 siblings, 1 reply; 96+ messages in thread
From: Eli Zaretskii @ 2024-04-19 10:51 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: theo, 70036, felician.nemeth, joaotavora
> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: João Távora <joaotavora@gmail.com>,
> felician.nemeth@gmail.com,
> 70036@debbugs.gnu.org, theo@thornhill.no
> Date: Fri, 19 Apr 2024 07:51:35 +0000
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> I still think the cleanest solution is to write file-truename
> >> in C.
> >
> > I explained in that past discussion why this is not simple. So if
> > simpler solutions exist, we should prefer them.
>
> There are some easy things that can be done to improve `file-truename'
> performance somewhat. For example, the number of calls to
> `file-name-nondirectory' can be trivially reduced in the `file-truename'
> code - it is called up to three times in a row.
>
> (see my earlier message in https://yhetil.org/emacs-bugs/87jzlmd831.fsf@localhost/)
>
> Will it be of interest?
Yes, of course. Those kinds of changes are no-brainers, really.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 8:27 ` João Távora
2024-04-19 8:49 ` João Távora
@ 2024-04-19 11:01 ` Eli Zaretskii
2024-04-19 11:32 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 12:00 ` João Távora
1 sibling, 2 replies; 96+ messages in thread
From: Eli Zaretskii @ 2024-04-19 11:01 UTC (permalink / raw)
To: João Távora; +Cc: felician.nemeth, 70036, theo
> From: João Távora <joaotavora@gmail.com>
> Date: Fri, 19 Apr 2024 09:27:31 +0100
> Cc: theo@thornhill.no, felician.nemeth@gmail.com, 70036@debbugs.gnu.org
>
> > > I still think the cleanest solution is to write file-truename
> > > in C.
> >
> > I explained in that past discussion why this is not simple. So if
> > simpler solutions exist, we should prefer them.
>
> Fair enough. Shifting complexity around is what we do. But
> having a performant file-truename is a strategically investment, it's
> a very common filesystem primitive that users expect to be as fast
> as it can be made. Common Lisp has TRUENAME, Python has 'realpath()',
> etc. We could compare (here benchmarks are definitely the best
> method)
file-truename does much more than just the equivalent of realpath. If
all we needed was a single call to realpath, we'd done that in C long
ago.
The problem with rewriting file-truename in C is that we must be 110%
compatible to what the Lisp implementation does, since it's such a
low-level API that is used so widely.
> > find-buffer-visiting was made much faster lately, but that speedup
> > AFAIR shows up only if the session has a lot of buffers, so trying
> > these benchmarks in "emacs -Q" will not typically show the effect, and
> > could even obscure the file-truename effect as well, because the
> > number of calls to file-truename will be much smaller.
>
> I'm not sure what test you are suggesting. If f-b-v performs _better_
> in "lots of buffers" situation, then we should measure Eglot's performance
> in the plausible _worse_ case of few buffers, no?
No, IMO we should try it in both "emacs -Q" and in a session with a
lot of buffers, to have the performance in its true relevant context.
Most real-life Emacs sessions have many more buffers than we have in
"emacs -Q". Worst-case testing is not always TRT, because it can skew
the perspective and lead to wrong decisions.
> > But if calling
> > find-buffer-visiting from Eglot can be avoided, that is of course even
> > better.
>
> Yes, that's what my latest patch does. But ideally it would be cleaner
> (IMHO) to have a fast usable find-buffer-visiting by speeding
> up its underlying file-truename.
We did that at least to some extent in the improvements submitted by
Ihor and now available on master. From where I stand, we now have a
reasonably performant implementation of find-buffer-visiting; I would
need benchmarks showing otherwise to change my mind.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 8:49 ` João Távora
@ 2024-04-19 11:12 ` Eli Zaretskii
2024-04-19 11:34 ` João Távora
0 siblings, 1 reply; 96+ messages in thread
From: Eli Zaretskii @ 2024-04-19 11:12 UTC (permalink / raw)
To: João Távora; +Cc: felician.nemeth, 70036, theo
> From: João Távora <joaotavora@gmail.com>
> Date: Fri, 19 Apr 2024 09:49:38 +0100
> Cc: theo@thornhill.no, felician.nemeth@gmail.com, 70036@debbugs.gnu.org
>
> On Fri, Apr 19, 2024 at 9:27 AM João Távora <joaotavora@gmail.com> wrote:
>
> > > It's only now, that we decided symlinks _should_ be resolved by Emacs,
> >
> > I _think_, but can't be 100% sure, that I explicitly decided that 6 years
> > ago, I just didn't document it explicitly beyond typing in "file-truename".
> > Git archeology brings me to a commit in 2018 where I was reorganizing
> > code, and file-truename was already there. I definitely knew about
> > expand-file-name in 2018 though.
>
> The fact that I chose find-buffer-visiting over get-file-buffer around
> the same time is another hint...
Without proper commentary, this is a very weak hint. It could, for
example, mean the problem you wanted to solve no longer exists, or
even that you were unsure about whether expand-file-name could do the
job.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 11:01 ` Eli Zaretskii
@ 2024-04-19 11:32 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 11:40 ` João Távora
2024-04-19 11:53 ` Eli Zaretskii
2024-04-19 12:00 ` João Távora
1 sibling, 2 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-19 11:32 UTC (permalink / raw)
To: Eli Zaretskii, João Távora; +Cc: 70036, felician.nemeth
Eli Zaretskii <eliz@gnu.org> writes:
>> From: João Távora <joaotavora@gmail.com>
>> Date: Fri, 19 Apr 2024 09:27:31 +0100
>> Cc: theo@thornhill.no, felician.nemeth@gmail.com, 70036@debbugs.gnu.org
>>
>> > > I still think the cleanest solution is to write file-truename
>> > > in C.
>> >
>> > I explained in that past discussion why this is not simple. So if
>> > simpler solutions exist, we should prefer them.
>>
>> Fair enough. Shifting complexity around is what we do. But
>> having a performant file-truename is a strategically investment, it's
>> a very common filesystem primitive that users expect to be as fast
>> as it can be made. Common Lisp has TRUENAME, Python has 'realpath()',
>> etc. We could compare (here benchmarks are definitely the best
>> method)
>
> file-truename does much more than just the equivalent of realpath. If
> all we needed was a single call to realpath, we'd done that in C long
> ago.
>
> The problem with rewriting file-truename in C is that we must be 110%
> compatible to what the Lisp implementation does, since it's such a
> low-level API that is used so widely.
>
>> > find-buffer-visiting was made much faster lately, but that speedup
>> > AFAIR shows up only if the session has a lot of buffers, so trying
>> > these benchmarks in "emacs -Q" will not typically show the effect, and
>> > could even obscure the file-truename effect as well, because the
>> > number of calls to file-truename will be much smaller.
>>
>> I'm not sure what test you are suggesting. If f-b-v performs _better_
>> in "lots of buffers" situation, then we should measure Eglot's performance
>> in the plausible _worse_ case of few buffers, no?
>
> No, IMO we should try it in both "emacs -Q" and in a session with a
> lot of buffers, to have the performance in its true relevant context.
> Most real-life Emacs sessions have many more buffers than we have in
> "emacs -Q". Worst-case testing is not always TRT, because it can skew
> the perspective and lead to wrong decisions.
>
>> > But if calling
>> > find-buffer-visiting from Eglot can be avoided, that is of course even
>> > better.
>>
>> Yes, that's what my latest patch does. But ideally it would be cleaner
>> (IMHO) to have a fast usable find-buffer-visiting by speeding
>> up its underlying file-truename.
>
I wonder - why can't we use just buffer-file-truename?
In the below function, wouldn't buffer-file-truename let us avoid the
file-truename in eglot-path-to-uri?
(defun eglot--TextDocumentIdentifier ()
"Compute TextDocumentIdentifier object for current buffer."
`(:uri ,(or eglot--cached-tdi
(setq eglot--cached-tdi
(eglot-path-to-uri (or buffer-file-name
(ignore-errors
(buffer-file-name
(buffer-base-buffer)))))))))
> We did that at least to some extent in the improvements submitted by
> Ihor and now available on master. From where I stand, we now have a
> reasonably performant implementation of find-buffer-visiting; I would
> need benchmarks showing otherwise to change my mind.
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 11:12 ` Eli Zaretskii
@ 2024-04-19 11:34 ` João Távora
2024-04-19 18:13 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 96+ messages in thread
From: João Távora @ 2024-04-19 11:34 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: felician.nemeth, 70036, theo
On Fri, Apr 19, 2024 at 12:12 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > > I _think_, but can't be 100% sure, that I explicitly decided that 6 years
> > > ago, I just didn't document it explicitly beyond typing in "file-truename".
> > > Git archeology brings me to a commit in 2018 where I was reorganizing
> > > code, and file-truename was already there. I definitely knew about
> > > expand-file-name in 2018 though.
> >
> > The fact that I chose find-buffer-visiting over get-file-buffer around
> > the same time is another hint...
>
> Without proper commentary, this is a very weak hint. It could, for
> example, mean the problem you wanted to solve no longer exists, or
> even that you were unsure about whether expand-file-name could do the
> job.
If I indeed _meant_ to solve the symlink problem, then it most definitely
still exists. I've demonstrated that.
Intentional or not, the fact of the matter is that Eglot was usable
from very early on projects with symlinks. Not all clients can boast
that, as seems to be evidenced some bug reports to the servers pertaining
to some other clients.
Regardless, foregoing this behaviour should be at a minimum documented
and would need an extremely well-fundamented reason. But there was not
even sufficient awareness that the behaviour was being abandoned. Which
is, like, totally OK and common and normal, and doesn't bring into
question the high credentials of anyone involved in this discussion.
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 11:32 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 11:40 ` João Távora
2024-04-19 11:47 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 20:23 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 11:53 ` Eli Zaretskii
1 sibling, 2 replies; 96+ messages in thread
From: João Távora @ 2024-04-19 11:40 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036, felician.nemeth
On Fri, Apr 19, 2024 at 12:32 PM Theodor Thornhill <theo@thornhill.no> wrote:
> I wonder - why can't we use just buffer-file-truename?
>
> In the below function, wouldn't buffer-file-truename let us avoid the
> file-truename in eglot-path-to-uri?
Never used, but try it out. It seems it's already a built-in cache
of whatever the buffer's truename is (and the docstring says
it does call the slow `file-truename`). But like in my patch, it's
a one-time thing. So yeah, if it's available in old Emacsen as well,
it could replace `eglot--cached-tdi`, but then the search in the
publishDiagnostics
handler would have to be adjusted accordingly to call uri-to-path again.
Try it out, may be cleaner indeed.
Beware though, Stefan has fixed the tests which is very good, but there
still isn't a test for the symlink scenario, so you'd have to test it
manually (or
even better, craft that test yourself, with clangd).
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 11:40 ` João Távora
@ 2024-04-19 11:47 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 11:51 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 11:51 ` João Távora
2024-04-19 20:23 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 2 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-19 11:47 UTC (permalink / raw)
To: João Távora; +Cc: Eli Zaretskii, 70036, felician.nemeth
João Távora <joaotavora@gmail.com> writes:
> On Fri, Apr 19, 2024 at 12:32 PM Theodor Thornhill <theo@thornhill.no> wrote:
>
>> I wonder - why can't we use just buffer-file-truename?
>>
>> In the below function, wouldn't buffer-file-truename let us avoid the
>> file-truename in eglot-path-to-uri?
>
> Never used, but try it out. It seems it's already a built-in cache
> of whatever the buffer's truename is (and the docstring says
> it does call the slow `file-truename`). But like in my patch, it's
> a one-time thing. So yeah, if it's available in old Emacsen as well,
> it could replace `eglot--cached-tdi`, but then the search in the
> publishDiagnostics
> handler would have to be adjusted accordingly to call uri-to-path again.
What would you consider as a sufficiently old Emacs?
>
> Try it out, may be cleaner indeed.
>
> Beware though, Stefan has fixed the tests which is very good, but there
> still isn't a test for the symlink scenario, so you'd have to test it
> manually (or
> even better, craft that test yourself, with clangd).
>
> João
Sure
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 11:47 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 11:51 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 12:01 ` João Távora
2024-04-19 11:51 ` João Távora
1 sibling, 1 reply; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-19 11:51 UTC (permalink / raw)
To: João Távora; +Cc: Eli Zaretskii, 70036, felician.nemeth
Theodor Thornhill <theo@thornhill.no> writes:
> João Távora <joaotavora@gmail.com> writes:
>
>> On Fri, Apr 19, 2024 at 12:32 PM Theodor Thornhill <theo@thornhill.no> wrote:
>>
>>> I wonder - why can't we use just buffer-file-truename?
>>>
>>> In the below function, wouldn't buffer-file-truename let us avoid the
>>> file-truename in eglot-path-to-uri?
>>
>> Never used, but try it out. It seems it's already a built-in cache
>> of whatever the buffer's truename is (and the docstring says
>> it does call the slow `file-truename`). But like in my patch, it's
>> a one-time thing. So yeah, if it's available in old Emacsen as well,
>> it could replace `eglot--cached-tdi`, but then the search in the
>> publishDiagnostics
>> handler would have to be adjusted accordingly to call uri-to-path again.
>
> What would you consider as a sufficiently old Emacs?
>
I guess this should be enough:
commit f6ed2e848d23035748b621a86be74956c093823f
Author: Richard M. Stallman <rms@gnu.org>
Date: Tue Feb 14 16:33:19 1995 +0000
(syms_of_buffer): Set up Lisp var buffer-file-truename.
(init_buffer_once): Set up flag and default value for file_truename.
(reset_buffer): Init file_truename slot.
diff --git a/src/buffer.c b/src/buffer.c
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -3011,0 +3014,5 @@
+ DEFVAR_PER_BUFFER ("buffer-file-truename", ¤t_buffer->file_truename,
+ make_number (Lisp_String),
+ "Truename of file visited in current buffer, or nil if not visiting a file.\n\
+The truename of a file is calculated by `file-truename'.\n\
+Each buffer has its own value of this variable.");
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 11:47 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 11:51 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 11:51 ` João Távora
1 sibling, 0 replies; 96+ messages in thread
From: João Távora @ 2024-04-19 11:51 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036, felician.nemeth
On Fri, Apr 19, 2024 at 12:47 PM Theodor Thornhill <theo@thornhill.no> wrote:
> What would you consider as a sufficiently old Emacs?
The first version supported by Eglot, the GNU ELPA Package. I think it's 26.3
if I'm not mistaken.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 11:32 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 11:40 ` João Távora
@ 2024-04-19 11:53 ` Eli Zaretskii
2024-04-19 11:59 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 12:03 ` João Távora
1 sibling, 2 replies; 96+ messages in thread
From: Eli Zaretskii @ 2024-04-19 11:53 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: 70036, felician.nemeth, joaotavora
> From: Theodor Thornhill <theo@thornhill.no>
> Cc: felician.nemeth@gmail.com, 70036@debbugs.gnu.org
> Date: Fri, 19 Apr 2024 13:32:42 +0200
>
> I wonder - why can't we use just buffer-file-truename?
buffer-file-truename records the _abbreviated_ file name, so it can
depend on directory-abbrev-alist. Whether this is important here, I
don't know, but we must at least consider this caveat before we
decide.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 11:53 ` Eli Zaretskii
@ 2024-04-19 11:59 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 12:03 ` João Távora
1 sibling, 0 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-19 11:59 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 70036, felician.nemeth, joaotavora
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Theodor Thornhill <theo@thornhill.no>
>> Cc: felician.nemeth@gmail.com, 70036@debbugs.gnu.org
>> Date: Fri, 19 Apr 2024 13:32:42 +0200
>>
>> I wonder - why can't we use just buffer-file-truename?
>
> buffer-file-truename records the _abbreviated_ file name, so it can
> depend on directory-abbrev-alist. Whether this is important here, I
> don't know, but we must at least consider this caveat before we
> decide.
Yeah, it also seems some of functions in question don't need 'path' to
be an existing buffer. I'll look at it, but I think it may be a no-go.
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 11:01 ` Eli Zaretskii
2024-04-19 11:32 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 12:00 ` João Távora
2024-04-19 12:13 ` Eli Zaretskii
1 sibling, 1 reply; 96+ messages in thread
From: João Távora @ 2024-04-19 12:00 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: felician.nemeth, 70036, theo
On Fri, Apr 19, 2024 at 12:01 PM Eli Zaretskii <eliz@gnu.org> wrote:
> No, IMO we should try it in both "emacs -Q" and in a session with a
> lot of buffers, to have the performance in its true relevant context.
> Most real-life Emacs sessions have many more buffers than we have in
> "emacs -Q". Worst-case testing is not always TRT, because it can skew
> the perspective and lead to wrong decisions.
The worst case here is relative. It's also a very plausible case
in the sense that starting Emacs, navigating to a file of a project
you're working on, and pressing M-x eglot is probably one of the most
common things you do with Eglot. So IOW the worst case is also the
common one.
> > > But if calling
> > > find-buffer-visiting from Eglot can be avoided, that is of course even
> > > better.
> >
> > Yes, that's what my latest patch does. But ideally it would be cleaner
> > (IMHO) to have a fast usable find-buffer-visiting by speeding
> > up its underlying file-truename.
>
> We did that at least to some extent in the improvements submitted by
> Ihor and now available on master. From where I stand, we now have a
> reasonably performant implementation of find-buffer-visiting; I would
> need benchmarks showing otherwise to change my mind.
Theo's latest measurements show that -- in context -- there is a 4%
time spent in find-buffer-visiting and all of that is due to its
delegation of work to file-truename. Does that change your mind?
I reproduced the measurements but personally didn't feel this in any way
problematic (in my machine) to the use of Eglot. But Theo says there
are Java projects that run much, much worse and with much more friction,
but he can't share the data with us for other reasons. So it didn't
really change _my_ mind. For _me_ `f-b-v` was reasonably fast enough
when running Theo's recipe and definitely fast enough when trying
any other project.
But what changes my mind is the fact that:
* I believe Theo is telling the truth that it's a much bigger bottleneck
in Java projects.
* the workaround isn't _that_ ugly.
In summary, I still think f-b-v use in Eglot's publishDiagnostics handler is
cleanest conceptually, but because I believe Theo that it's a serious
bottleneck, I'm not opposed to skipping it with the alternative technique
already presented.
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 11:51 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 12:01 ` João Távora
0 siblings, 0 replies; 96+ messages in thread
From: João Távora @ 2024-04-19 12:01 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036, felician.nemeth
On Fri, Apr 19, 2024 at 12:51 PM Theodor Thornhill <theo@thornhill.no> wrote:
> I guess this should be enough:
>
> commit f6ed2e848d23035748b621a86be74956c093823f
> Author: Richard M. Stallman <rms@gnu.org>
> Date: Tue Feb 14 16:33:19 1995 +0000
:-)
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 11:53 ` Eli Zaretskii
2024-04-19 11:59 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 12:03 ` João Távora
1 sibling, 0 replies; 96+ messages in thread
From: João Távora @ 2024-04-19 12:03 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: felician.nemeth, 70036, Theodor Thornhill
On Fri, Apr 19, 2024 at 12:54 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Theodor Thornhill <theo@thornhill.no>
> > Cc: felician.nemeth@gmail.com, 70036@debbugs.gnu.org
> > Date: Fri, 19 Apr 2024 13:32:42 +0200
> >
> > I wonder - why can't we use just buffer-file-truename?
>
> buffer-file-truename records the _abbreviated_ file name, so it can
> depend on directory-abbrev-alist. Whether this is important here, I
> don't know, but we must at least consider this caveat before we
> decide.
We definitely need a full, true, unabbreviated file name there.
That's what LSP servers like.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 12:00 ` João Távora
@ 2024-04-19 12:13 ` Eli Zaretskii
2024-04-19 12:20 ` João Távora
0 siblings, 1 reply; 96+ messages in thread
From: Eli Zaretskii @ 2024-04-19 12:13 UTC (permalink / raw)
To: João Távora; +Cc: felician.nemeth, 70036, theo
> From: João Távora <joaotavora@gmail.com>
> Date: Fri, 19 Apr 2024 13:00:54 +0100
> Cc: theo@thornhill.no, felician.nemeth@gmail.com, 70036@debbugs.gnu.org
>
> On Fri, Apr 19, 2024 at 12:01 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > > Yes, that's what my latest patch does. But ideally it would be cleaner
> > > (IMHO) to have a fast usable find-buffer-visiting by speeding
> > > up its underlying file-truename.
> >
> > We did that at least to some extent in the improvements submitted by
> > Ihor and now available on master. From where I stand, we now have a
> > reasonably performant implementation of find-buffer-visiting; I would
> > need benchmarks showing otherwise to change my mind.
>
> Theo's latest measurements show that -- in context -- there is a 4%
> time spent in find-buffer-visiting and all of that is due to its
> delegation of work to file-truename. Does that change your mind?
No. The original slow-down due to find-buffer-visiting (admittedly,
not in Eglot) was much worse.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 12:13 ` Eli Zaretskii
@ 2024-04-19 12:20 ` João Távora
0 siblings, 0 replies; 96+ messages in thread
From: João Távora @ 2024-04-19 12:20 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: felician.nemeth, 70036, theo
On Fri, Apr 19, 2024 at 1:13 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > Theo's latest measurements show that -- in context -- there is a 4%
> > time spent in find-buffer-visiting and all of that is due to its
> > delegation of work to file-truename. Does that change your mind?
>
> No. The original slow-down due to find-buffer-visiting (admittedly,
> not in Eglot) was much worse.
I understand, wouldn't change my mind, either. But Theo says it's much
worse in some other cases, so I don't mind installing a that f-b-v
bypass (with a sufficiently explicit comment about a desire to
return to it if file-truename is ever sped up).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 6:45 ` Eli Zaretskii
2024-04-19 7:38 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 12:54 ` João Távora
2024-04-19 14:32 ` Eli Zaretskii
1 sibling, 1 reply; 96+ messages in thread
From: João Távora @ 2024-04-19 12:54 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: felician.nemeth, 70036, Theodor Thornhill
On Fri, Apr 19, 2024 at 7:45 AM Eli Zaretskii <eliz@gnu.org> wrote:
> Thanks, but that is not what I asked to provide, for us to make a
> decision in this case.
> Could you please show benchmark times of the old code (before your
> changes), the code after your changes, and the current code in Git
I just noticed this and think a comment needs to be made about Eglot
and performance measurements specifically.
If a good `benchmark-run` or invocation can be made to measure a specific
problem, it is usually a very good tool. But in Eglot, it's rare (*)
What some in this discussion may not understand is that Eglot works mostly
asynchronously. The user invokes commands that ask something of the
server and that server will later respond with something that is hopefully
still relevant somehow. On-the-fly diagnostics work like this, and even
startup can be asynchronous. So any slowdowns are usually felt subjectively
in input lag across a session where the user is doing some work.
More often than not, there isn't a
(benchamark-run 10 (eglot-foo <some-spoofed-arguiments>))
that can tell us something useful. It's more like "I feel diagnostics take
longer to appear", "ElDoc spins my CPU fan", "completions feel laggy", and
things like that.
Of course, special Elisp code can be crafted to simulate how a user
interacts with Eglot, but that's not always easy due to the need to
mock complex objects (the server object, notably) and the aforementioned
async nature.
So that's why the best way to communicate these problems is via a profile
accompanied by the usual bullet-proof Emacs -Q recipe. It should contain a
clear description of when profiler-start and profiler-stop are invoked, and
what the user did in between. And that's _exactly_ what Theo did here,
and was useful to me: I reproduced his results perfectly. Those profiles
easily tell us who the hotspots are at the Elisp level.
That's IF the hotspots are even at the Elisp level, because often a
slow-responding server is all it takes. In those cases Eglot/Elisp may
or may not be "off the hook", depending on whether the cause for server
slowness is some earlier misconfiguration that is ultimately Eglot's
fault.
Maybe some of this info should be added to the Eglot manual in its
troubleshooting section.
João
(*) The eglot--glob section is a notable exception. I used
benchmark-run to optimize back then.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 12:54 ` João Távora
@ 2024-04-19 14:32 ` Eli Zaretskii
0 siblings, 0 replies; 96+ messages in thread
From: Eli Zaretskii @ 2024-04-19 14:32 UTC (permalink / raw)
To: João Távora; +Cc: felician.nemeth, 70036, theo
> From: João Távora <joaotavora@gmail.com>
> Date: Fri, 19 Apr 2024 13:54:23 +0100
> Cc: Theodor Thornhill <theo@thornhill.no>, felician.nemeth@gmail.com, 70036@debbugs.gnu.org
>
> On Fri, Apr 19, 2024 at 7:45 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > Thanks, but that is not what I asked to provide, for us to make a
> > decision in this case.
> > Could you please show benchmark times of the old code (before your
> > changes), the code after your changes, and the current code in Git
>
> I just noticed this and think a comment needs to be made about Eglot
> and performance measurements specifically.
>
> If a good `benchmark-run` or invocation can be made to measure a specific
> problem, it is usually a very good tool. But in Eglot, it's rare (*)
>
> What some in this discussion may not understand is that Eglot works mostly
> asynchronously. The user invokes commands that ask something of the
> server and that server will later respond with something that is hopefully
> still relevant somehow. On-the-fly diagnostics work like this, and even
> startup can be asynchronous. So any slowdowns are usually felt subjectively
> in input lag across a session where the user is doing some work.
>
> More often than not, there isn't a
>
> (benchamark-run 10 (eglot-foo <some-spoofed-arguiments>))
>
> that can tell us something useful. It's more like "I feel diagnostics take
> longer to appear", "ElDoc spins my CPU fan", "completions feel laggy", and
> things like that.
benchamark-run can be unusable in this case, but primitives that
measure time are still useful, even if one needs to insert some
special calls into the code that measure time.
> So that's why the best way to communicate these problems is via a profile
> accompanied by the usual bullet-proof Emacs -Q recipe. It should contain a
> clear description of when profiler-start and profiler-stop are invoked, and
> what the user did in between. And that's _exactly_ what Theo did here,
> and was useful to me: I reproduced his results perfectly. Those profiles
> easily tell us who the hotspots are at the Elisp level.
The profiler is not very useful in such cases because it doesn't
provide a clear picture of the time it took some code to execute.
Also because typically there's a large portion of it that points to
irrelevant functions line execute-extended-command etc.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 11:34 ` João Távora
@ 2024-04-19 18:13 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 18:59 ` João Távora
0 siblings, 1 reply; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-19 18:13 UTC (permalink / raw)
To: João Távora, Eli Zaretskii; +Cc: 70036, felician.nemeth
João Távora <joaotavora@gmail.com> writes:
> On Fri, Apr 19, 2024 at 12:12 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
>> > > I _think_, but can't be 100% sure, that I explicitly decided that 6 years
>> > > ago, I just didn't document it explicitly beyond typing in "file-truename".
>> > > Git archeology brings me to a commit in 2018 where I was reorganizing
>> > > code, and file-truename was already there. I definitely knew about
>> > > expand-file-name in 2018 though.
>> >
>> > The fact that I chose find-buffer-visiting over get-file-buffer around
>> > the same time is another hint...
>>
>> Without proper commentary, this is a very weak hint. It could, for
>> example, mean the problem you wanted to solve no longer exists, or
>> even that you were unsure about whether expand-file-name could do the
>> job.
>
> If I indeed _meant_ to solve the symlink problem, then it most definitely
> still exists. I've demonstrated that.
>
> Intentional or not, the fact of the matter is that Eglot was usable
> from very early on projects with symlinks. Not all clients can boast
> that, as seems to be evidenced some bug reports to the servers pertaining
> to some other clients.
FWIW this isn't true AFAICT.
Try this:
create foo.go:
```
package main
func foo () int {
return 1
}
```
Then main.go
```
package main
func main () {
foo()
}
```
now `ln -s main.go mainlink.go`
Open main.go in emacs and M-x eglot. Observe error:
```
compiler [DuplicateDecl]: main redeclared in this block (this error: other declaration of main)
```
This is from the revert commit. Same behavior from pre-my-fix, my-fix,
your first fix and the latest iteration. For now I'm guessing on the
register file watcher init for gopls not caring about symlinks.
>
> Regardless, foregoing this behaviour should be at a minimum documented
> and would need an extremely well-fundamented reason. But there was not
> even sufficient awareness that the behaviour was being abandoned. Which
> is, like, totally OK and common and normal, and doesn't bring into
> question the high credentials of anyone involved in this discussion.
>
> João
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 18:13 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 18:59 ` João Távora
2024-04-19 19:42 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 96+ messages in thread
From: João Távora @ 2024-04-19 18:59 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036, felician.nemeth
On Fri, Apr 19, 2024 at 7:14 PM Theodor Thornhill <theo@thornhill.no> wrote:
>
> João Távora <joaotavora@gmail.com> writes:
>
> Open main.go in emacs and M-x eglot. Observe error:
> ```
> compiler [DuplicateDecl]: main redeclared in this block (this error: other declaration of main)
> ```
Reproduced, but that's not Eglot's fault, couldn't be. Eglot told
gopls _nothing_ about mainlink.go. Check your events buffer! No
didOpen for mainlink.go! In fact you probably don't even need to didOpen
any file for that problem to be reported as a project-wide diagnostic.
A simple initialize will do, I bet. IOW, you barely need a client.
Gopls decided to do some out-of-band (meaning out-of-LSP) analysis
of the code, bumped into its own ignorance (or policy, who knows?)
re. symlinks, and decided to issue diagnostics. Seems something a
Go compiler would do. Maybe it does? Are symlinks even supported in
Go projects?
Bottom line is Eglot wasn't ever the hindrance for this to work and it
should not suddenly become one. Not without a very good reason, which
this performance matter clearly isn't (not least because there are lots
of alternative solutions as we've seen)
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 18:59 ` João Távora
@ 2024-04-19 19:42 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 0 replies; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-19 19:42 UTC (permalink / raw)
To: João Távora; +Cc: Eli Zaretskii, 70036, felician.nemeth
João Távora <joaotavora@gmail.com> writes:
> On Fri, Apr 19, 2024 at 7:14 PM Theodor Thornhill <theo@thornhill.no> wrote:
>>
>> João Távora <joaotavora@gmail.com> writes:
>>
>> Open main.go in emacs and M-x eglot. Observe error:
>> ```
>> compiler [DuplicateDecl]: main redeclared in this block (this error: other declaration of main)
>> ```
>
> Reproduced, but that's not Eglot's fault, couldn't be. Eglot told
> gopls _nothing_ about mainlink.go. Check your events buffer! No
> didOpen for mainlink.go! In fact you probably don't even need to didOpen
> any file for that problem to be reported as a project-wide diagnostic.
> A simple initialize will do, I bet. IOW, you barely need a client.
> Gopls decided to do some out-of-band (meaning out-of-LSP) analysis
> of the code, bumped into its own ignorance (or policy, who knows?)
> re. symlinks, and decided to issue diagnostics. Seems something a
> Go compiler would do. Maybe it does? Are symlinks even supported in
> Go projects?
>
> Bottom line is Eglot wasn't ever the hindrance for this to work and it
> should not suddenly become one. Not without a very good reason, which
> this performance matter clearly isn't (not least because there are lots
> of alternative solutions as we've seen)
>
Sure. I think it locates this file from the globbing in file
watchers. Because it matches a pattern it is allowed to read it. But
it's not important, mostly an example of how symlinks and lsps are
difficult.
As another point of reference, lsp-mode behaves the same as Eglot.
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 11:40 ` João Távora
2024-04-19 11:47 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 20:23 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 21:32 ` João Távora
1 sibling, 1 reply; 96+ messages in thread
From: Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-19 20:23 UTC (permalink / raw)
To: João Távora; +Cc: Eli Zaretskii, 70036, felician.nemeth
João Távora <joaotavora@gmail.com> writes:
> On Fri, Apr 19, 2024 at 12:32 PM Theodor Thornhill <theo@thornhill.no> wrote:
>
>> I wonder - why can't we use just buffer-file-truename?
>>
>> In the below function, wouldn't buffer-file-truename let us avoid the
>> file-truename in eglot-path-to-uri?
>
> Never used, but try it out. It seems it's already a built-in cache
> of whatever the buffer's truename is (and the docstring says
> it does call the slow `file-truename`). But like in my patch, it's
> a one-time thing. So yeah, if it's available in old Emacsen as well,
> it could replace `eglot--cached-tdi`, but then the search in the
> publishDiagnostics
> handler would have to be adjusted accordingly to call uri-to-path again.
>
> Try it out, may be cleaner indeed.
>
> Beware though, Stefan has fixed the tests which is very good, but there
> still isn't a test for the symlink scenario, so you'd have to test it
> manually (or
> even better, craft that test yourself, with clangd).
>
> João
Please check out 49ef173b0287e17273e4476df16dca5f2196b11c. I just pushed
the new version, with a test for the symlink scenario as a regression
test.
Thanks,
Theo
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 20:23 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-19 21:32 ` João Távora
0 siblings, 0 replies; 96+ messages in thread
From: João Távora @ 2024-04-19 21:32 UTC (permalink / raw)
To: Theodor Thornhill; +Cc: Eli Zaretskii, 70036, felician.nemeth
On Fri, Apr 19, 2024 at 9:23 PM Theodor Thornhill <theo@thornhill.no> wrote:
> Please check out 49ef173b0287e17273e4476df16dca5f2196b11c. I just pushed
> the new version, with a test for the symlink scenario as a regression
> test.
Looks great. I pushed some very minor tweaks (naming and comments)
João
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-18 20:21 ` João Távora
` (2 preceding siblings ...)
2024-04-19 0:57 ` Yuan Fu
@ 2024-04-22 22:11 ` Dmitry Gutov
3 siblings, 0 replies; 96+ messages in thread
From: Dmitry Gutov @ 2024-04-22 22:11 UTC (permalink / raw)
To: João Távora, Eli Zaretskii; +Cc: theo, 70036, felician.nemeth
Hi Joao,
On 18/04/2024 23:21, João Távora wrote:
> * I had to take a long timeout from Eglot maintenance for personal
> reasons managed to get maintainers for Flymake and Jsonrpc, but I am
> still Eglot maintainer.
Would it make sense to update the "Maintainer:" headers for flymake and
jsonrpc?
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-19 10:51 ` Eli Zaretskii
@ 2024-04-30 11:30 ` Ihor Radchenko
2024-05-02 9:40 ` Eli Zaretskii
0 siblings, 1 reply; 96+ messages in thread
From: Ihor Radchenko @ 2024-04-30 11:30 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: theo, 70036, felician.nemeth, joaotavora
[-- Attachment #1: Type: text/plain, Size: 517 bytes --]
Eli Zaretskii <eliz@gnu.org> writes:
>> There are some easy things that can be done to improve `file-truename'
>> performance somewhat. For example, the number of calls to
>> `file-name-nondirectory' can be trivially reduced in the `file-truename'
>> code - it is called up to three times in a row.
>>
>> (see my earlier message in https://yhetil.org/emacs-bugs/87jzlmd831.fsf@localhost/)
>>
>> Will it be of interest?
>
> Yes, of course. Those kinds of changes are no-brainers, really.
See the attached patch.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-performance-of-file-truename-bug-70036.patch --]
[-- Type: text/x-patch, Size: 2954 bytes --]
From cde58b309588008707cc8b00919eb24801e42eb6 Mon Sep 17 00:00:00 2001
Message-ID: <cde58b309588008707cc8b00919eb24801e42eb6.1714476584.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Tue, 30 Apr 2024 14:27:04 +0300
Subject: [PATCH] Improve performance of `file-truename' (bug#70036)
* lisp/files.el (file-truename): Avoid repetitive calls to
`file-name-nondirectory'. These calls contribute significantly to CPU
time. See the benchmarks in
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=70036#47
---
lisp/files.el | 43 ++++++++++++++++++++++---------------------
1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/lisp/files.el b/lisp/files.el
index 7dec67c5cf0..b7ebb727c72 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1504,27 +1504,28 @@ file-truename
(new (file-name-as-directory (file-truename dirfile counter prev-dirs))))
(setcar prev-dirs (cons (cons old new) (car prev-dirs)))
(setq dir new))))
- (if (equal ".." (file-name-nondirectory filename))
- (setq filename
- (directory-file-name (file-name-directory (directory-file-name dir)))
- done t)
- (if (equal "." (file-name-nondirectory filename))
- (setq filename (directory-file-name dir)
- done t)
- ;; Put it back on the file name.
- (setq filename (concat dir (file-name-nondirectory filename)))
- ;; Is the file name the name of a link?
- (setq target (file-symlink-p filename))
- (if target
- ;; Yes => chase that link, then start all over
- ;; since the link may point to a directory name that uses links.
- ;; We can't safely use expand-file-name here
- ;; since target might look like foo/../bar where foo
- ;; is itself a link. Instead, we handle . and .. above.
- (setq filename (files--splice-dirname-file dir target)
- done nil)
- ;; No, we are done!
- (setq done t))))))))
+ (let ((filename-no-dir (file-name-nondirectory filename)))
+ (if (equal ".." filename-no-dir)
+ (setq filename
+ (directory-file-name (file-name-directory (directory-file-name dir)))
+ done t)
+ (if (equal "." filename-no-dir)
+ (setq filename (directory-file-name dir)
+ done t)
+ ;; Put it back on the file name.
+ (setq filename (concat dir filename-no-dir))
+ ;; Is the file name the name of a link?
+ (setq target (file-symlink-p filename))
+ (if target
+ ;; Yes => chase that link, then start all over
+ ;; since the link may point to a directory name that uses links.
+ ;; We can't safely use expand-file-name here
+ ;; since target might look like foo/../bar where foo
+ ;; is itself a link. Instead, we handle . and .. above.
+ (setq filename (files--splice-dirname-file dir target)
+ done nil)
+ ;; No, we are done!
+ (setq done t)))))))))
filename))
(defun file-chase-links (filename &optional limit)
--
2.44.0
[-- Attachment #3: Type: text/plain, Size: 224 bytes --]
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
^ permalink raw reply related [flat|nested] 96+ messages in thread
* bug#70036: a fix that
2024-04-30 11:30 ` Ihor Radchenko
@ 2024-05-02 9:40 ` Eli Zaretskii
0 siblings, 0 replies; 96+ messages in thread
From: Eli Zaretskii @ 2024-05-02 9:40 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: theo, 70036, felician.nemeth, joaotavora
> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: joaotavora@gmail.com, felician.nemeth@gmail.com, 70036@debbugs.gnu.org,
> theo@thornhill.no
> Date: Tue, 30 Apr 2024 11:30:39 +0000
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> There are some easy things that can be done to improve `file-truename'
> >> performance somewhat. For example, the number of calls to
> >> `file-name-nondirectory' can be trivially reduced in the `file-truename'
> >> code - it is called up to three times in a row.
> >>
> >> (see my earlier message in https://yhetil.org/emacs-bugs/87jzlmd831.fsf@localhost/)
> >>
> >> Will it be of interest?
> >
> > Yes, of course. Those kinds of changes are no-brainers, really.
>
> See the attached patch.
Thanks, now installed on master.
^ permalink raw reply [flat|nested] 96+ messages in thread
end of thread, other threads:[~2024-05-02 9:40 UTC | newest]
Thread overview: 96+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-27 19:08 bug#70036: 30.0.50; Move file-truename to the C level Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-27 19:44 ` Eli Zaretskii
2024-03-27 21:56 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 1:14 ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 3:05 ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 7:04 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 7:03 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 6:22 ` Eli Zaretskii
2024-03-28 7:03 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-27 20:12 ` Felician Nemeth
2024-03-27 21:43 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 6:03 ` Eli Zaretskii
2024-03-28 7:10 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 8:52 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 11:55 ` Felician Nemeth
2024-03-28 12:08 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-30 9:46 ` Felician Nemeth
2024-03-30 11:18 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-30 12:45 ` Eli Zaretskii
2024-03-31 12:57 ` Felician Nemeth
2024-03-31 13:32 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 9:22 ` Ihor Radchenko
2024-03-28 10:59 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 11:18 ` Ihor Radchenko
2024-03-28 11:41 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 11:51 ` Ihor Radchenko
2024-03-28 12:47 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-28 13:52 ` Eli Zaretskii
2024-04-18 15:32 ` bug#70036: a fix that João Távora
2024-04-18 15:39 ` João Távora
2024-04-18 15:40 ` Ihor Radchenko
2024-04-18 15:45 ` João Távora
2024-04-18 15:49 ` Eli Zaretskii
2024-04-18 16:11 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-18 16:15 ` João Távora
2024-04-18 16:29 ` Eli Zaretskii
2024-04-18 17:22 ` João Távora
2024-04-18 17:53 ` Eli Zaretskii
2024-04-18 20:21 ` João Távora
[not found] ` <874jbycrd7.fsf@dick>
2024-04-18 21:26 ` João Távora
2024-04-18 21:37 ` João Távora
2024-04-19 9:17 ` Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-18 21:32 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-18 22:06 ` João Távora
2024-04-18 23:59 ` João Távora
2024-04-19 6:09 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 6:26 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 8:06 ` João Távora
2024-04-19 9:05 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 8:01 ` João Távora
2024-04-19 9:10 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 9:22 ` João Távora
2024-04-19 5:58 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 7:52 ` João Távora
2024-04-19 9:14 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 6:56 ` Eli Zaretskii
2024-04-19 7:51 ` Ihor Radchenko
2024-04-19 10:51 ` Eli Zaretskii
2024-04-30 11:30 ` Ihor Radchenko
2024-05-02 9:40 ` Eli Zaretskii
2024-04-19 8:27 ` João Távora
2024-04-19 8:49 ` João Távora
2024-04-19 11:12 ` Eli Zaretskii
2024-04-19 11:34 ` João Távora
2024-04-19 18:13 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 18:59 ` João Távora
2024-04-19 19:42 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 11:01 ` Eli Zaretskii
2024-04-19 11:32 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 11:40 ` João Távora
2024-04-19 11:47 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 11:51 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 12:01 ` João Távora
2024-04-19 11:51 ` João Távora
2024-04-19 20:23 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 21:32 ` João Távora
2024-04-19 11:53 ` Eli Zaretskii
2024-04-19 11:59 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 12:03 ` João Távora
2024-04-19 12:00 ` João Távora
2024-04-19 12:13 ` Eli Zaretskii
2024-04-19 12:20 ` João Távora
2024-04-19 6:45 ` Eli Zaretskii
2024-04-19 7:38 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-19 12:54 ` João Távora
2024-04-19 14:32 ` Eli Zaretskii
2024-04-19 0:57 ` Yuan Fu
2024-04-19 1:20 ` João Távora
2024-04-22 22:11 ` Dmitry Gutov
2024-04-18 16:21 ` Eli Zaretskii
2024-04-18 16:12 ` João Távora
2024-04-18 16:24 ` Eli Zaretskii
2024-04-18 16:33 ` Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-18 16:36 ` Eli Zaretskii
2024-04-18 17:26 ` João Távora
2024-04-18 17:27 ` João Távora
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).