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

Expensive calls tocider-current-repl in latency sensitive contexts #3344

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

Expensive calls tocider-current-repl in latency sensitive contexts #3344

yuhan0 opened this issue May 19, 2023 · 10 comments

Comments

@yuhan0
Copy link
Contributor

yuhan0 commented May 19, 2023

Expected behavior

Several features in Cider run on short idle timers or hooks that run synchronously while the user is typing/editing.

  • Eldoc hints
  • Company etc. auto completion
  • electric / aggressive indentation modes
  • Auto formatting

Any latency or momentary freezes in these contexts make for a unpleasant editing experience, so they should be made as optimized as possible.

Actual behavior

Many of these features call the harmless-looking (cider-current-repl) under the hood, and profiling shows it is in fact an expensive operation involving various heuristics and filesystem lookups.

The performance impact grows ~linearly with the number of open REPL connections, See #3343 for more details and a sample backtrace.

  • cider-eldoc and cider-complete via. (cider-connected-p)
  • clojure-indent-line via cider--get-symbol-indent -> cider-resolve-var.

These are the main ones that showed up when profiling my own editing usage - eg. I don't use cider-format but that seems like it should be affected too. Searching the codebase also finds various cider-nrepl-sync-request functions taking an optional connection arg which forwards to a call to (cider-current-repl).

Steps to reproduce the problem

Set the eldoc timer to a short delay to magnify the issues:
(setq eldoc-idle-delay 0.01)
then jack-in to many projects simultaneously.

Navigating or typing in a connected clj source buffer should get increasingly laggy with the number of open connections.

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

Operating system

macOS 12.5.1

JDK distribution

Temurin jdk-20.0.1+9

@vemv
Copy link
Member

vemv commented May 19, 2023

Several features in Cider run on short idle timers or hooks that run synchronously while the user is typing/editing.
Eldoc hints
Company etc. auto completion
electric / aggressive indentation modes
Auto formatting

I'm not sure this statement is true.

  • Eldoc, presumably, is only invoked when one is done typing
    • Therefore, arguably, (setq eldoc-idle-delay 0.01) is not a valid repro
    • Observation - many systems "cannot perform" if you push them at 100x their intended capacity! And that's perfectly fine
  • company is only triggered on TAB or such
  • aggressive indentation is not a CIDER feature - how they implement it is outside of our control
  • Auto formatting should happen on specific moments (presumably on save / or M-x cider-format*)
    • this seems very different from while the user is typing/editing - we don't interrupt the user

Just like in today's other issue, I'm not trying to negate the problems - let's just simply make sure that the diagnostics are precise. We cannot come up with a correct solution otherwise.

I also think it would be a good idea to create one issue per problem (I'd be OK with an issue in this repo for the agressive indentation, even if it's not "our" problem. Spoiler: maybe we should create a specially-tailored, lightweight indentation mode just for that context).

Cheers - V

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 19, 2023

Thanks for the feedback! These findings were prompted by my direct experience of stuttering and latency while typing, so the problem definitely exists to some extent. I can't prove a direct link to eg. Eldoc but the profiler shows a large percentage of CPU and memory being taken up by its idle timer, specifically by the call to cider-current-repl.

My eldoc-idle-delay is set to the default value of 0.5s, but I suggested pushing it to an extreme value to magnify the effects on more powerful machines where they might be hard to observe.

Eldoc, presumably, is only invoked when one is done typing

Code is usually not typed in a single unbroken stream, so once you pause for half a second between keystrokes, Eldoc gets triggered, and then the next character you typed does not appear until it is done with its request. This happened frequently enough to feel stuttery and jarring, to the point that I ended up spending hours to track down these performance issues and file bug reports 😅

company is only triggered on TAB or such

There are many ways to configure Company - I believe you can also have it trigger on idle delay so an overlay / childframe pops up when you pause typing. I recently tried out Corfu which has this option in corfu-auto-mode, which is where I noticed the same sort of stuttering issues, ultimately due to cider-complete freezing Emacs between keystrokes.

aggressive indentation is not a CIDER feature - how they implement it is outside of our control

Agreed, which is why I did not mention it originally - essentially it calls indent-region on a short idle timer to keep everything in the current defun aligned, working on the assumption that indenting is a very cheap operation. And indeed it works smoothly in Elisp, no stuttering even on very large forms. So it's unfortunate that Cider's indentation logic isn't optimized enough to keep up with this mode, although I've already made changes that bring it up to speed - no need for a separate lightweight mode!

Auto formatting should happen on specific moments (presumably on save / or M-x cider-format*)

Yes, feel free to ignore this one - I didn't have performance issues with formatting, but added it for 'completeness' after grepping through the codebase to see where else (cider-current-repl) was being used.

@vemv
Copy link
Member

vemv commented May 20, 2023

Code is usually not typed in a single unbroken stream, so once you pause for half a second between keystrokes, Eldoc gets triggered, and then the next character you typed does not appear until it is done with its request. This happened frequently enough to feel stuttery and jarring, to the point that I ended up spending hours to track down these performance issues and file bug reports

Ok. Anyway, had you considered increasing eldoc-idle-delay? One doesn't use eldoc all the time. We could document that recommendation somewhere. Or we could go as far as changing the default in a buffer-local manner, for all buffers with CIDER enabled and that happen to use the default value.

This doesn't close the door to optimizing this code path, however I'd find it wise to prioritize things very carefully.

I recently tried out Corfu which has this option in corfu-auto-mode,

I find this a delicate point, because I don't want to tell you "don't use that", however not all features are necessarily good ideas all the time. As I would imagine it, corfu-auto-mode blindly trusts that any given completion backend is instantaneous?

CIDER, while of course strives to be performant, is generally built on the principle of runtime-powered insights. This can easily involve hitting the JVM, clojure code, the filesystem, etc. That performs more than acceptably for the constraints it was built for, however corfu-auto-mode is newer / built in a non-coordinated fashion.

Again, that's not to close the door to possible optimizations. Let's just perceive edge cases as such when, IMO, they are. That allows optimal task prioritization.

So it's unfortunate that Cider's indentation logic isn't optimized enough to keep up with this mode, although I've already made changes that bring it up to speed - no need for a separate lightweight mode!

Nice. I guess that this is a forthcoming PR? Looking forward!

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 21, 2023

I'll admit that a large part of my motivation here is a sort of Moral Indignation at poorly optimized code... ie. how many battery lifespans and CPU cycles are being spent churning away in the background at something so seemingly trivial as the current REPL 🫠. Like a Bitcoin miner, generating waste heat while doing no useful work.

Tonsky has a great rant on the topic: https://tonsky.me/blog/disenchantment/ Not to get too philosophical, but Emacs is such a rare blessing in the modern software landscape, where an end-user like me can experience faults with a tool and be empowered enough to introspect the root causes and modify them directly.

So suggestions to accommodate the lagginess by avoiding certain completion styles or having Cider tweak the buffer-local Eldoc configs feel somewhat beside the point, when there appear to be low-hanging performance gains to be had... the tradeoff between dumb static linting vs. runtime-powered smarts shouldn't have to be such a hard dichotomy.

And speaking of delicate points, there's maybe a element of privilege involved when it comes to prioritizing these performance concerns. They disproportionately affect users on older hardware, and the fact is I'm already encountering them on a relatively new M1 Macbook Pro, even if they only crop up in somewhat extreme 'edge cases'.

Putting some concrete numbers to it, I had 4-5 separate REPLs open and maybe 60+ clj files in the background (the majority from nosing around in clj-kondo's corpus directory). After pruning most of these with the help of ibuffer, things quickly got back to a usable state.
But it does mean that someone running Cider on a slower machine (by some factor X) would reach this threshold sooner. Eg. with 2 REPLs and 20 open buffers causing stuttery levels of typing latency, and something like 5 REPLs being straight up unusable without sacrificing Eldoc and other niceties.

Just trying to be aware that our 'barely noticeable edge cases' might be someone else's 'annoying daily occurrence', any chance to save resources and widen accessibility in general shouldn't be lightly overlooked.

@vemv
Copy link
Member

vemv commented Jun 23, 2023

I hadn't replied to this one back then, in good part because I didn't feel like arguing on the internet...

My tldr would be that PRs are most welcome - let's just try together to summarize things as they really are. OP appears to describe a series of acute problems, that later can be seen as Moral Indignation ones, as you worded it...

I'll gladly review optimizations. I think it's also reasonable to weigh the value of PRs against any possible complexity or lack of test coverage they might, hypothetically, produce.

I don't think that would be the case with you as we've seen nothing but impressive contributions!

@vemv
Copy link
Member

vemv commented Jun 23, 2023

btw, (cider-current-repl) quickly enters into Sesman territory, which we intend to simplify, and, why not, optimize.

Would you be OK if we close this issue and create one named Ensure (cider-current-repl) is fast?

That would seem focused and actionable. It would be probably one of the finishing touches (while also a design concern) in the sesman refactoring.

@yuhan0
Copy link
Contributor Author

yuhan0 commented Jun 24, 2023

Yeah absolutely - looking back I'm not sure why I was making such a big deal of this, some work related frustrations seeping through at the time, probably 😶 Sorry about that, and thanks for your work helping to maintain the project! @vemv Would you prefer to write up the new issue yourself?

@yuhan0 yuhan0 closed this as completed Jun 24, 2023
@vemv
Copy link
Member

vemv commented Jun 24, 2023

Cheers 🍻

Will create an issue. I think GH recently introduced "subtask" issue types.

@vemv
Copy link
Member

vemv commented Sep 12, 2023

In case it's useful, today I ran the following experiment:

(require 'benchmark)
(format "%.8f" (benchmark-elapse (cider-current-repl)))

It returns 0.00005400. That might be fast enough for many problems to disappear?

(We have fixed/simplified a bunch of Sesman aspects this summer)

Would love to know if anything has changed for you.

Cheers - V

@vemv
Copy link
Member

vemv commented Sep 13, 2023

I have a wip PR that further optimizes things #3463

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

No branches or pull requests

3 participants