From 81b9bdceef4558103208001f11dd4e0671fcd334 Mon Sep 17 00:00:00 2001 From: Tommi Reiman Date: Sat, 9 Feb 2019 11:51:15 +0200 Subject: [PATCH] Small improvement * Sort linear routes secondary with static path length * Unwrap data-matchers from linear-router * Simplify StaticMatcher impl --- modules/reitit-core/java-src/reitit/Trie.java | 64 +++++++++++------ modules/reitit-core/src/reitit/trie.cljc | 72 +++++++++++-------- perf-test/clj/reitit/go_perf_test.clj | 2 + 3 files changed, 88 insertions(+), 50 deletions(-) diff --git a/modules/reitit-core/java-src/reitit/Trie.java b/modules/reitit-core/java-src/reitit/Trie.java index e994dd4d..738a9516 100644 --- a/modules/reitit-core/java-src/reitit/Trie.java +++ b/modules/reitit-core/java-src/reitit/Trie.java @@ -63,6 +63,8 @@ public class Trie { Match match(int i, int max, char[] path, Match match); int depth(); + + int length(); } public static StaticMatcher staticMatcher(String path, Matcher child) { @@ -98,6 +100,11 @@ public class Trie { return child.depth() + 1; } + @Override + public int length() { + return path.length; + } + @Override public String toString() { return "[\"" + new String(path) + "\" " + child + "]"; @@ -129,52 +136,50 @@ public class Trie { return 1; } + @Override + public int length() { + return 0; + } + @Override public String toString() { return (data != null ? data.toString() : "nil"); } } - public static WildMatcher wildMatcher(Keyword parameter, Matcher child) { - return new WildMatcher(parameter, child); + public static WildMatcher wildMatcher(Keyword parameter, char end, Matcher child) { + return new WildMatcher(parameter, end, child); } static final class WildMatcher implements Matcher { private final Keyword key; + private final char end; private final Matcher child; - WildMatcher(Keyword key, Matcher child) { + WildMatcher(Keyword key, char end, Matcher child) { this.key = key; + this.end = end; this.child = child; } @Override public Match match(int i, int max, char[] path, Match match) { - if (i < max && path[i] != '/') { + if (i < max && path[i] != end) { boolean hasPercent = false; boolean hasPlus = false; + int stop = max; for (int j = i; j < max; j++) { final char c = path[j]; - switch (c) { - case '/': - final Match m = child.match(j, max, path, match); - if (m != null) { - m.params.assoc(key, decode(path, i, j, hasPercent, hasPlus)); - } - return m; - case '%': - hasPercent = true; - break; - case '+': - hasPlus = true; - break; - default: - break; + if (c == end) { + stop = j; + break; } + hasPercent = hasPercent || c == '%'; + hasPlus = hasPlus || c == '+'; } - final Match m = child.match(max, max, path, match); + final Match m = child.match(stop, max, path, match); if (m != null) { - m.params.assoc(key, decode(path, i, max, hasPercent, hasPlus)); + m.params.assoc(key, decode(path, i, stop, hasPercent, hasPlus)); } return m; } @@ -186,6 +191,11 @@ public class Trie { return child.depth() + 1; } + @Override + public int length() { + return 0; + } + @Override public String toString() { return "[" + key + " " + child + "]"; @@ -220,6 +230,11 @@ public class Trie { return 1; } + @Override + public int length() { + return 0; + } + @Override public String toString() { return "[" + parameter + " " + new DataMatcher(data) + "]"; @@ -237,7 +252,7 @@ public class Trie { LinearMatcher(List childs) { this.childs = childs.toArray(new Matcher[0]); - Arrays.sort(this.childs, Comparator.comparing(Matcher::depth).reversed()); + Arrays.sort(this.childs, Comparator.comparing(Matcher::depth).thenComparing(Matcher::length).reversed()); this.size = childs.size(); } @@ -257,6 +272,11 @@ public class Trie { return Arrays.stream(childs).mapToInt(Matcher::depth).max().orElseThrow(NoSuchElementException::new); } + @Override + public int length() { + return 0; + } + @Override public String toString() { return Arrays.toString(childs); diff --git a/modules/reitit-core/src/reitit/trie.cljc b/modules/reitit-core/src/reitit/trie.cljc index 52584e2e..645c64df 100644 --- a/modules/reitit-core/src/reitit/trie.cljc +++ b/modules/reitit-core/src/reitit/trie.cljc @@ -15,7 +15,8 @@ (defprotocol Matcher (match [this i max path]) (view [this]) - (depth [this])) + (depth [this]) + (length [this])) (defn -assoc! [match k v] (let [params (or (:path-params match) (transient {}))] @@ -33,8 +34,7 @@ (not= (get s1 i) (get s2 i)) (if-not (zero? i) (subs s1 0 i)) ;; recur - :else - (recur (inc i)))))) + :else (recur (inc i)))))) (defn- -keyword [s] (if-let [i (str/index-of s "/")] @@ -81,10 +81,13 @@ (assoc node :data data) (instance? Wild path) - (update-in node [:wilds (:value path)] (fn [n] (-insert (or n (-node {})) ps data))) + (let [next (first ps)] + (if (or (instance? Wild next) (instance? CatchAll next)) + (throw (ex-info (str "Two following wilds: " path ", " next) {})) + (update-in node [:wilds path] (fn [n] (-insert (or n (-node {})) ps data))))) (instance? CatchAll path) - (assoc-in node [:catch-all (:value path)] (-node {:data data})) + (assoc-in node [:catch-all path] (-node {:data data})) (str/blank? path) (-insert node ps data) @@ -118,8 +121,7 @@ #?(:cljs (defn decode! [path start end percent?] - (let [path (subs path start end)] - (if percent? (js/decodeURIComponent path) path)))) + (if percent? (js/decodeURIComponent (subs path start end)) path))) (defn data-matcher [data] #?(:clj (Trie/dataMatcher data) @@ -129,7 +131,8 @@ (if (= i max) match)) (view [_] data) - (depth [_] 1))))) + (depth [_] 1) + (length [_]))))) (defn static-matcher [path matcher] #?(:clj (Trie/staticMatcher ^String path ^Trie$Matcher matcher) @@ -143,25 +146,27 @@ (if (= (get p (+ i j)) (get path j)) (recur (inc j))))))) (view [_] [path (view matcher)]) - (depth [_] (inc (depth matcher))))))) + (depth [_] (inc (depth matcher))) + (length [_] (count path)))))) -(defn wild-matcher [key matcher] - #?(:clj (Trie/wildMatcher key matcher) +(defn wild-matcher [key end matcher] + #?(:clj (Trie/wildMatcher key (if end (Character. end)) matcher) :cljs (reify Matcher (match [_ i max path] - (if (and (< i max) (not= (get path i) \/)) + (if (and (< i max) (not= (get path i) end)) (loop [percent? false, j i] (if (= max j) (if-let [match (match matcher max max path)] (-assoc! match key (decode! path i max percent?))) (let [c ^char (get path j)] - (case c - \/ (if-let [match (match matcher j max path)] - (-assoc! match key (decode! path i j percent?))) + (condp = c + end (if-let [match (match matcher j max path)] + (-assoc! match key (decode! path i j percent?))) \% (recur true (inc j)) (recur percent? (inc j)))))))) (view [_] [key (view matcher)]) - (depth [_] (inc (depth matcher)))))) + (depth [_] (inc (depth matcher))) + (length [_])))) (defn catch-all-matcher [key data] #?(:clj (Trie/catchAllMatcher key data) @@ -170,11 +175,12 @@ (match [_ i max path] (if (< i max) (-assoc! match key (decode! path i max true)))) (view [_] [key [data]]) - (depth [_] 1))))) + (depth [_] 1) + (length [_]))))) (defn linear-matcher [matchers] #?(:clj (Trie/linearMatcher matchers) - :cljs (let [matchers (vec (reverse (sort-by depth matchers))) + :cljs (let [matchers (vec (reverse (sort-by (juxt depth length) matchers))) size (count matchers)] (reify Matcher (match [_ i max path] @@ -183,7 +189,8 @@ (or (match (get matchers j) i max path) (recur (inc j)))))) (view [_] (mapv view matchers)) - (depth [_] (apply max (map depth matchers))))))) + (depth [_] (apply max 0 (map depth matchers))) + (length [_]))))) ;; ;; public api @@ -201,14 +208,17 @@ (-insert (or node (-node {})) (split-path path) data))) (defn compile [{:keys [data children wilds catch-all]}] - (let [matchers (cond-> [] - data (conj (data-matcher data)) - children (into (for [[p c] children] (static-matcher p (compile c)))) - wilds (into (for [[p c] wilds] (wild-matcher p (compile c)))) - catch-all (into (for [[p c] catch-all] (catch-all-matcher p (:data c)))))] - (if (rest matchers) - (linear-matcher matchers) - (first matchers)))) + (let [ends (fn [{:keys [children]}] (or (keys children) ["/"])) + matchers (-> [] + (cond-> data (conj (data-matcher data))) + (into (for [[p c] children] (static-matcher p (compile c)))) + (into (for [[p c] wilds, end (ends c)] + (wild-matcher (:value p) (first end) (compile (update c :children select-keys [end]))))) + (into (for [[p c] catch-all] (catch-all-matcher (:value p) (:data c)))))] + (cond + (> (count matchers) 1) (linear-matcher matchers) + (= (count matchers) 1) (first matchers) + :else (data-matcher nil)))) (defn pretty [matcher] #?(:clj (-> matcher str read-string eval) @@ -284,7 +294,13 @@ ["/v1/orgs/:org-id/topics" 57]] (insert) (compile) - (pretty)) + (pretty) + (./aprint)) + + (-> [["/{a}/2"] + ["/{a}.2"]] + (insert) + (compile)) (-> [["/kikka" 2] ["/kikka/kakka/kukka" 3] diff --git a/perf-test/clj/reitit/go_perf_test.clj b/perf-test/clj/reitit/go_perf_test.clj index f62983de..36d5d3b9 100644 --- a/perf-test/clj/reitit/go_perf_test.clj +++ b/perf-test/clj/reitit/go_perf_test.clj @@ -332,6 +332,7 @@ ;; 305ns (trie-router, no injects) ;; 281ns (trie-router, no injects, optimized) ;; 277ns (trie-router, no injects, switch-case) - 690ns clojure + ;; 273ns (trie-router, no injects, direct-data) (let [req (map->Req {:request-method :get, :uri "/repos/julienschmidt/httprouter/stargazers"})] (title "param") (assert (= {:status 200, :body "/repos/:owner/:repo/stargazers"} (app req))) @@ -346,6 +347,7 @@ ;; 66µs (trie-router, no injects) ;; 64µs (trie-router, no injects, optimized) - 124µs (clojure) ;; 63µs (trie-router, no injects, switch-case) - 124µs (clojure) + ;; 63ns (trie-router, no injects, direct-data) (let [requests (mapv route->req routes)] (title "all") (cc/quick-bench