cljr-slash: detect when the input is an existing namespace

vemv commented 3 years ago

Context

https://github.com/clojure-emacs/clj-refactor.el/blob/466822ff6f9da584f7cf72c868017b8840574dbd/clj-refactor.el#L1928 is an expensive call, even when refactor-nrepl has caching and parallelism for it. Especially as project sizes grow.

If for the input e.g. next.jdbc/, we determine next.jdbc is a already a namespace, then we can consider that expensive call to be skippable. e.g we can check if (find-ns 'next.jdbc) succeeds, and skip computations accordingly, because since next.jdbc is itself a ns, we can infer it's not an alias and therefore doesn't need project aliases to be queried.

It is annoying to have cljr-slash introduce a delay even when it will not contribute something useful.

Considerations

It is possible that next.jdbc could be an alias used somewhere: com.something.else :as next.jdbc. That's a pretty bad thing to do, so I'd favor a fast experience over accuracy for that edge case .

Open questions

Can we make clj-refactor.el perform a find-ns call? It would seem a nice self-contained way of achieving this. I see it currently calls (cider* ...) in a few occasions.

An alternative could be to make namespace-aliases accept some extra arguments e.g. the user input. Then refactor-nrepl could perform the find-ns itself, contextually.

expez commented 3 years ago

Have you profiled the call to cljr-slash? Using the profiler is really easy.

One thing that might make this op slow is if you have a bajillion aliases in your project. Maybe the middleware is super fast but emacs is spending a lot of time parsing the result?

In any event, by sending down the thing before the / to the middleware (along with the ns/file we're in?) it could return a more accurate result, faster, instead of sending up everything it knows about clj and cljs aliases and letting the client sort it out.

Sounds like we might benefit from introducing a more focused op 👍

vemv commented 3 years ago

I timed (refactor-nrepl.ns.libspecs/namespace-aliases) now, thanks for the suggestion in that direction!

So I noticed two things in https://github.com/clojure-emacs/refactor-nrepl/blob/1b646079ef3e3c4438c3eeb20514e4d2731f444e/src/refactor_nrepl/ns/libspecs.clj#L53-L62 :

the main slowness had to do with a personal hack I was applying on top of refactor-nrepl
- sorry for that!
Still, consecutive calls to (refactor-nrepl.ns.libspecs/namespace-aliases) could be a little faster (from 75ms on a medium-sized project to hopefully ~half), I noticed a couple optimizations there:
- core/find-in-project is performed twice, e.g. we're traversing the project twice. Probably we can consolidate some work across clj and cljs.
- we shouldn't traverse folders like target/and ~/.gitlibs (i.e. non-source folders present in the classpath) in search of .clj / .cljs namespaces. These just make the search slower and can yield false positives. Might be the main culprit for :cljs slowness, who knows.
- same for lein checkouts.

So, I'll close this issue as it was kind of misguided, but I have found other optimizations that should serve large/cljs projects and aide correctness in general.

vemv commented 3 years ago

To be clearer about my reasoning, In theory https://github.com/clojure-emacs/clj-refactor.el/issues/494 is still valid as-is, for example if I type next.jdbc/ traversing the whole project is a bit wasteful, even if it was super-fast (probably it will take ~20ms after optimizing it, depending on project size).

i.e. we're hitting the SSD, I think it causes it to wear a little bit.

But that can be solved at a later moment, it's bit of an edge case and I'm not a fan of complicating the Elisp 😄

clojure-emacs / clj-refactor.el