Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optimize lsp when it is called multiple times #1498

Merged
merged 1 commit into from Mar 9, 2020
Merged

optimize lsp when it is called multiple times #1498

merged 1 commit into from Mar 9, 2020

Conversation

redguardtoo
Copy link
Contributor

@redguardtoo redguardtoo commented Mar 9, 2020

If I use recommend setup (add-hook 'XXX-mode-hook #'lsp-deferred) in my javascript project, function lsp is called when I load a new js file. So the require statement in below code is always executed,

(defun lsp (&optional arg)
   ...
  (when (and lsp-auto-configure)
    (seq-do (lambda (package)
              ;; loading client is slow and `lsp' can be called repeatedly
              (require package nil t))
            lsp-client-packages))
  ...
)

and it's better not to load same package multiple times.

The steps to find find the bottleneck of loading js file,
Step 1, define below function my-connect-lsp,

(defun my-connect-lsp (&optional no-reconnect)
  "Connect lsp server.  If NO-RECONNECT is t, don't shutdown existing lsp connection."
  (interactive "P")
  (when (and (not no-reconnect)
             (fboundp 'lsp-disconnect))
    (lsp-disconnect))
  (when buffer-file-name
    (unless (and (boundp 'lsp-mode) lsp-mode)
      (require 'profiler)
      (require 'lsp-mode)
      (profiler-stop)
      (profiler-cpu-start 1000000)
      (lsp-deferred))))

Step 2, patch lsp-mode.el with below patch,

diff --git a/lsp-mode.el b/lsp-mode.el
index 6f3dcb8..ffc1ee7 100644
--- a/lsp-mode.el
+++ b/lsp-mode.el
@@ -7173,6 +7173,7 @@ argument ask the user to select which language server to start. "
           (lsp-mode 1)
           (when lsp-auto-configure (lsp--auto-configure))
           (setq lsp-buffer-uri (lsp--buffer-uri))
+          (profiler-report-cpu)
           (lsp--info "Connected to %s."
                      (apply 'concat (--map (format "[%s]" (lsp--workspace-print it))
                                            lsp--buffer-workspaces)))))

Step 3, open any js file and run my-connect-lsp, got below result,

image

@yyoncho
Copy link
Member

@yyoncho yyoncho commented Mar 9, 2020

I wonder whether we should add a variable here and execute this piece of code only once? WDYT?

@redguardtoo
Copy link
Contributor Author

@redguardtoo redguardtoo commented Mar 9, 2020

I wonder whether we should add a variable here and execute this piece of code only once? WDYT?

Please note my code already makes the lsp fast enough so only the first time call takes time. There is no need for extra flag to run lsp only once.

As you can see in the attached screenshot, the bottleneck is lsp-register-custom-settings (third line is too slow),

(defun lsp-register-custom-settings (props)
  (let ((-compare-fn #'lsp--compare-setting-path))
    (setq lsp-client-settings (-uniq (append props lsp-client-settings)))))

@kiennq
Copy link
Member

@kiennq kiennq commented Mar 9, 2020

I've updated your comment to make the image visible

@yyoncho yyoncho merged commit 355d4da into emacs-lsp:master Mar 9, 2020
1 check passed
@yyoncho
Copy link
Member

@yyoncho yyoncho commented Mar 9, 2020

Sorry, I didn't realize that the bottleneck is the register custom settings call.

Thank you for contributing to lsp-mode.

@nbfalcon
Copy link
Member

@nbfalcon nbfalcon commented Oct 19, 2020

Actually, that doesn't fix the issue at all - the featurep check with require is completely redundant, since require already checks if a feature is provided before loading it.

The actual issue is as follows: lsp requires all packages in lsp-auto-configure-features. The problem here is that not all of them are installed, so they will never be loaded (NOERROR=t), meaning their features won't be provided. Require in that case is slowest, since it has to go trough the entire load-path for each package that isn't installed.

(benchmark-run 10 (require 'foo nil t))
(0.139475209 0 0.0)

takes 100ms on my system; that makes 10ms per not-installed package in lsp-auto-configure-features. We need to either remove non-installed features from lsp-auto-configure-features or guard that block. We could have clients add-to-list to that variable in autoloaded progn blocks.

@redguardtoo
Copy link
Contributor Author

@redguardtoo redguardtoo commented Oct 19, 2020

Hi, @nbfalcon , I checked the code of featurep and require,

image

I'm not familiar with Emacs C code. But looks featurep avoids the loop statement in Vcurrent_load_list

@nbfalcon
Copy link
Member

@nbfalcon nbfalcon commented Oct 20, 2020

@redguardtoo we might end up avoiding that loop, but that isn't the cause of require's slowness.
Please try the following in scratch:

(require 'lsp-mode) ; ensure that its already provided
(benchmark-run 1000 (require 'lsp-mode nil t))
(0.0015731909999999998 0 0.0)

Note how requiring lsp-mode, which is already provided, 1000 times, takes 10ms. Unlike I assumed, featurep is still much faster than require (microseconds), so I will revert my change. But that doesn't invalidate my other point: this performance issue isn't fixed. Your patches saves less than 10 milliseconds, which I wouldn't consider slow. The main performance issue, as described above, is walking the load-path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants