From 086168717f6df048a800aabc7eac68adb0edb951 Mon Sep 17 00:00:00 2001 From: Arne Brasseur Date: Sat, 18 Sep 2021 10:43:21 +0200 Subject: [PATCH] Remove costly io/resource lookup The `immutable-source-file?` check is used in `class-info` to know if it makes sense to invalidate a cache entry based on modification time. This check uses the `:file` it is given to see if the resource is inside a jar or not. This `:path` comes from `orchard.java.{parser,legacy-parser}`, and is the relative path on the classpath. This is then looked up via `io/resource`, so that the URL can be checked to see if it's a jar or not. This resource check is expensive, depending on circumstances cider-nrepl's "stacktrace" analysis can get extremely slow, around 10~20 seconds to analyze all stack frames and return a result, which means that the error buffer will only pop up 10~20 seconds after evaluating a form which causes an error. The check is performed whether we have a cached result or not. However java.parser already calls `io/resource` to resolve the full resource URL, so we should be able to use the information we already have. However, the way java.parser was doing this caused inconsistencies between filesystem resources vs archived resources. It does a `(.getPath (io/resource path))`, and stores that under `:file`. This works for regular files, because `.getPath` on a `file:` URL returns the path of the file. For `jar:` URLs it returns the nested url+the jar entry, so a `:file` URL but with a dangling `!/jar/entry`. In other words the `:file` entry is either an actual file path, or something that looks like a `file:` URL but isn't. This patch - Makes the result from java.parser consistent: `:path` is a relative classpath resource path, `:file` is a file on the filesystem path. Either it points at the resource, or at the archive containing the resource. - Add an `archive?` flag to the result of `source-info`, so we don't have to figure out again later if it's an archived resource or not - Changes the check in `class-info` to be a cheap lookup of two map keys. If it's an archive or if we don't have a filesystem path, then don't bother checking the lastModified time. Illustration of why calling `getPath` on `jar:` URLs is not useful: ```clojure (io/resource "lambdaisland/witchcraft.clj") ;;=> #java.net.URL "file:/srv/mc/witchcraft/src/lambdaisland/witchcraft.clj" (.getPath (io/resource "lambdaisland/witchcraft.clj")) ;;=> "/srv/mc/witchcraft/src/lambdaisland/witchcraft.clj" (io/resource "clojure/lang/RT.class") ;;=> #java.net.URL "jar:file:/root/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar!/clojure/lang/RT.class" (.getPath (io/resource "clojure/lang/RT.class")) ;;=> "file:/root/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar!/clojure/lang/RT.class" ``` This has chopped of the `jar:` prefix, but not the `!/jar/file/entry` suffix. The result is a `file:` URL that can't be resolved. --- CHANGELOG.md | 1 + src/orchard/java.clj | 12 +----- src/orchard/java/legacy_parser.clj | 32 +++++++++++--- src/orchard/java/parser.clj | 42 ++++++++++++++----- test/orchard/java/DummyClass.java | 21 ++++++++++ test/orchard/java/parser_test.clj | 67 ++++++++++++++++++++++++++++++ test/orchard/java_test.clj | 6 ++- 7 files changed, 153 insertions(+), 28 deletions(-) create mode 100644 test/orchard/java/DummyClass.java create mode 100644 test/orchard/java/parser_test.clj diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b6db7372..e7f6ef368 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ ### Bugs Fixed * [#123](https://github.com/clojure-emacs/orchard/pull/123): Fix info lookups from namespaces that don't yet exist +* [#124](https://github.com/clojure-emacs/orchard/pull/124): Remove costly `io/resource` lookup ## 0.7.1 (2021-04-18) diff --git a/src/orchard/java.clj b/src/orchard/java.clj index 0565b93e9..4cca70b66 100644 --- a/src/orchard/java.clj +++ b/src/orchard/java.clj @@ -250,16 +250,6 @@ (def cache (atom {})) -(defn- immutable-source-file? - "Return true if the source file is effectively immutable. Specifically, this - returns true if no source file is available, or if the source file is in a - jar/zip archive." - [info] - (let [path (:file info) - src (when path (io/resource path))] - (or (not src) - (re-find #"\.(jar|zip)!" (str src))))) - (defn class-info "For the class symbol, return (possibly cached) Java class and member info. Members are indexed first by name, and then by argument types to list all @@ -269,7 +259,7 @@ info (if cached (:info cached) (class-info* class)) - last-modified (if (immutable-source-file? info) + last-modified (if (or (:archive? info) (nil? (:path info))) 0 (.lastModified ^File (io/file (:path info)))) stale (not= last-modified (:last-modified cached)) diff --git a/src/orchard/java/legacy_parser.clj b/src/orchard/java/legacy_parser.clj index d7030c88c..4c7ed2888 100644 --- a/src/orchard/java/legacy_parser.clj +++ b/src/orchard/java/legacy_parser.clj @@ -13,6 +13,7 @@ ModifierFilter RootDocImpl) (com.sun.tools.javac.tree JCTree) (java.io StringReader) + (java.net URL JarURLConnection) (java.net URI) (javax.swing.text.html HTML$Tag HTMLEditorKit$ParserCallback) (javax.swing.text.html.parser ParserDelegator) @@ -260,6 +261,18 @@ (str/replace "." "/") (str ".java"))) +(def url-protocol (memfn ^URL getProtocol)) + +(defn jar-path + "Given a jar:file:...!/... URL, return the location of the jar file on the + filesystem. Returns nil on any other URL." + [^URL jar-resource] + (when (= "jar" (url-protocol jar-resource)) + (let [^JarURLConnection conn (.openConnection jar-resource) + inner-url (.getJarFileURL conn)] + (when (= "file" (url-protocol inner-url)) + (.getPath inner-url))))) + (defn source-info "If the source for the Java class is available on the classpath, parse it and return info to supplement reflection. Specifically, this includes source @@ -270,9 +283,18 @@ (try (let [path (source-path klass)] (when-let [root (parse-java path)] - (assoc (->> (map parse-info (.classes root)) - (filter #(= klass (:class %))) - (first)) - :file path - :path (. (io/resource path) getPath)))) + (let [resource-url (io/resource path) + protocol (url-protocol resource-url)] + (assoc (->> (map parse-info (.classes root)) + (filter #(= klass (:class %))) + (first)) + ;; relative path on the classpath + :file path + ;; filesystem object for this resource, either the file + ;; itself, or its jar archive + :path (case protocol + "file" (.getPath resource-url) + "jar" (jar-path resource-url)) + ;; is the resource inside an archive + :archive? (= "jar" protocol))))) (catch Abort _))) diff --git a/src/orchard/java/parser.clj b/src/orchard/java/parser.clj index 789f4f1ff..78b7544c8 100644 --- a/src/orchard/java/parser.clj +++ b/src/orchard/java/parser.clj @@ -7,6 +7,7 @@ [clojure.string :as str]) (:import (java.io StringReader StringWriter) + (java.net URL JarURLConnection) (javax.lang.model.element Element ElementKind ExecutableElement TypeElement VariableElement) (javax.swing.text.html HTML$Tag HTMLEditorKit$ParserCallback) @@ -285,6 +286,18 @@ (str module "/" path) path)))) +(def url-protocol (memfn ^URL getProtocol)) + +(defn jar-path + "Given a jar:file:...!/... URL, return the location of the jar file on the + filesystem. Returns nil on any other URL." + [^URL jar-resource] + (when (= "jar" (url-protocol jar-resource)) + (let [^JarURLConnection conn (.openConnection jar-resource) + inner-url (.getJarFileURL conn)] + (when (= "file" (url-protocol inner-url)) + (.getPath inner-url))))) + (defn source-info "If the source for the Java class is available on the classpath, parse it and return info to supplement reflection. Specifically, this includes source @@ -296,15 +309,24 @@ (when-let [path (source-path klass)] (when-let [^DocletEnvironment root (parse-java path (module-name klass))] (try - (assoc (->> (.getIncludedElements root) - (filter #(#{ElementKind/CLASS - ElementKind/INTERFACE - ElementKind/ENUM} - (.getKind ^Element %))) - (map #(parse-info % root)) - (filter #(= klass (:class %))) - (first)) - :file path - :path (.getPath (io/resource path))) + (let [resource-url (io/resource path) + protocol (url-protocol resource-url)] + (assoc (->> (.getIncludedElements root) + (filter #(#{ElementKind/CLASS + ElementKind/INTERFACE + ElementKind/ENUM} + (.getKind ^Element %))) + (map #(parse-info % root)) + (filter #(= klass (:class %))) + (first)) + ;; relative path on the classpath + :file path + ;; filesystem object for this resource, either the file + ;; itself, or its jar archive + :path (case protocol + "file" (.getPath resource-url) + "jar" (jar-path resource-url)) + ;; is the resource inside an archive + :archive? (= "jar" protocol))) (finally (.close (.getJavaFileManager root)))))) (catch Throwable _))) diff --git a/test/orchard/java/DummyClass.java b/test/orchard/java/DummyClass.java new file mode 100644 index 000000000..134fd6ffb --- /dev/null +++ b/test/orchard/java/DummyClass.java @@ -0,0 +1,21 @@ +package orchard.java; + +/** + * Class level docstring. + * + * 
+ *   DummyClass dc = new DummyClass();
+ *
+ * + * @author Arne Brasseur + */ +public class DummyClass { + /** + * Method-level docstring. + * + * @returns the string "hello" + */ + public String dummyMethod() { + return "hello"; + } +} diff --git a/test/orchard/java/parser_test.clj b/test/orchard/java/parser_test.clj new file mode 100644 index 000000000..9bf6d31d7 --- /dev/null +++ b/test/orchard/java/parser_test.clj @@ -0,0 +1,67 @@ +(ns orchard.java.parser-test + (:require [orchard.misc :as misc] + [clojure.test :refer :all])) + +(when (>= orchard.misc/java-api-version 9) + (require '[orchard.java.parser :as parser]) + + (defn compile-class-from-source + "Compile a java file on the classpath. + Returns true if all went well." + [classname] + (let [compiler (javax.tools.ToolProvider/getSystemJavaCompiler)] + (.. compiler + (getTask + nil ;; out + nil ;; fileManager + nil ;; diagnosticListener + nil ;; compilerOptions + nil ;; classnames for annotation processing + ;; compilationUnits + [(.. compiler + (getStandardFileManager nil nil nil) + (getJavaFileForInput javax.tools.StandardLocation/CLASS_PATH + classname + javax.tools.JavaFileObject$Kind/SOURCE))]) + call))) + + (deftest source-info-test + (is (compile-class-from-source "orchard.java.DummyClass")) + + (testing "file on the filesystem" + (is (= {:class 'orchard.java.DummyClass, + :members + '{orchard.java.DummyClass + {[] + {:name orchard.java.DummyClass, + :type void, + :argtypes [], + :argnames [], + :doc nil, + :line 12, + :column 8}}, + dummyMethod + {[] + {:name dummyMethod, + :type java.lang.String, + :argtypes [], + :argnames [], + :doc "Method-level docstring. @returns the string \"hello\"", + :line 18, + :column 3}}}, + :doc + "Class level docstring.\n\n```\n DummyClass dc = new DummyClass();\n```\n\n@author Arne Brasseur", + :line 12, + :column 1, + :file "orchard/java/DummyClass.java", + :path (str (System/getProperty "user.dir") + "/test/orchard/java/DummyClass.java") + :archive? false} + ((resolve 'orchard.java.parser/source-info) 'orchard.java.DummyClass)))) + + (testing "java file in a jar" + (let [rt-info ((resolve 'orchard.java.parser/source-info) 'clojure.lang.RT)] + (is (= {:file "clojure/lang/RT.java" :archive? true} + (select-keys rt-info [:file :archive?]))) + (is (re-find #"/.*/.m2/repository/org/clojure/clojure/.*/clojure-.*-sources.jar" + (:path rt-info))))))) diff --git a/test/orchard/java_test.clj b/test/orchard/java_test.clj index 719bf58c4..fd5b3ceff 100644 --- a/test/orchard/java_test.clj +++ b/test/orchard/java_test.clj @@ -81,14 +81,16 @@ (deftest map-structure-test (when jdk-parser? (testing "Parsed map structure = reflected map structure" - (let [cols #{:file :line :column :doc :argnames :argtypes :path} + (let [cols #{:file :line :column :doc :argnames :argtypes :path :archive?} keys= #(= (set (keys (apply dissoc %1 cols))) (set (keys %2))) c1 (class-info* 'clojure.lang.Compiler) c2 (with-redefs [source-info (constantly nil)] (class-info* 'clojure.lang.Compiler))] ;; Class info - (is (keys= c1 c2)) + (is (keys= c1 c2) (str "Difference: " + (pr-str [(remove (set (keys c1)) (keys c2)) + (remove (set (keys c2)) (keys c1))]))) ;; Members (is (keys (:members c1))) (is (= (keys (:members c1))