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

Simplify nbb-related check #3466

Closed
vemv opened this issue Sep 16, 2023 · 3 comments
Closed

Simplify nbb-related check #3466

vemv opened this issue Sep 16, 2023 · 3 comments

Comments

@vemv
Copy link
Member

vemv commented Sep 16, 2023

Background

I noticed some time ago that even a cider-connect (which should be very fast) was a touch slower than usual.

Diagnostic

I found out that this piece of code:

cider/cider-connection.el

Lines 356 to 360 in 09d7219

;; This check is currently basically for nbb.
;; See `cider-sync-tooling-eval', but it is defined on a higher layer
(nrepl-dict-get
(nrepl-sync-request:eval "cljs.core/demunge" (current-buffer) nil 'tooling)
"value"))

Adds 2 seconds to the cider--connected-handler sequence.

(Those 2s will only be reproducible on a 'cold' JVM - subsequent calls will be faster. However please note that basically all JVMs are cold when you first start them)

Task

Run the check only when it's relevant.

I reckon it might be tricky since it might not be clear in advance whether we're looking at a nbb connection.

@vemv
Copy link
Member Author

vemv commented Sep 16, 2023

Hi @benjamin-asdf!

I noticed this issue and that you gracefully added nbb support to CIDER.

I wonder if you could check it out?

Perhaps it's easy if (cider-runtime) returns 'babashka for nbb connections. Is it the case?

If so, we could only run the check when the runtime is 'babashka - that would probably zap 90% of the problem.

Cheers - V

@vemv
Copy link
Member Author

vemv commented Sep 16, 2023

...while we're there, cljs.core/demunge could become (and (find-ns 'cljs.core) (resolve 'cljs.core/demunge)) - returning values is always at least a fast toucher than throwing exceptions (plus, we make sure we're not hitting anything like an exception-related nrepl middleware)

@benjamin-asdf
Copy link
Contributor

Nice find @vemv See pull request. Turns out we can rely on the describe op (runtime..) instead and shave of those 1-2 seconds for jvm startup!
I fear the penalty is then pushed down to the next first eval call though. Because as you mentionend, only the first takes this long. But I agree having the startup quick is nicer.

This is a 36% improvement on my end!

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 a pull request may close this issue.

2 participants