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

delay loading of middleware to speed up startup #198

Merged
merged 1 commit into from Jul 10, 2017

Conversation

ryfow
Copy link
Contributor

@ryfow ryfow commented Jun 24, 2017

There is probably a more elegant way to delay code loading, but this is the most obvious way I could think of.

These changes drop somewhere between 3 and 5 seconds off of repl startup time for me. Tests pass, but I only did a test of a couple features in emacs by hand. I don't actually use refactor-nrepl very much, so I'm not particularly familiar with how people use it.

clojure-emacs/cider#1717

Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (run lein do clean, test)
  • Code inlining with mranderson works and tests pass with inlined code (run ./build.sh install -- takes a long time)
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)

Thanks!

@benedekfazekas
Copy link
Member

this deffo looks interesting. thanks for investing the energy. give me a bit more time though to have a proper look.

@ryfow
Copy link
Contributor Author

ryfow commented Jun 28, 2017

For sure. There are a variety of ways to crack this nut, and I'm quite certain that it's worth cracking. I'd love to get cider-jack-in to start up faster. cider-nrepl recently merged some similar commits related to cljfmt, fipp and puget being slow to load.

I've thought about writing a delayed-refer macro that takes arguments similar to (:require) and produces functions that do basically what require-and-resolve does. It'd basically hide the fact that (delay) is being used under the covers. If you'd use something like that, I'd be willing to write it. Let me know if there's some other route you're interested in.

I'm not sure what other people are using for profiling load-time related stuff, but I did publish the tool I've been using. It might help you see how I got to these changes. https://github.com/ryfow/flame-du-jour

@expez
Copy link
Member

expez commented Jun 29, 2017

This looks good to me.

I've thought about writing a delayed-refer macro that takes arguments similar to (:require) and produces functions that do basically what require-and-resolve does.

Initially I was a bit put-off by having to do this for every function in the middleware ns, but this is probably the namespace we visit the least (as it's only really edited when we add new ops).

If @benedekfazekas signs off, I'm cool with merging this without spending more time on bikeshedding to make it pretty :)

@benedekfazekas benedekfazekas merged commit bebdee6 into clojure-emacs:master Jul 10, 2017
@benedekfazekas
Copy link
Member

sorry for the delay (pun intended). LGTM


(defn- version-reply [{:keys [transport] :as msg}]
(reply transport msg :status :done :version (core/version)))

(def ^:private warm-ast-cache
(delay
(require-and-resolve 'refactor.nrepl.analyzer/warm-ast-cache)))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is typo error.

refactor.nrepl.analzyer => refactor-nrepl.analzyer

@benedekfazekas
Copy link
Member

thanks for spotting this. fortunately have not published a new snapshot yet. will check features locally before a I do.

benedekfazekas pushed a commit that referenced this pull request Jul 16, 2017
- fix typo, thx @vmfhrmfoaj
- fix deref fails in threading macro
@benedekfazekas
Copy link
Member

benedekfazekas commented Jul 23, 2017

new snapshot is on clojars. sry for the delay

@bbatsov
Copy link
Member

bbatsov commented Oct 2, 2017

@benedekfazekas Wouldn't it be better to leverage the new API introduced by @vspinu in 0.6? I think the resulting code is cleaner and it'd be good if there's consistency between cider-nrepl and refactor-nrepl.

@benedekfazekas
Copy link
Member

yes, you are right. will try to find time to look into this. perhaps worth cutting a release too

@benedekfazekas
Copy link
Member

had a really quick look of commits/PRs around this. not sure what is the API and 0.6 you are referring to @bbatsov ... any pointer would be appreciated ;)

@bbatsov
Copy link
Member

bbatsov commented Oct 2, 2017

@benedekfazekas This is the relevant commit clojure-emacs/cider-nrepl@48dc5d2

@bbatsov
Copy link
Member

bbatsov commented Oct 2, 2017

The API in effect is two macros - clojure-emacs/cider-nrepl@48dc5d2#diff-4bbaa7a85d7bf72817b39b7a0452456fR17 and the one that follows.

The important thing is to just move all the middleware registration to the same file just as we did in this commit, to allow for them to be known but dynamically loaded.

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 this pull request may close these issues.

None yet

5 participants