Faster REPL - don't load extra middleware on first sync eval request #2078

vspinu · 2017-08-22T01:09:35Z

This saves a couple of extra seconds and brings cider repl startup time to barebones empty-project lein repl of 4s for me.

Send :inhibit-cider-middleware condition on first request. Relies on a feature added in Dynamically load middleware cider-nrepl#438.
Combine require-repl-utils and set-initial-ns into one sync request for efficiency. Note that require-repl-utils needs to be a sync request. It could happen that the following init-debuger request would occur before it has finished, resulting in "reader/reader not found error".

vspinu · 2017-08-24T10:36:36Z

From #2081:

I was just wondering whether to try to include there some of @vspinu's performance improvement patches or push those back to 0.16.

It depends how long would it take to release 0.16. I would like to see #2069 in 0.16 and that change with associated features/fixes might take quite some months. If we can pull it in within a couple of months then this change can also go in 0.16, if not then it might be better included now.

As noted in 721440b, there is one side effect of dynamic loading of middlware - it can occur concurrently. But Clojure seems to have trouble loading namespaces in parallel.

I haven't investigated this thoroughly but I run into issues when debug middleware was requested before out middleware in async requests. As debug is heavy, out middleware ends up loaded simultaneously with debug and didn't load correctly. This is why I changed the order of those two requests on emacs side. out middleware is very light and doesn't cause issues with the debug.

Fixing this on cider's side is easy, just implement a kind of deffered chain to load cider's middleware. But then, some other packages (nrepl-refactor?) might load same namespaces in parallel. One partial solution is to run cider-connected-hook at the end of the deffered chain and request external packages to rely on cider-connected-hook (which they should do anyways).

xiongtx · 2017-08-24T17:11:43Z

But Clojure seems to have trouble loading namespaces in parallel.

Is this due to dependencies between namespaces? Needs a topological sort?

vspinu · 2017-08-24T17:42:25Z

Is this due to dependencies between namespaces? Needs a topological sort?

No. It's just that if two actions simultaneously try to load same namespace, something goes wrong and one of them is not loaded correctly.

xiongtx · 2017-08-24T23:25:30Z

Can you put up a minimal repo that reproduces this problem? Seems like a Clojure level issue, not cider-nrepl.

vspinu · 2017-08-24T23:45:59Z

Yes, it's a Clojure level issue. If I had a minimal rep. example I would have had already reported it on JIRA.

bbatsov · 2017-08-25T04:43:30Z

As noted in 721440b, there is one side effect of dynamic loading of middlware - it can occur concurrently. But Clojure seems to have trouble loading namespaces in parallel.

Things like these are generally my concern - don't like introducing potentially breaking changes in a bugfix release.

It depends how long would it take to release 0.16. I would like to see #2069 in 0.16 and that change with associated features/fixes might take quite some months. If we can pull it in within a couple of months then this change can also go in 0.16, if not then it might be better included now.

There's never a precise roadmap for releases. Generally I cut them when I feel we've added something important enough to warrant a release.

bbatsov · 2017-09-16T10:06:37Z

cider-repl.el

-  "Set the REPL BUFFER's initial namespace (by altering `cider-buffer-ns').
-This is \"user\" by default but can be overridden in apps like lein (:init-ns)."
+(defun cider-repl-require-repl-utils-and-set-ns (buffer)
+  "Require standard REPL util functions and set the REPL BUFFER NS.


NS -> ns (it's not a param).

bbatsov · 2017-09-16T10:07:02Z

cider-repl.el

-This is \"user\" by default but can be overridden in apps like lein (:init-ns)."
+(defun cider-repl-require-repl-utils-and-set-ns (buffer)
+  "Require standard REPL util functions and set the REPL BUFFER NS.
+Namespace is \"user\" by default but can be overridden in apps like


default, but

bbatsov · 2017-09-16T10:07:13Z

cider-repl.el

+(defun cider-repl-require-repl-utils-and-set-ns (buffer)
+  "Require standard REPL util functions and set the REPL BUFFER NS.
+Namespace is \"user\" by default but can be overridden in apps like
+lein (:init-ns). Booth of these operations need to be done as a sync


booth -> both

bbatsov · 2017-09-16T10:07:46Z

cider-repl.el

+  "Require standard REPL util functions and set the REPL BUFFER NS.
+Namespace is \"user\" by default but can be overridden in apps like
+lein (:init-ns). Booth of these operations need to be done as a sync
+request at the beginning of the session. Bundling them for efficiency."


There's one space missing here before the final sentence.
them -> them together

bbatsov · 2017-09-16T10:09:04Z

cider-repl.el

  (when cider-repl-display-in-current-window
    (add-to-list 'same-window-buffer-names (buffer-name buffer)))
  (pcase cider-repl-pop-to-buffer-on-connect
    (`display-only (display-buffer buffer))
    ((pred identity) (pop-to-buffer buffer)))
+  (cider-repl-require-repl-utils-and-set-ns buffer)


Why did you move those down?

The REPL shows a bit earlier (.5-1 sec for me). Also, the repl is always shown, even when errors occur with those subsequent requests.

bbatsov · 2017-09-16T10:10:36Z

cider.el

    (cider--subscribe-repl-to-server-out)
    (when cider-auto-mode
      (cider-enable-on-existing-clojure-buffers))
+    ;; Middleware on cider-nrepl side is loaded dynamically on first
+    ;; usage. Loading middleware concurrently can lead to confusing issues
+    ;; (likely a clojure bug). Debug middleware is heavy, so enable it at the


I recall you verified this is a bug, so I think you should mention it here. The next sentence doesn't sound very reassuring. 😃

bbatsov · 2017-09-16T10:11:31Z

I guess you can also add to this PR a changelog entry, as users are going to be quite excited about the speedup.

bbatsov · 2017-09-27T05:13:17Z

@vspinu ping :-)

vspinu · 2017-09-27T16:38:56Z

Sorry for being slow on this. My kid was just born a couple of weeks ago, so my bandwidth is 1/10th of what it was before :) Will try to fix all my PRs by Saturday.

bbatsov · 2017-09-27T17:12:16Z

Sorry for being slow on this. My kid was just born a couple of weeks ago, so my bandwidth is 1/10th of what it was before :) Will try to fix all my PRs by Saturday.

Thanks and congratulations! 🎉 🎂

- Sending :inhibit-cider-middleware condition on first request - Combining require-repl-utils and set-initial-ns into one sync request for efficiency

vspinu · 2017-09-30T22:13:52Z

@bbatsov, I have addressed your comments in all of my PRs (also the one on cider-nrepl side). Added all change-log entries here to avoid conflicts.

bbatsov · 2017-10-01T08:12:03Z

👍

vspinu mentioned this pull request Aug 24, 2017

Cider won't start under Clojure 1.9.0-alpha18 #2081

Closed

bbatsov reviewed Sep 16, 2017

View reviewed changes

vspinu force-pushed the faster-repl branch 2 times, most recently from 5dbdf2f to 4b4aa08 Compare September 30, 2017 20:06

vspinu added 2 commits September 30, 2017 23:45

Faster REPL - don't load extra middleware on first sync request

1051494

- Sending :inhibit-cider-middleware condition on first request - Combining require-repl-utils and set-initial-ns into one sync request for efficiency

Init the debugger at the end to avoid concurrent middleware loading

d968c82

vspinu force-pushed the faster-repl branch from 4b4aa08 to d968c82 Compare September 30, 2017 21:46

Document recent changes by @vspinu

0e54b5b

bbatsov merged commit 46367ce into clojure-emacs:master Oct 1, 2017

vspinu mentioned this pull request Oct 2, 2017

cider-jack-in broken repl with latest from elpa #2092

Closed

vspinu deleted the faster-repl branch June 20, 2018 10:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster REPL - don't load extra middleware on first sync eval request #2078

Faster REPL - don't load extra middleware on first sync eval request #2078

vspinu commented Aug 22, 2017

vspinu commented Aug 24, 2017 •

edited

Loading

xiongtx commented Aug 24, 2017 •

edited

Loading

vspinu commented Aug 24, 2017

xiongtx commented Aug 24, 2017 •

edited

Loading

vspinu commented Aug 24, 2017 •

edited

Loading

bbatsov commented Aug 25, 2017 •

edited

Loading

bbatsov Sep 16, 2017

bbatsov Sep 16, 2017

bbatsov Sep 16, 2017

bbatsov Sep 16, 2017

bbatsov Sep 16, 2017

vspinu Sep 30, 2017

bbatsov Sep 16, 2017

bbatsov commented Sep 16, 2017

bbatsov commented Sep 27, 2017

vspinu commented Sep 27, 2017

bbatsov commented Sep 27, 2017

vspinu commented Sep 30, 2017

bbatsov commented Oct 1, 2017

Faster REPL - don't load extra middleware on first sync eval request #2078

Faster REPL - don't load extra middleware on first sync eval request #2078

Conversation

vspinu commented Aug 22, 2017

vspinu commented Aug 24, 2017 • edited Loading

xiongtx commented Aug 24, 2017 • edited Loading

vspinu commented Aug 24, 2017

xiongtx commented Aug 24, 2017 • edited Loading

vspinu commented Aug 24, 2017 • edited Loading

bbatsov commented Aug 25, 2017 • edited Loading

bbatsov Sep 16, 2017

Choose a reason for hiding this comment

bbatsov Sep 16, 2017

Choose a reason for hiding this comment

bbatsov Sep 16, 2017

Choose a reason for hiding this comment

bbatsov Sep 16, 2017

Choose a reason for hiding this comment

bbatsov Sep 16, 2017

Choose a reason for hiding this comment

vspinu Sep 30, 2017

Choose a reason for hiding this comment

bbatsov Sep 16, 2017

Choose a reason for hiding this comment

bbatsov commented Sep 16, 2017

bbatsov commented Sep 27, 2017

vspinu commented Sep 27, 2017

bbatsov commented Sep 27, 2017

vspinu commented Sep 30, 2017

bbatsov commented Oct 1, 2017

vspinu commented Aug 24, 2017 •

edited

Loading

xiongtx commented Aug 24, 2017 •

edited

Loading

xiongtx commented Aug 24, 2017 •

edited

Loading

vspinu commented Aug 24, 2017 •

edited

Loading

bbatsov commented Aug 25, 2017 •

edited

Loading