Optimize namespace-aliases

vemv · vemv · commit 1399da2029c2 · 2021-09-17T12:04:39.000+02:00
Parallelizes it, and shrinks the corpus by removing non source/test dirs from the classpath.

Testing it over refactor-nrepl itself, I got a performance gain from 70ms (cached case) to 20ms (cached case).

The performance gains can be larger as projects scale. It also can plausibly ameliorate the mistery slowness for the :cljs branch.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,7 @@
   * This increases the chances that a namespace will be found, which in turns makes refactor-nrepl more complete/accurate.
 * Replace Cheshire with `clojure.data.json`
 * Build ASTs more robustly (by using locks, `require`, and ruling out certain namespaces like refactor-nrepl itself)
+* Improve `namespace-aliases` performance and make it return more accurate results.
 * Honor internal `future-cancel` calls, improving overall responsiveness and stability.
 
 ### Bugs fixed
diff --git a/src/refactor_nrepl/core.clj b/src/refactor_nrepl/core.clj
@@ -1,15 +1,17 @@
 (ns refactor-nrepl.core
-  (:require [clojure.java.io :as io]
-            [clojure.string :as str]
-            [clojure.tools.namespace.parse :as parse]
-            [clojure.tools.reader.reader-types :as readers]
-            [orchard.java.classpath :as cp]
-            [orchard.misc :as misc]
-            [me.raynes.fs :as fs]
-            [refactor-nrepl.util :as util :refer [normalize-to-unix-path]]
-            [refactor-nrepl.s-expressions :as sexp]
-            [refactor-nrepl.config :as config])
-  (:import [java.io File FileReader PushbackReader StringReader]))
+  (:require
+   [clojure.java.io :as io]
+   [clojure.string :as str]
+   [clojure.tools.namespace.parse :as parse]
+   [clojure.tools.reader.reader-types :as readers]
+   [me.raynes.fs :as fs]
+   [orchard.java.classpath :as cp]
+   [orchard.misc :as misc]
+   [refactor-nrepl.config :as config]
+   [refactor-nrepl.s-expressions :as sexp]
+   [refactor-nrepl.util :as util :refer [normalize-to-unix-path]])
+  (:import
+   (java.io File FileReader PushbackReader StringReader)))
 
 (defn version []
   (let [v (-> (or (io/resource "refactor-nrepl/refactor-nrepl/project.clj")
@@ -57,6 +59,18 @@
                 (when (.isDirectory ^File f) f)))
        (remove (comp ignore-dir-on-classpath? str))))
 
+(defn source-dirs-on-classpath
+  "Like `#'dirs-on-classpath`, but restricted to dirs that look like
+  (interesting) source/test dirs."
+  []
+  (->> (dirs-on-classpath)
+       (remove (fn [^File f]
+                 (let [s (-> f .toString)]
+                   (or (-> s (.contains "resources"))
+                       (-> s (.contains "target"))
+                       (-> s (.contains ".gitlibs"))))))
+       (remove util/dir-outside-root-dir?)))
+
 (defn project-root
   "Return the project root directory.
 
@@ -141,34 +155,32 @@
                              {:read-cond :allow :features #{dialect}})
          (catch Exception _ nil))))))
 
-(defn- data-file?
-  "True of f is named like a clj file but represents data.
-
-  E.g. true for data_readers.clj"
-  [path-or-file]
-  (let [path (.getPath (io/file path-or-file))
-        data-files #{"data_readers.clj" "project.clj" "boot.clj"}]
-    (reduce (fn [acc data-file] (or acc (.endsWith path data-file)))
-            false
-            data-files)))
+(defn cljc-extension? [^String path]
+  (.endsWith path ".cljc"))
 
 (defn cljc-file?
   [path-or-file]
   (let [path (.getPath (io/file path-or-file))]
-    (and (.endsWith path ".cljc")
+    (and (cljc-extension? path)
          (read-ns-form path))))
 
+(defn cljs-extension? [^String path]
+  (.endsWith path ".cljs"))
+
 (defn cljs-file?
   [path-or-file]
   (let [path (.getPath (io/file path-or-file))]
-    (and (.endsWith path ".cljs")
+    (and (cljs-extension? path)
          (read-ns-form path))))
 
+(defn clj-extension? [^String path]
+  (.endsWith path ".clj"))
+
 (defn clj-file?
   [path-or-file]
   (let [path (.getPath (io/file path-or-file))]
-    (and (not (data-file? path-or-file))
-         (.endsWith path ".clj")
+    (and (not (util/data-file? path-or-file))
+         (clj-extension? path)
          (read-ns-form path))))
 
 (defn source-file?
@@ -194,11 +206,29 @@
 
 (defn find-in-project
   "Return the files in the project satisfying (pred ^File file)."
-  [pred]
-  (->> (dirs-on-classpath)
-       (pmap (partial find-in-dir pred))
-       (apply concat)
-       distinct))
+  ([pred]
+   (find-in-project pred (dirs-on-classpath)))
+  ([pred dirs]
+   (->> dirs
+        (pmap (partial find-in-dir pred))
+        (apply concat)
+        distinct)))
+
+(defn source-files-with-clj-like-extension
+  "Finds files with .clj* extension in the project, without inspecting them.
+
+  Meant as a particularly fast operation (as it doesn't slurp files)."
+  ([ignore-errors?]
+   (source-files-with-clj-like-extension ignore-errors? (source-dirs-on-classpath)))
+  ([ignore-errors? dirs]
+   (find-in-project (util/with-suppressed-errors
+                      (comp (some-fn clj-extension?
+                                     cljc-extension?
+                                     cljs-extension?)
+                            (fn [^File f]
+                              (.getPath f)))
+                      ignore-errors?)
+                    dirs)))
 
 (defn throw-unless-clj-file [file-path]
   (when-not (re-matches #".+\.clj$" file-path)
diff --git a/src/refactor_nrepl/ns/libspecs.clj b/src/refactor_nrepl/ns/libspecs.clj
@@ -1,8 +1,10 @@
 (ns refactor-nrepl.ns.libspecs
-  (:require [refactor-nrepl.core :as core]
-            [refactor-nrepl.ns.ns-parser :as ns-parser]
-            [refactor-nrepl.util :as util])
-  (:import [java.io File]))
+  (:require
+   [refactor-nrepl.core :as core]
+   [refactor-nrepl.ns.ns-parser :as ns-parser]
+   [refactor-nrepl.util :as util])
+  (:import
+   (java.io File)))
 
 ;; The structure here is {path {lang [timestamp value]}}
 ;; where lang is either :clj or :cljs
@@ -43,23 +45,34 @@
     (put-cached-libspec f lang)))
 
 (defn namespace-aliases
-  "Return a map of file type to a map of aliases to namespaces
+  "Returns a map of file type to a map of aliases to namespaces
 
   {:clj {util com.acme.util str clojure.string
    :cljs {gstr goog.str}}}"
   ([]
    (namespace-aliases false))
   ([ignore-errors?]
-   {:clj  (->> (core/find-in-project (util/with-suppressed-errors
-                                       (some-fn core/clj-file? core/cljc-file?)
-                                       ignore-errors?))
-               (map (partial get-libspec-from-file-with-caching :clj))
-               aliases-by-frequencies)
-    :cljs (->> (core/find-in-project (util/with-suppressed-errors
-                                       (some-fn core/cljs-file? core/cljc-file?)
-                                       ignore-errors?))
-               (map (partial get-libspec-from-file-with-caching :cljs))
-               aliases-by-frequencies)}))
+   (namespace-aliases ignore-errors? (core/source-dirs-on-classpath)))
+  ([ignore-errors? dirs]
+   (let [;; fetch the file list just once (as opposed to traversing the project once for each dialect)
+         files (core/source-files-with-clj-like-extension ignore-errors? dirs)
+         ;; pmap parallelizes a couple things:
+         ;;   - `pred`, which is IO-intentive
+         ;;   - `aliases-by-frequencies`, which is moderately CPU-intensive
+         [clj-files cljs-files] (pmap (fn [[dialect pred] corpus]
+                                        (->> corpus
+                                             (filter pred)
+                                             (map (partial get-libspec-from-file-with-caching dialect))
+                                             aliases-by-frequencies))
+                                      [[:clj (util/with-suppressed-errors
+                                               (some-fn core/clj-file? core/cljc-file?)
+                                               ignore-errors?)]
+                                       [:cljs (util/with-suppressed-errors
+                                                (some-fn core/cljs-file? core/cljc-file?)
+                                                ignore-errors?)]]
+                                      (repeat files))]
+     {:clj  clj-files
+      :cljs cljs-files})))
 
 (defn- unwrap-refer
   [file {:keys [ns refer]}]
diff --git a/src/refactor_nrepl/util.clj b/src/refactor_nrepl/util.clj
@@ -1,6 +1,10 @@
 (ns refactor-nrepl.util
-  (:require [clojure.string :as string])
-  (:import java.util.regex.Pattern))
+  (:require
+   [clojure.java.io :as io]
+   [clojure.string :as string])
+  (:import
+   (java.io File)
+   (java.util.regex Pattern)))
 
 (defn normalize-to-unix-path
   "Replace use / as separator and lower-case."
@@ -99,3 +103,34 @@
   ;; in other places that already are using `some-fn`, `every-pred`, etc
   ([_]
    (.isInterrupted (Thread/currentThread))))
+
+(defn dir-outside-root-dir?
+  "Dirs outside the root dir often represent uninteresting dirs to examine, e.g.
+  Lein checkouts, .gitlibs from deps.edn, or other extraneous source paths
+  which aren't directly related to the project.
+
+  By excluding them, one gets better and more accurate performance."
+  [^File f]
+  {:pre [(-> f .isDirectory)]}
+  (let [f (-> f .getCanonicalPath File.)
+        root-dir (File. (System/getProperty "user.dir"))
+        parent-dirs (->> f
+                         (iterate (fn [^File f]
+                                    (some-> f .getCanonicalPath File. .getParent File.)))
+                         (take-while some?)
+                         (set))]
+    (not (parent-dirs root-dir))))
+
+(defn data-file?
+  "True of f is named like a clj file but represents data.
+
+  E.g. true for data_readers.clj"
+  [path-or-file]
+  (let [path (.getPath (io/file path-or-file))
+        data-files #{"data_readers.clj" "project.clj" "boot.clj"}]
+    (reduce (fn [acc data-file]
+              (let [v (or acc (.endsWith path data-file))]
+                (cond-> v
+                  v reduced)))
+            false
+            data-files)))
diff --git a/test/refactor_nrepl/core_test.clj b/test/refactor_nrepl/core_test.clj
@@ -2,14 +2,14 @@
   (:require
    [clojure.test :refer [are deftest is testing]]
    [refactor-nrepl.config :as config]
-   [refactor-nrepl.core :refer [ignore-dir-on-classpath? read-ns-form]])
+   [refactor-nrepl.core :as sut])
   (:import
    (java.io File)))
 
 (defmacro assert-ignored-paths
   [paths pred]
   `(doseq [p# ~paths]
-     (is (~pred (ignore-dir-on-classpath? p#)))))
+     (is (~pred (sut/ignore-dir-on-classpath? p#)))))
 
 (deftest test-ignore-dir-on-classpath?
   (let [not-ignored ["/home/user/project/test"
@@ -32,9 +32,16 @@
   (are [input expected] (testing input
                           (assert (-> input File. .exists))
                           (is (= expected
-                                 (read-ns-form input)))
+                                 (sut/read-ns-form input)))
                           true)
     "test-resources/readable_file_incorrect_aliases.clj" nil
     "testproject/src/com/example/one.clj"                '(ns com.example.one
                                                             (:require [com.example.two :as two :refer [foo]]
                                                                       [com.example.four :as four]))))
+
+(deftest source-files-with-clj-like-extension-test
+  (let [result (sut/source-files-with-clj-like-extension true)]
+    (doseq [extension [".clj" ".cljs" ".cljc"]]
+      (is (pos? (count (filter (fn [^File f]
+                                 (-> f .getPath (.endsWith extension)))
+                               result)))))))
diff --git a/test/refactor_nrepl/ns/namespace_aliases_test.clj b/test/refactor_nrepl/ns/namespace_aliases_test.clj
@@ -1,12 +1,13 @@
 (ns refactor-nrepl.ns.namespace-aliases-test
-  (:require [clojure.test :refer [deftest is]]
-            [refactor-nrepl.core :as core]
-            [refactor-nrepl.ns.libspecs :as sut]
-            [refactor-nrepl.util :as util]
-            [refactor-nrepl.unreadable-files :refer [ignore-errors?]]))
+  (:require
+   [clojure.test :refer [deftest is]]
+   [refactor-nrepl.core :as core]
+   [refactor-nrepl.ns.libspecs :as sut]
+   [refactor-nrepl.unreadable-files :refer [ignore-errors?]]
+   [refactor-nrepl.util :as util]))
 
 (defn finds [selector alias libspec]
-  (let [aliases (selector (sut/namespace-aliases ignore-errors?))]
+  (let [aliases (selector (sut/namespace-aliases ignore-errors? (core/dirs-on-classpath)))]
     (some (fn [[k vs]]
             (and (= k alias)
                  (some #{libspec} vs)))
diff --git a/test/refactor_nrepl/util_test.clj b/test/refactor_nrepl/util_test.clj
@@ -1,6 +1,9 @@
 (ns refactor-nrepl.util-test
-  (:require [clojure.test :refer [deftest is]]
-            [refactor-nrepl.util :as sut]))
+  (:require
+   [clojure.test :refer [are deftest is]]
+   [refactor-nrepl.util :as sut])
+  (:import
+   (java.io File)))
 
 (deftest with-additional-ex-data-test
   (try
@@ -10,3 +13,22 @@
     (catch clojure.lang.ExceptionInfo e
       (let [{:keys [foo]} (ex-data e)]
         (is (= foo :bar))))))
+
+(deftest dir-outside-root-dir?-test
+  (are [input expected] (= expected
+                           (sut/dir-outside-root-dir? input))
+    (File. (System/getProperty "user.dir")) false
+    (File. ".")                             false
+    (File. "src")                           false
+    (File. "/")                             true))
+
+(deftest data-file?-test
+  (are [input expected] (= expected
+                           (sut/data-file? input))
+    "project.clj"       true
+    "boot.clj"          true
+    "data_readers.clj"  true
+    "project.cljs"      false
+    "boot.cljs"         false
+    "data_readers.cljs" false
+    "a.clj"             false))
