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

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

Closed
vemv opened this issue Sep 16, 2021 · 3 comments
Closed

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

vemv opened this issue Sep 16, 2021 · 3 comments

Comments

@vemv
Copy link
Member

vemv commented Sep 16, 2021

Related to #492 but would not be fixed by #493

Context

(when-let (aliases (cljr--call-middleware-for-namespace-aliases))
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
Copy link
Member

expez commented Sep 16, 2021

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
Copy link
Member Author

vemv commented Sep 16, 2021

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 vemv closed this as completed Sep 16, 2021
@vemv
Copy link
Member Author

vemv commented Sep 16, 2021

To be clearer about my reasoning, In theory #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 😄

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

No branches or pull requests

2 participants