cider-eval-last-sexp became slow on Android

[alexander-yakushev]

At some point of time CIDER became less responsive on Android than it had been before. When doing C-x C-e on some simple values (just constants) there is a noticeable (~1 sec) delay before the value returns back. When typing the same in CIDER's REPL the response arrives almost immediately. Or, another example, completion suggestions show up also almost immediately when triggered. When I do M-: (cider-interactive-eval "42" 0 nil) — the delay is there. So the problem seems to touch only cider-interactive-eval.

Were there any server-side changes to how this function works recently? It is also possible that reflection is to blame (reflection is a weak spot on Android), I have the list of reflection cases but can't figure which one can be responsible:

Reflection warning, clojure/tools/nrepl/middleware.clj:135:3 - call to method replaceAll can't be resolved (target class is unknown).
Reflection warning, clojure/tools/trace.clj:231:9 - call to method setStackTrace on java.lang.Object can't be resolved (no such method).
Reflection warning, clojure/tools/trace.clj:242:11 - call to method setStackTrace on java.lang.Object can't be resolved (no such method).
Reflection warning, clojure/tools/trace.clj:244:49 - call to java.lang.Exception ctor can't be resolved.
Reflection warning, clojure/tools/trace.clj:244:11 - call to method setStackTrace on java.lang.Object can't be resolved (no such method).
Reflection warning, clojure/tools/trace.clj:277:11 - call to method setStackTrace on java.lang.Object can't be resolved (no such method).
Reflection warning, clojure/tools/trace.clj:278:17 - call to method setStackTrace on java.lang.Object can't be resolved (no such method).
Reflection warning, clojure/tools/nrepl/ack.clj:47:3 - reference to field close can't be resolved.
Reflection warning, clojure/tools/nrepl/middleware/interruptible_eval.clj:53:52 - reference to field iterator on java.lang.Object can't be resolved.
Reflection warning, clojure/tools/nrepl/middleware/interruptible_eval.clj:109:3 - call to java.util.concurrent.ThreadPoolExecutor ctor can't be resolved.
Reflection warning, clojure/tools/nrepl/middleware/session.clj:41:54 - call to method append on clojure.tools.nrepl.StdOutBuffer can't be resolved (argument types: java.lang.CharSequence, java.lang.Integer, java.lang.Number).
Reflection warning, clojure/tools/nrepl/middleware/session.clj:230:30 - call to method put on java.io.Writer can't be resolved (no such method).
Reflection warning, clojure/tools/nrepl/middleware/session.clj:230:30 - call to method put on java.io.Writer can't be resolved (no such method).
Reflection warning, clojure/tools/nrepl/server.clj:42:21 - reference to field close can't be resolved.
Reflection warning, clojure/tools/nrepl/server.clj:133:28 - call to java.net.InetSocketAddress ctor can't be resolved.
Reflection warning, dynapath/defaults.clj:13:52 - reference to field getURLs can't be resolved.
Reflection warning, dynapath/defaults.clj:27:28 - call to method addURL can't be resolved (target class is unknown).
Reflection warning, dynapath/util.clj:30:22 - reference to field getParent can't be resolved.
Reflection warning, cider/nrepl/middleware/util/java/parser.clj:74:37 - reference to field getKind can't be resolved.
Reflection warning, cider/nrepl/middleware/util/java/parser.clj:74:37 - reference to field asInterface can't be resolved.
Reflection warning, cider/nrepl/middleware/util/java/parser.clj:257:35 - reference to field classes can't be resolved.
Reflection warning, cljs_tooling/complete.clj:70:20 - call to method startsWith can't be resolved (target class is unknown).
Reflection warning, clojure/java/classpath.clj:65:19 - reference to field getParent can't be resolved.
Reflection warning, clojure/tools/nrepl/cmdline.clj:83:20 - reference to field getLocalPort can't be resolved.
Reflection warning, clojure/tools/nrepl/cmdline.clj:84:30 - reference to field getLocalPort can't be resolved.
Reflection warning, clojure/tools/nrepl/cmdline.clj:86:17 - reference to field getLocalPort can't be resolved.

[alexander-yakushev]

Also, (cider-eval "(* 2 20)" (cider-repl-handler (current-buffer))) is instant.

[bbatsov]

A while back we reimplemented the interactive eval to use load-file instead of eval. I think this was about 2 months ago. Check out this discussion for more details. This is definitely not a problem on the JVM, no idea what's different on Android.

[alexander-yakushev]

Found the problem. Call to ns is expensive on Android. From the discussion you linked it isn't quite clear to me why the namespace declaration has to be re-evaluated each time. For example, call to in-ns is much cheaper.

[bbatsov]

From the discussion you linked it isn't quite clear to me why the namespace declaration has to be re-evaluated each time. For example, call to in-ns is much cheaper.

If we evaluate the entire ns definition the user doesn't have to eval the namespace form in advance - we're always certain that all the ns references in the code that is about to evaluated are resolved. Before this change was made people would usually run load-file before starting to evaluate code interactively.

Any idea why ns is so slow? Perhaps that's something we can fix/improve?

[alexander-yakushev]

Not sure, ns contains a lot of code. I guess refering clojure.core might be an issue. I thought if remembering whether we already executed ns might help, but if user extends ns declaration he will be forced to manually reevaluate it.

My best bet to solve this is to create a dedicated interactive-eval middleware op in cider-nrepl that separately takes form, form location, and ns form. The middleware reconstructs the file and calls load-file on it. This way we can avoid the \n madness on the client, and in Android projects I will be able to override the middleware from Clojure to make eval fast again.

[bbatsov]

My best bet to solve this is to create a dedicated interactive-eval middleware op in cider-nrepl that separately takes form, form location, and ns form. The middleware reconstructs the file and calls load-file on it. This way we can avoid the \n madness on the client, and in Android projects I will be able to override the middleware from Clojure to make eval fast again.

I've thought about this, but I have two problems with this approach:

• interactive eval is a pretty basic op and I'd prefer to have it available even without the middleware (although we can fallback to the existing code if the middleware op is not present)
• not sure how the load-file delegation will look like - if we fire the request from the Clojure code, the client won't know about it and won't know how to handle it (handlers in cider are associated with request ids). So, we'll probably have to copy the load-file middleware and make some adjustments to it. That's not a terrible approach, but it's not ideal either.

[alexander-yakushev]

interactive eval is a pretty basic op and I'd prefer to have it available even without the middleware (although we can fallback to the existing code if the middleware op is not present)

Agreed. But the point is, I cannot just override load-file middleware because it has a use on its own. So, this middleware handles two things instead of one — both loading files and interactive-eval stuff.
Fallback would probably be sensible. Without the middleware CIDER is already incredibly crippled, so using simple interruptible-eval instead wouldn't hurt much more.

So, we'll probably have to copy the load-file middleware and make some adjustments to it. That's not a terrible approach, but it's not ideal either.

It seems like we can directly use clojure.tools.nrepl.middleware.load-file/load-file-code in the custom smart-eval middleware, so not much has to be reimplemented. Also, like I said, cider-nrepl's custom middleware can also handle reconstructing the file (piling up newlines) for load-file so that client doesn't have to do it.

[bbatsov]

It seems like we can directly use clojure.tools.nrepl.middleware.load-file/load-file-code in the custom smart-eval middleware, so not much has to be reimplemented. Also, like I said, cider-nrepl's custom middleware can also handle reconstructing the file (piling up newlines) for load-file so that client doesn't have to do it.

That sounds good to me. I think, however, that we should alter the interruptible-eval middleware in nREPL itself, instead of doing it cider-nrepl. Adding a few optional params to the eval op won't break backward compatibility and will be beneficial for other users of nREPL. @cemerick what do you think about all this?

[bbatsov]

This is solved by the solution for #830.