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

Parallelize referred-syms-by-file&fullname #320

Merged
merged 2 commits into from
Jul 9, 2021

Conversation

vemv
Copy link
Member

@vemv vemv commented Jul 8, 2021

Part of #230

Parallelizes referred-syms-by-file&fullname and core/ functions used by referred-syms-by-file&fullname.

(the core/ parallelization will be only noticable in large projects)

One can verify the improvement with the following repl input over refactor-nrepl itself:

(require 'refactor-nrepl.ns.libspecs)
(in-ns 'refactor-nrepl.ns.libspecs)
(time (referred-syms-by-file&fullname true))

...It's clearly better than master performance for both the non-cached and cached (i.e. subsequent invocations) cases.

(the numbers of course will depend on your CPU count)

@@ -184,7 +188,10 @@
(defn find-in-project
"Return the files in the project satisfying (pred ^File file)."
[pred]
(-> find-in-dir (partial pred) (mapcat (dirs-on-classpath)) distinct))
(->> (dirs-on-classpath)
(pmap (partial find-in-dir pred))
Copy link
Member Author

Choose a reason for hiding this comment

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

I chose pmap for simplicity, it's better than some tend to say

(because pmap won't create one thread per item... it takes the CPU count in account)

...it would be inefficient for very small inputs, but it's safe to say that most projects have multiple dirs in the classpath (especially in monorepos), plenty of namespaces, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Good choice! The small workloads that will get slightly slower will still be more than fast enough in the face of some fork/join overhead, I think :)

@@ -88,10 +88,10 @@
{:clj (->> (core/find-in-project (util/with-suppressed-errors
(some-fn core/clj-file? core/cljc-file?)
ignore-errors?))
(map (juxt identity (partial get-libspec-from-file-with-caching :clj)))
(pmap (juxt identity (partial get-libspec-from-file-with-caching :clj)))
Copy link
Member

Choose a reason for hiding this comment

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

I'd add a note about the use of pmap, as people tend to "optimize" it by removing it. :D

CHANGELOG.md Outdated
@@ -3,6 +3,8 @@
## Unreleased

#### Changes
* [(Part of #230)](https://github.com/clojure-emacs/refactor-nrepl/issues/230) Parallelize various fuctionality
Copy link
Member

Choose a reason for hiding this comment

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

One blank line is missing. And a :.

Copy link
Member

@expez expez left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -184,7 +188,10 @@
(defn find-in-project
"Return the files in the project satisfying (pred ^File file)."
[pred]
(-> find-in-dir (partial pred) (mapcat (dirs-on-classpath)) distinct))
(->> (dirs-on-classpath)
(pmap (partial find-in-dir pred))
Copy link
Member

Choose a reason for hiding this comment

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

Good choice! The small workloads that will get slightly slower will still be more than fast enough in the face of some fork/join overhead, I think :)

@vemv vemv force-pushed the 230--parallelize-namespace-aliases branch from 932cd34 to 02bc6d3 Compare July 9, 2021 14:14
@vemv
Copy link
Member Author

vemv commented Jul 9, 2021

🍻

@vemv vemv merged commit ba5382b into master Jul 9, 2021
@vemv vemv deleted the 230--parallelize-namespace-aliases branch July 9, 2021 14:44
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.

3 participants