From c2fa9227171d62ca6964c5112183c3323799a0de Mon Sep 17 00:00:00 2001 From: Nathan Marz Date: Sat, 21 May 2016 09:44:20 -0400 Subject: [PATCH] automatically factor anonymous functions with pred, 20% more effient cache strategy, more efficient hot path, added ability with must-cache-paths to error when a path can't be cached and get detailed information why --- src/clj/com/rpl/specter.cljx | 11 +- src/clj/com/rpl/specter/impl.cljx | 185 +++++++++++++++++++---------- src/clj/com/rpl/specter/macros.clj | 35 +++--- 3 files changed, 145 insertions(+), 86 deletions(-) diff --git a/src/clj/com/rpl/specter.cljx b/src/clj/com/rpl/specter.cljx index 86f5ff6..37dfc23 100644 --- a/src/clj/com/rpl/specter.cljx +++ b/src/clj/com/rpl/specter.cljx @@ -26,6 +26,8 @@ (defn comp-paths [& paths] (i/comp-paths* (vec paths))) +(def must-cache-paths! i/must-cache-paths!) + ;; Selection functions (def ^{:doc "Version of select that takes in a path pre-compiled with comp-paths"} @@ -403,15 +405,12 @@ (transform* [aset structure next-fn] (i/filter-transform aset structure next-fn))) -(defpath +(def ^{:doc "Keeps the element only if it matches the supplied predicate. This is the late-bound parameterized version of using a function directly in a path."} pred - [afn] - (select* [this structure next-fn] - (i/filter-select afn structure next-fn)) - (transform* [this structure next-fn] - (i/filter-transform afn structure next-fn))) + i/pred* + ) (defpath ^{:doc "Navigates to the provided val if the structure is nil. Otherwise it stays diff --git a/src/clj/com/rpl/specter/impl.cljx b/src/clj/com/rpl/specter/impl.cljx index 94ecf0d..1b1538b 100644 --- a/src/clj/com/rpl/specter/impl.cljx +++ b/src/clj/com/rpl/specter/impl.cljx @@ -633,26 +633,42 @@ params-maker ; can be null ]) -(def CACHE - #+clj (java.util.concurrent.ConcurrentHashMap.) +(defonce PATH-CACHE + #+clj (mutable-cell (java.util.HashMap.)) #+cljs (atom {}) ) -#+clj -(defn add-cache! [k v] - (.put ^java.util.concurrent.ConcurrentHashMap CACHE k v)) +(def ^:dynamic *must-cache-paths* false) + +(defn must-cache-paths! + ([] (must-cache-paths! true)) + ([v] (alter-var-root #'*must-cache-paths* (constantly v)))) #+clj -(defn get-cache [k] - (.get ^java.util.concurrent.ConcurrentHashMap CACHE k)) +(defn add-path-cache! [k v] + ;; This looks very inefficient but is actually the best approach + ;; without real inline caches. The total number of copies will be the + ;; number of Specter callsites (plus perhaps a few more if + ;; multiple callsites attempt to be cached at exactly the same time). + ;; Eventually it will stabilize and there won't be any more garbage generated. + ;; The `select` performance using this cache strategy is ~20% faster for + ;; the [:a :b :c] path use case than ConcurrentHashMap. + (let [newmap (java.util.HashMap. ^java.util.HashMap (get-cell PATH-CACHE))] + (.put newmap k v) + (set-cell! PATH-CACHE newmap) + )) + +#+clj +(defn get-path-cache [k] + (.get ^java.util.HashMap (get-cell PATH-CACHE) k)) #+cljs (defn add-cache! [k v] - (swap! CACHE (fn [m] (assoc m k v)))) + (swap! PATH-CACHE (fn [m] (assoc m k v)))) #+cljs (defn get-cache [k] - (get @CACHE k)) + (get @PATH-CACHE k)) (defn- extract-original-code [p] (cond @@ -672,70 +688,109 @@ (reset! failed-atom true) nil) -(defn- magic-precompilation* [p params-atom failed-atom] - (cond - (vector? p) - (mapv - #(magic-precompilation* % params-atom failed-atom) - p) - - (instance? LocalSym p) - (magic-fail! failed-atom) - - (instance? VarUse p) - (let [v (:var p) - vv (var-get v)] - (if (and (-> v meta :dynamic not) - (valid-navigator? vv)) - vv - (magic-fail! failed-atom) - )) - - (instance? SpecialFormUse p) - (magic-fail! failed-atom) - - (instance? FnInvocation p) - (let [op (:op p) - ps (:params p)] - (if (instance? VarUse op) - (let [v (:var op) - vv (var-get v)] - (if (-> v meta :dynamic) - (magic-fail! failed-atom) - (cond - (instance? ParamsNeededPath vv) - ;;TODO: if all params are constants, then just bind the path right here - ;;otherwise, add the params - (do - (swap! params-atom concat ps) - vv - ) - - (and (fn? vv) (-> vv meta :pathedfn)) - (let [subpath (mapv #(magic-precompilation* % params-atom failed-atom) - ps)] - (if @failed-atom - nil - (apply vv subpath) - )) - - :else - (magic-fail! failed-atom) - ))) - (magic-fail! failed-atom) - )) - - :else - (magic-fail! failed-atom) +(def pred* + (->ParamsNeededPath + (->TransformFunctions + RichPathExecutor + (fn [params params-idx vals structure next-fn] + (let [afn (aget ^objects params params-idx)] + (if (afn structure) + (next-fn params (inc params-idx) vals structure) + ))) + (fn [params params-idx vals structure next-fn] + (let [afn (aget ^objects params params-idx)] + (if (afn structure) + (next-fn params (inc params-idx) vals structure) + structure + )))) + 1 )) +(defn- magic-precompilation* [p params-atom failed-atom] + (let [magic-fail! (fn [& reason] + (if *must-cache-paths* + (println "Failed to cache path:" (apply str reason))) + (reset! failed-atom true) + nil)] + (cond + (vector? p) + (mapv + #(magic-precompilation* % params-atom failed-atom) + p) + + (instance? LocalSym p) + (magic-fail! "Local symbol " (:sym p) " where navigator expected") + + (instance? VarUse p) + (let [v (:var p) + vv (var-get v)] + (cond (-> v meta :dynamic) + (magic-fail! "Var " (:sym p) " is dynamic") + (valid-navigator? vv) vv + :else (magic-fail! "Var " (:sym p) " is not a navigator") + )) + + (instance? SpecialFormUse p) + (if (-> p :code first (= 'fn*)) + (do + (swap! params-atom conj (:code p)) + pred* + ) + (magic-fail! "Special form " (:code p) " where navigator expected") + ) + + (instance? FnInvocation p) + (let [op (:op p) + ps (:params p)] + (if (instance? VarUse op) + (let [v (:var op) + vv (var-get v)] + (if (-> v meta :dynamic) + (magic-fail! "Var " (:sym op) " is dynamic") + (cond + (instance? ParamsNeededPath vv) + ;;TODO: if all params are constants, then just bind the path right here + ;;otherwise, add the params + ;; - could extend this to see if it contains nested function calls which + ;; are only on constants + (do + (swap! params-atom concat ps) + vv + ) + + (and (fn? vv) (-> vv meta :pathedfn)) + (let [subpath (mapv #(magic-precompilation* % params-atom failed-atom) + ps)] + (if @failed-atom + nil + (apply vv subpath) + )) + + :else + (magic-fail! "Var " (:sym op) " must be either a parameterized " + "navigator or a higher order pathed constructor function") + ))) + (magic-fail! "Code at " (extract-original-code p) " is in " + "function invocation position and must be either a parameterized " + "navigator or a higher order pathed constructor function" + ) + )) + + :else + (if (valid-navigator? p) + p + (magic-fail! "Constant " p " is not a valid navigator")) + ))) + (defn magic-precompilation [prepared-path used-locals] (let [params-atom (atom []) failed-atom (atom false) path (magic-precompilation* prepared-path params-atom failed-atom) ] (if @failed-atom - (->CachedPathInfo nil nil) + (if *must-cache-paths* + (throw-illegal "Failed to cache path") + (->CachedPathInfo nil nil)) (let [precompiled (comp-paths* path) params-code (mapv extract-original-code @params-atom) array-sym (gensym "array") diff --git a/src/clj/com/rpl/specter/macros.clj b/src/clj/com/rpl/specter/macros.clj index 6888d78..9527080 100644 --- a/src/clj/com/rpl/specter/macros.clj +++ b/src/clj/com/rpl/specter/macros.clj @@ -315,8 +315,8 @@ `(i/extend-protocolpath* ~protpath ~(protpath-sym protpath) ~(vec extensions))) (defmacro defpathedfn [name & args] - (let [[n args] (m/name-with-attributes name args)] - `(def ~n (vary-meta (fn ~@args) assoc :pathedfn true)))) + (let [[name args] (m/name-with-attributes name args)] + `(def ~name (vary-meta (fn ~@args) assoc :pathedfn true)))) (defn ic-prepare-path [locals-set path] @@ -344,16 +344,23 @@ path )) -;; still possible to mess this up with alter-var-root! +;; still possible to mess this up with alter-var-root (defmacro ic! [& path] ; "inline cache" (let [local-syms (-> &env keys set) used-locals (vec (i/walk-select local-syms vector path)) prepared-path (ic-prepare-path local-syms (walk/macroexpand-all (vec path))) - ;; TODO: will turning this into a keyword make it faster? + ;; TODO: unclear if using long here versus string makes + ;; a significant difference + ;; - but using random longs creates possibility of collisions + ;; (birthday problem) + ;; - ideally could have a real inline cache that wouldn't + ;; have to do any hashing/equality checking at all + ;; - with invokedynamic here, could go directly to the code + ;; to invoke and/or parameterize the precompiled path without + ;; a bunch of checks beforehand cache-id (str (java.util.UUID/randomUUID)) ] - - `(let [info# (i/get-cache ~cache-id) + `(let [info# (i/get-path-cache ~cache-id) ^com.rpl.specter.impl.CachedPathInfo info# (if info# @@ -362,19 +369,17 @@ ~prepared-path ~(mapv (fn [e] `(quote ~e)) used-locals) )] - (i/add-cache! ~cache-id info#) + (i/add-path-cache! ~cache-id info#) info# )) precompiled# (.-precompiled info#) params-maker# (.-params-maker info#)] - (cond (nil? precompiled#) - ~path - - (and precompiled# (nil? params-maker#)) - precompiled# - - :else - (i/bind-params* precompiled# (params-maker# ~@used-locals) 0) + (if (some? precompiled#) + (if (nil? params-maker#) + precompiled# + (i/bind-params* precompiled# (params-maker# ~@used-locals) 0) + ) + (i/comp-paths* ~(vec path)) )) ))