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

Performance issues #3343

Closed
yuhan0 opened this issue May 18, 2023 · 10 comments
Closed

Performance issues #3343

yuhan0 opened this issue May 18, 2023 · 10 comments

Comments

@yuhan0
Copy link
Contributor

yuhan0 commented May 18, 2023

Sorry in advance for the long and slightly rambly post, and not sticking to the template as these aren't actual bugs but performance issues. I recently had to restart a repl multiple times in a row, and decided to investigate the long periods of freezing after a cider-connect, and the frustrating stutters and lags that can come when with dealing with multiple REPL connections / many Clojure buffers in the background / large top-level expressions.

Computing (cider-current-repl)

This appears to be the most serious culprit - (cider-current-repl) is called frequently all over the place, in particular by eldoc and completion frameworks which are run while the user is typing. Any momentary freezes or extra latency in such situations are especially noticeable.

The problem is that every invocation of cider-current-repl does an expensive traversal over all the Sesman sessions (~ repls) , repeatedly calculating if each session is "friendly" in the current context, filtering links, sorting by relevance, etc.

Here is a typical backtrace from the CPU profiler taken from a few seconds of stuttery typing in a clj buffer:

     Samples  %     Function
         655  53% - timer-event-handler
         655  53%  - apply
         608  49%   - explain-pause--wrap-callback
         604  49%    - apply
         477  38%     - #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_12>
         477  38%      - eldoc-print-current-symbol-info
         477  38%       - eldoc--invoke-strategy
         477  38%        - eldoc-documentation-default
         477  38%         - cider-eldoc
         477  38%          - if
         459  37%           - and
         458  37%            - cider-connected-p
         458  37%             - process-live-p
         458  37%              - get-buffer-process
         458  37%               - cider-current-repl
         458  37%                - let
         458  37%                 - if
         458  37%                  - let*
         458  37%                   - cider-repls
         458  37%                    - let
         456  37%                     - cond
         456  37%                      - let
         456  37%                       - cdr
         456  37%                        - if
         456  37%                         - sesman-current-session
         449  36%                          - sesman--friendly-sessions
         449  36%                           - seq-filter
         449  36%                            - seq-map
         449  36%                             - apply
         449  36%                              - #<compiled 0x184952064dc8c434>
         449  36%                               - mapcar
         449  36%                                - #<compiled -0x1c3c6faa880b536a>
         449  36%                                 - #<compiled 0xa0720de2248211>
         449  36%                                  - sesman-friendly-session-p
         449  36%                                   - apply
         449  36%                                    - #<lambda 0x1b3898c825b0516>
         449  36%                                     - progn
         449  36%                                      - progn
         449  36%                                       - let*
         447  36%                                        - and
         218  17%                                         - file-truename
         169  13%                                          - file-truename
         137  11%                                           - file-truename
         110   8%                                            - file-truename
          81   6%                                             - file-truename
          42   3%                                              - file-truename
          18   1%                                                 file-truename

Since I do mostly JVM Clojure development on long-running REPLs, the buffer -> repl mappings practically never change, and all these CPU cycles are wasted on returning the exact same result each time. Even in CLJS development with multiple connections it shouldn't be necessary to recompute this information so frequently!

Refreshing the dynamic font lock

This is a less frequently occuring but more significant lag, taking place on initial cider-connect or cider-jack-in, ocasionally on interactive evals and ns refreshes. Each time it happens the entire interface freezes for up to a few seconds, the worst ones I've experienced were maybe 20-30 seconds long.

The underlying cause turned out to be cider-refresh-dynamic-font-lock being indiscretely recomputed on background Clojure buffers, including ones belonging to separate sessions/projects. This can be an expensive operation per buffer, some calls took up to 300ms.

As far as I can tell there are two paths which trigger this:

  • cider-enable-on-existing-clojure-buffers after an initial connection, which reinitializes (cider-mode +1) on every Clojure-mode buffer in (buffer-list), including edn files, background *diff-syntax* buffers used to fontify diffs, and various other temp buffers. Among other things this forces recompilation of the font-lock-keywords in that buffer.
  • cider-repl--state-handler responding to a list of changed-namespaces from the server, and refreshing the font lock in all cider-mode buffers that reference those namespaces. The issue is that these buffers may belong to different connections/repls, and should not be affected when they share a common namespace like user. Because (cider-current-ns) defaults to user for namespace-less edn or project.clj files, these will also be refreshed. On initial connect, clojure.core is one of these changed-namespaces, so this also triggers another refresh on practically all source buffers in the background.

Possible solutions

Somewhere along the cider-current-repl callstack shown above it should be possible to memoize the output of one of these functions - I suspect either cider-repls or sesman-current-session? I'm happy to dive in and stick a buffer-local cache variable in a way that works for my clj-centric workflow, but I'm not familiar enough with the Clojurescript ecosystem to know what situations this cache has to be invalidated and recomputed, which will probably break somebody else's workflow. cider-connected-hook and cider-disconnected-hook are obvious sites to invalidate the cache, and probably "upgrading" from a clj to cljs repl - Is there a trigger for "downgrading" when the JS environment is disconnected (browser close)? And there's ClojureCLR, ClojureDart, Babashka, Node repls, nbb, piggiebacks, figwheels, various other words that I've heard of but unsure if they're relevant to this situation..

Both issues might have a common theme, that the Sesman API does not seem to have an easily accessible (and fast) way of mapping REPLs to their associated buffers.

The mapping instead goes from buffers to 'relevant' sessions, via cider-current-session(s), which feels somewhat backward? Still struggling to understand the terminology (siblings, friendliness, linked projects, the difference between sessions/hosts/connections/repls etc.), but I can appreciate some of the complexity involved.

If each session could keep a cached list/hashmap of buffers it applies to, then (cider-current-repl) in a buffer could just iterate through existing sessions, and pick the most relevant one which applies to it. And it would be a simple matter to target dynamic font-lock refreshing to only buffers belonging to the same session.

About cider-enable-on-all-existing-clojure-buffers, I'm unconvinced why this function exists at all? Cider users should already have it added to their clojure-mode-hook, otherwise it seems intrusive to automatically add it and force Cider on all other buffers, even ignoring the performance issues. I also found in my local copy of Cider that I had commented out cider-possibly-disable-on-existing-clojure-buffers a few years ago for some reason, I think due to unexpected behaviour on repl disconnect.

Environment & Version information

CIDER version information

;; CIDER 1.7.0 (Côte d'Azur), nREPL 1.0.0
;; Clojure 1.11.1, Java 20

Lein / Clojure CLI version

Clojure CLI version 1.11.1.1273

Emacs version

GNU Emacs 29.0.90 (build 1, aarch64-apple-darwin21.6.0, NS appkit-2113.60 Version 12.5.1 (Build 21G83)) of 2023-05-10

Operating system

macOS 10.14

JDK distribution

Temurin jdk-20.0.1+9

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 18, 2023

Another thing that should be possible to optimize is the indentation code, I've found that editing large (100+ LOC) forms with aggressive-indent-mode turned on is painfully laggy, and profiling shows a lot of work being done via cider--get-symbol-indent->cider-resolve-var, which does an O(n) lookup in the nrepl dict of every symbol in the current ns. This could probably be a cached hashmap that gets invalidated in the cider-repl--state-handler only when a namespace is changed, along with the font lock keywords.

@vemv
Copy link
Member

vemv commented May 18, 2023

Sounding good, kudos for the investigation!

It might be more tractable to split this into one issue per problem?

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 18, 2023

Yes, I was hoping to fix this myself but got more uncertain as I dug into Sesman and its abstractions - part of the complexity is Sesman trying to be as generic as possible and introducing these layers of indirection, although Cider looks to be its only implementation.

I ended up wondering, "why not let each REPL keep track of its buffers?" which seems like it would fix both of the issues. But not being the target audience for why Sesman exists in the first place (time-varying, many-to-many relationships between buffers and REPLs?), there are probably some invalid simplifying assumptions I'm making.

So maybe it's better to list the surface issues than jump to an underlying fix. I'd say there are 4 of them?

  • Expensive computation of current REPL in latency-sensitive contexts (eldoc, completion, indentation),
  • unnecessary refreshing of font-lock keywords in buffers where nothing needs to be recomputed
  • sub-optimal means of retrieving indentation specs
  • Strange cider-auto-mode (not actually a mode) way of enabling/disabling Cider across Clojure buffers, possibly a relic from early in the development period

If you like I can open separate issues for each and maybe a Discussion for how Sesman interacts with more complex cljs etc. workflows.

@vemv
Copy link
Member

vemv commented May 18, 2023

If you like I can open separate issues for each and maybe a Discussion for how Sesman interacts with more complex cljs etc. workflows.

It SGTM, thanks!

wrt Sesman, just a few days ago I talked with @bbatsov about proceeding with his intention (which is reflected in some GH thread from months ago): we just be able to optimize for the "80%" case (single project, one clj repl, maybe 1 cljs repl, dead-simple matching between buffers and repls depending on the file extension and/or major mode).

(That's an approximate idea - not authoritative)

Given all of us three want more or less the same thing for Sesman, feel free to create an issue if there isn't a relevant one already.


FWIW I intend to recap what Sesman intends to solve, where/how it fails, and finally fix it, but that's fully compatible with the described task.

i.e. we can have a feature flag (just like we have for enrich-classpath - another pending topic).

@bbatsov
Copy link
Member

bbatsov commented May 19, 2023

wrt Sesman, just a few days ago I talked with @bbatsov about proceeding with his intention (which is reflected in some GH thread from months ago): we just be able to optimize for the "80%" case (single project, one clj repl, maybe 1 cljs repl, dead-simple matching between buffers and repls depending on the file extension and/or major mode).

I'd say that's a good summary. I still like the concept of sessions as a way of grouping related connections, but I don't like the complicated automated session discovery we currently use, so I'd us to make this manual and as simple as possible.

FWIW I intend to recap what Sesman intends to solve, where/how it fails, and finally fix it, but that's fully compatible with the described task.

I'll dig the original issue somewhere, but it was basically create to simplify the previous automatic REPL inference logic and because the author of sesman needed similar logic for another project this was made as a standalone package instead being part of CIDER.

@bbatsov
Copy link
Member

bbatsov commented May 19, 2023

Re sesman's mission - see vspinu/sesman#1 and #2069 Lots of discussions and background there.

@bbatsov
Copy link
Member

bbatsov commented May 19, 2023

Yes, I was hoping to fix this myself but got more uncertain as I dug into Sesman and its abstractions - part of the complexity is Sesman trying to be as generic as possible and introducing these layers of indirection, although Cider looks to be its only implementation.

Touche - that's why I was skeptical from the very beginning of relying on a generic library. 5 years down the road our experience points us in the direction of just the session management for the specific needs of CIDER and making it part of CIDER. This will take care of much of the performance issues you've observed. Not to mention that if there's a manual session selection by default there's no inference logic overhead.

@bbatsov
Copy link
Member

bbatsov commented May 19, 2023

About cider-enable-on-all-existing-clojure-buffers, I'm unconvinced why this function exists at all? Cider users should already have it added to their clojure-mode-hook, otherwise it seems intrusive to automatically add it and force Cider on all other buffers, even ignoring the performance issues. I also found in my local copy of Cider that I had commented out cider-possibly-disable-on-existing-clojure-buffers a few years ago for some reason, I think due to unexpected behaviour on repl disconnect.

That's mostly for the benefit of people who are using both CIDER and inf-clojure, as them enabling them the minor modes in clojure-mode-hook would create some issues with conflicting functionality. So, it was basically some shortcut to prevent them from doing a bit of manual work. Not sure how many people rely on this - at any rate I'm not attached to this functionality at all and I'm open to rethinking/removing it.

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 19, 2023

Thanks for the clarifications! Think I'll leave the higher level design decisions wrt. Sesman to the rest of the team who have a better idea of the considerations and workflows to support.

Closing this issue in favour of the separate ones created.

I've also hacked together a hashmap cache for indent-specs, experiencing a significant improvement in editing latency on large defuns :) Will test it out more before submitting a PR.

@yuhan0 yuhan0 closed this as completed May 19, 2023
@vspinu
Copy link
Contributor

vspinu commented May 20, 2023

Sesman trying to be as generic as possible and introducing these layers of indirection

Context to which REPL is associated can be pretty much anything. For example a code block in a multi-mode buffer (org-mode, markdown etc). This sesman's question suggested using git-branch as a context. Context could be a virtual environment which is currently active or maybe dir-local .env environment variables.

For this reason keeping a cache of all linked contexts together with a REPL didn't seem like a good idea. Even with buffers as contexts it's not entirely trivial how to manage that cache.

Efficiency wasn't a concern because the assumption was that sesman would be used primary in interactive use-cases not in font-lock. Maybe the right approach is to investigate why cider-current-repl is used so frequently during the font-lock and cache the results accordingly.

Regarding the session concept. The notion of session is really unavoidable if we want to deal with multiple repl's (clj/cls) as a unit. But the primary use case for session abstraction is actually to encapsulate tooling associated with a running repls - completion processes, flyspell, ssh connection etc.

author of sesman needed similar logic for another project this was made as a standalone package instead being part of CIDER.

Yeh ... I am really sorry for becoming a bottleneck on this one. I indeed planned to use it for ESS and eventually to create a lightweight language agnostic repl-interaction package with sesman at the core. I still hope for that. But life turned into a different direction. I stopped using clojure and at this moment I am not even allowed to use emacs for my work for security reasons.

Feel free to move away from sesman of course. But if there is anything I can do on the sesman side to improve the current situation please let me know. I will try to find time.

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

No branches or pull requests

4 participants