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

Dynamically load middleware #438

Merged
merged 3 commits into from Sep 16, 2017

Conversation

@vspinu
Copy link
Contributor

vspinu commented Aug 21, 2017

Addresses #218 and clojure-emacs/cider#1717.

It now takes only 0.25s for me to load cider.nrepl.clj. So startup time is now compatible with lein repl with an empty project. Some extra work could be done on emacs side to avoid loading extra things (like debugger) on startup.

I will pile a couple more related commits here in the following days to further fix loading of the debugg.clj, error_handling.clj and maybe something else.

@vspinu vspinu force-pushed the vspinu:dynaload branch from e0c964e to 934a70c Aug 21, 2017
@vspinu

This comment has been minimized.

Copy link
Contributor Author

vspinu commented Aug 21, 2017

Current flame graph. No cider middleware there except the very light pprint and version.

I am done here. Not optimizing the debugger in any specific way as it loads too many heavy things, and should be mostly dealt from emacs side anyway.

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Aug 22, 2017

Hmm, that was one unexpected change. I'll try to take a look at the code later today. I was under the impression dynamic middleware loading was problematic and I'm curious to see what approach did you take.

@vspinu vspinu force-pushed the vspinu:dynaload branch 4 times, most recently from 88aeaf9 to 055d282 Aug 22, 2017
@vspinu vspinu force-pushed the vspinu:dynaload branch from 055d282 to e0e2155 Aug 22, 2017
@xiongtx

This comment has been minimized.

Copy link
Member

xiongtx commented Sep 6, 2017

@bbatsov Any update here? Seems like a real quality-of-life improvement if it could be merged.

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Sep 6, 2017

@bbatsov Any update here? Seems like a real quality-of-life improvement if it could be merged.

It's a huge change and I haven't had time to carefully look into it. I don't make big changes lightly because there might be all sorts of unforeseen implications. It seems that no one else has looked into the PR as well, looking at the feedback it has gathered.

@xiongtx

This comment has been minimized.

Copy link
Member

xiongtx commented Sep 6, 2017

It seems that no one else has looked into the PR as well, looking at the feedback it has gathered.

Touché 😉

@Malabarba

This comment has been minimized.

Copy link
Member

Malabarba commented Sep 6, 2017

I agree with Bozhidar.
If it works a advertised I think it's an awesome improvement and I'm eager to get it out there. I'll try to give it some due attention soon.

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Sep 7, 2017

So, I finally had the time to look more closely into this and here are my main concerns:

  • we split the "actual" middleware definition from the implementation which makes the code a bit harder to read. I understand why this is necessary, but I don't like it much.
  • all the "on-demand" middleware definitions are dumped in a single file together with some utility functions, which I don't like at all
  • we've dispensed completely with the regular definitions of the middlewares - perhaps things should have a "standard" definition and this lazy one, just in case someone runs into problems with the lazy loading
  • trivial middleware without big dependencies doesn't really benefit from this, but I see all the middleware has been deferred
  • I think it's strange that we now end up having something that's not really the middleware under cider.nrepl.middleware

In general I really wonder if it's our place to jump over such hoops to make deferred middleware loading a reality. Seems to me something like this should be part of the core nREPL functionality. I'd love to hear from @cemerick on this.

Anyways, let's see what @Malabarba thinks about those changes as well. They are certainly pretty useful, but come at the cost of more complicated codebase. For me the slow startup times of the REPL are not that big of an issue as you don't really restart it that often so I wonder how far should we be willing to go to defer the loading of the middleware.

P.S. To be clear - I'm not opposed to merging this, but I think there's room for improvement at this point.

[cider.nrepl.middleware.version]
[cider.nrepl.print-method]))

(def DELAYS (atom nil))

This comment has been minimized.

Copy link
@bbatsov

bbatsov Sep 7, 2017

Member

That's one extremely obsure name, without a docstring.


(defmacro ^{:arglists '([name handler-fn descriptor]
[name handler-fn handle descriptor])}
def-wrapper

This comment has been minimized.

Copy link
@bbatsov

bbatsov Sep 7, 2017

Member

Probably def-middleware(-wrapper) or def-nrepl-wrapper would be a better name for this.

cider.nrepl.middleware.ns/wrap-ns
cider.nrepl.middleware.spec/wrap-spec
cider.nrepl.middleware.out/wrap-out
:dev {:repl-options {:nrepl-middleware [cider.nrepl/wrap-apropos

This comment has been minimized.

Copy link
@bbatsov

bbatsov Sep 7, 2017

Member

I definitely don't like that all the middleware wrappers ended up being in some utility namespace.

(or (resolve sym)
(throw (IllegalArgumentException. (format "Cannot resolve %s" sym)))))

(defmacro run-delayed-handler

This comment has been minimized.

Copy link
@bbatsov

bbatsov Sep 7, 2017

Member

I'm not sure run-delayed-handler is the most appropriate name for this.

This comment has been minimized.

Copy link
@vspinu

vspinu Sep 10, 2017

Author Contributor

This macro does a bunch of things at compile time, but the name is highly suggestive for what happens at run-time. Have a look at the usage. I find it ok, but wouldn't mind a better name though.


(defmacro run-delayed-handler
"Make a delay of `fn-name` and place it in `DELAYS` atom at compile time.
Require and invoke the delay at run-time with arguments h and msg."

This comment has been minimized.

Copy link
@bbatsov

bbatsov Sep 7, 2017

Member

What are h and msg? 😃

This comment has been minimized.

Copy link
@vspinu

vspinu Sep 10, 2017

Author Contributor

This is an internal function. Definition of h and msg are in def-wrapper, if not it should be improved. Will smooth this out.


;;; Wrappers

(def-wrapper wrap-debug cider.nrepl.middleware.debug/handle-debug

This comment has been minimized.

Copy link
@bbatsov

bbatsov Sep 7, 2017

Member

I don't like that all the middleware definitions are dumped in a file used for utility functions.

This comment has been minimized.

Copy link
@vspinu

vspinu Sep 10, 2017

Author Contributor

Not sure I understand. That is a declaration of the middleware files. We can rename it, but i think nrepl.clj is good enough. What utility functions? There are no other utilities there except def-wrapper which is used for defining wrappers.

This comment has been minimized.

Copy link
@bbatsov

bbatsov Sep 16, 2017

Member

My bad. I just saw what the file originally just defined the default handler.

{:doc "Provide instrumentation and debugging functionality."
:expects #{"eval"}
:requires #{#'pprint/wrap-pprint-fn #'session}
:handles {"debug-input"

This comment has been minimized.

Copy link
@bbatsov

bbatsov Sep 7, 2017

Member

I wonder if those definitions should mark just the ops something handle and obtain the full metadata from the actual middleware descriptor which would be defined as usual. Overall I really hate that the middleware implementation and definition are split and as I result you have to jump to two places to figure out how something is supposed to behave.

This comment has been minimized.

Copy link
@vspinu

vspinu Sep 10, 2017

Author Contributor

I wonder if those definitions should mark just the ops something handle and obtain the full metadata from the actual middleware descriptor which would be defined as usual.

This is just not possible. See my main answer. In order to get the metadata of something, you need to load the file where that "something" is declared.

Overall I really hate that the middleware implementation and definition are split and as I result you have to jump to two places to figure out how something is supposed to behave.

This is not as bad as you make it sound. Logic is in one place, exposed API declaration is in another place. It's actually a plus IMO.

This comment has been minimized.

Copy link
@bbatsov

bbatsov Sep 16, 2017

Member

Well, those things are always subjective. 😃 Anyways, I can live with this for now. Unfortunately I don't really have time to work on this and I'm not the type of person who'd just complain something instead of do it better themselves.

@@ -41,7 +41,7 @@
and may be case senstive. Types returned correspond to Apropos types.
Docstring search returns the full doc; symbol search returns an abbreviated
version."
[ns query search-ns docs? privates? case-sensitive? filter-regexps]
[{:keys [ns query search-ns docs? privates? case-sensitive? filter-regexps]}]

This comment has been minimized.

Copy link
@bbatsov

bbatsov Sep 7, 2017

Member

I saw this change at few places and it seems somewhat unrelated, so it should probably be in a separate commit for clarity's sake.

This comment has been minimized.

Copy link
@vspinu

vspinu Sep 10, 2017

Author Contributor

I don't remember exactly anymore, but I am pretty confident that this change is needed to make the handle-apropos behave as all other handle-xyz functions (i.e. take in two args - an nrepl handler and a map msg).

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Sep 7, 2017

I've added a few remarks inline.

Also - this PR should probably undo the delayed loading of deps done by some previous PRs.

@vspinu

This comment has been minimized.

Copy link
Contributor Author

vspinu commented Sep 10, 2017

  • we split the "actual" middleware definition from the implementation which
    makes the code a bit harder to read.
  • all the "on-demand" middleware definitions are dumped in a single file
    together with some utility functions
  • I think it's strange that we now end up having something that's not really
    the middleware under cider.nrepl.middleware

These are stylistic concerns and I think we should not put too much weight on them in order to decide on this.

There are some notable benefits of the proposed organization as well:

  • All middleware is declared in an uniform style in one place. This would be quite useful for anyone studing or perusing cider's code base. I never had a good view of the entire cider-nrepl functionality before I made this change. With this change, all of the cider-nrepl's api is in one 100 line declaration file. Cannot be nicer ;)

  • The fact that wrapping is separated from actual logic means that one can tweak on cider code without being concerned with how nrepl works or what middlware is. Those internals being transparently handled in the background. - With this change you can happily debug all of the logic. Right now after each time modification to the middlware declaration you need to restart cider session to see an effect.

  • Splitting declarations from main logic is nothing new. C/C++ header files or Emacs auto-load files are examples. In fact, if you want to load a file dynamically you need to have such a declaration file anyways. So the proposed solution, or a variant thereof, is actually the only way to go.

  • we've dispensed completely with the regular definitions of the middlewares -
    perhaps things should have a "standard" definition and this lazy one, just
    in case someone runs into problems with the lazy loading

No objection, but it should be part of the same global "middleware" declaration file (see below).

  • trivial middleware without big dependencies doesn't really benefit from this,
    but I see all the middleware has been deferred

Not all at the moment but almost all. I think all of them should be actually declared for organizational reasons - either all or nothing IMO. Otherwise it would create extra layeir of complexity. Each time you would have to decide whther to auto-load or not. Future changes to middleware might introduce heavy changes. External users and developers of the middlware have to deal with different naming conventions of the middleware.

In general I really wonder if it's our place to jump over such hoops to make
deferred middleware loading a reality.

I consider this a pretty straightforward change, so I frankly don't see any major "hoops". I am actually pretty surprised that you see this as "complicated".

Seems to me something like this should be part of the core nREPL functionality.

I am afraid that won't solve your stylistic concerns. Even if nrepl itself comes with some kind of solution you will still need to declare something in a separate file from the main logic. You cannot load part of the file and later load the rest. These are irreconcilable targets.

but I think there's room for improvement at this point.

Adding the following is probably a good idea:

  • Name middleware with corresponding prefix. That is, the def-wrapper macro can create the middleware ns and place the wrapper there.

  • def-wrapper can define two wrappers one dynamic and one standard. This would be backward compatible change. Folks who place cider middleware by hand in nrepl-options won't see it breaking.

@cemerick

This comment has been minimized.

Copy link

cemerick commented Sep 14, 2017

Just to define some terms as I'll use them (and as they've been used by a couple of others that've worked on things similar to this):

  • deferred loading is where portions of a middleware stack are activated either via explicit demand, or, as here, based on some ambient criteria (e.g. receiving the first message of a particular type)
  • dynamic loading is where the middleware stack can be altered at any time, including adding, removing, and reordering it to satisfy user/client demands.

This PR looks to accomplish the former for a cider stack.

To answer @bbatsov's question, dynamic loading will definitely be part of nREPL. Phil has worked on this more than I have and has a couple of concrete proposals, so the particular approach isn't set in stone, but the result would also satisfy the objectives of any deferred-loading change. (Since a dynamically-loaded middleware wouldn't be required at all until a client/user requested it.)

To be clear, I am not saying "don't merge this". If loading the code that supports cider's middleware stack is determined to be a serious UX problem for the community here, then godspeed.

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Sep 16, 2017

Splitting declarations from main logic is nothing new. C/C++ header files or Emacs auto-load files are examples. In fact, if you want to load a file dynamically you need to have such a declaration file anyways. So the proposed solution, or a variant thereof, is actually the only way to go.

Unless you generate this during say build time of the artifact. :-) There's always more than one way, but this is definitely the simplest one - I'm not arguing with this.

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Sep 16, 2017

@cemerick Thanks for your input!

To answer @bbatsov's question, dynamic loading will definitely be part of nREPL. Phil has worked on this more than I have and has a couple of concrete proposals, so the particular approach isn't set in stone, but the result would also satisfy the objectives of any deferred-loading change. (Since a dynamically-loaded middleware wouldn't be required at all until a client/user requested it.)

Looking forward to this!

In the mean time I'll merge this as I believe the benefits outweigh the drawbacks. Let's see if this is going to cause any practical issues for our users.

@benedekfazekas @expez I think you should switch to this same style for refactor-nrepl, otherwise we'd not be solving the slow startup for everyone.

@bbatsov bbatsov merged commit 4e23821 into clojure-emacs:master Sep 16, 2017
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Sep 16, 2017

@vspinu Thanks for another great contribution!

@vspinu

This comment has been minimized.

Copy link
Contributor Author

vspinu commented Sep 17, 2017

Ohh. I though there is some more work to be done here. Particularly:

  • Name middleware with corresponding prefix. That is, the def-wrapper macro can create the middleware ns and place the wrapper there.
  • def-wrapper can define two wrappers one dynamic and one standard. This would be a backward compatible change. Folks who place cider middleware by hand in nrepl-options won't see it breaking.

Do you have an opinion on this?

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Sep 17, 2017

I like both ideas!

I haven't actually pushed a new build out, so the end users won't see the impact of this commit for a while.

@alexander-yakushev

This comment has been minimized.

Copy link
Member

alexander-yakushev commented Oct 1, 2017

The presence of refactor-nrepl still slows down the loading considerably.

Tested with [cider/cider-nrepl "0.16.0-SNAPSHOT"] (as of 2017-10-01).

With refactor-nrepl: 44.15s user 1.76s system 247% cpu 18.536 total
Without refactor-nrepl: 14.79s user 0.74s system 264% cpu 5.876 total

@bbatsov @vspinu

@benedekfazekas

This comment has been minimized.

Copy link
Member

benedekfazekas commented Oct 1, 2017

what is the cljr version you are testing with? try with latest snapshot in case you are using an older version there was a PR merged not long ago with delayed loading of some nses...

@alexander-yakushev

This comment has been minimized.

Copy link
Member

alexander-yakushev commented Oct 1, 2017

Yeah, much better with 2.4.0-SNAPSHOT! Thank you!

23.96s user 0.85s system 250% cpu 9.885 total

@benedekfazekas

This comment has been minimized.

Copy link
Member

benedekfazekas commented Oct 1, 2017

no worries although I only merged that PR. so thanks @ryfow

@vspinu

This comment has been minimized.

Copy link
Contributor Author

vspinu commented Oct 2, 2017

Without refactor-nrepl: 14.79s user 0.74s system 264% cpu 5.876 total

Is this a slow machine or a big project? On my i7 laptop it takes 4s with no project and 5-6 sec on cider-nrepl itself.

How long does it take for the plain lein repl to start in your project? With this PR and newest CIDER there should be barely any difference.

@alexander-yakushev

This comment has been minimized.

Copy link
Member

alexander-yakushev commented Oct 2, 2017

5.876 total

This is the actual (perceived) time.

I don't use Leiningen much, but in Boot with or without the newest cider-nrepl the time is almost the same.

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Oct 3, 2017

I don't use Leiningen much, but in Boot with or without the newest cider-nrepl the time is almost the same.

That's super odd. What's the time in boot with and without cider-nrepl?

@alexander-yakushev

This comment has been minimized.

Copy link
Member

alexander-yakushev commented Oct 3, 2017

@bbatsov Sorry, I must have confused you. The startup time with the latest cider-nrepl and without any cider-nrepl at all is (almost) the same. This is what is expected (and awesome)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.