-
Notifications
You must be signed in to change notification settings - Fork 69
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
delay loading of middleware to speed up startup #198
Conversation
this deffo looks interesting. thanks for investing the energy. give me a bit more time though to have a proper look. |
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 I've thought about writing a 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 |
This looks good to me.
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 :) |
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))) |
There was a problem hiding this comment.
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
thanks for spotting this. fortunately have not published a new snapshot yet. will check features locally before a I do. |
- fix typo, thx @vmfhrmfoaj - fix deref fails in threading macro
new snapshot is on clojars. sry for the delay |
@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. |
yes, you are right. will try to find time to look into this. perhaps worth cutting a release too |
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 ;) |
@benedekfazekas This is the relevant commit clojure-emacs/cider-nrepl@48dc5d2 |
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. |
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):
lein do clean, test
)./build.sh install
-- takes a long time)Thanks!