Improve performance of params middleware

[bsless]

Improve performance of assoc-query-params and assoc-form-params

[weavejester]

Can I see the benchmarks?

[bsless]

I'll rerun them and share the results here, both criterium and clj-async-profiler

[bsless]

(ns user
  (:require
   [clojure.pprint :refer [pprint]]
   [ring.util.io :refer [string-input-stream]]
   [ring.middleware.params :refer [wrap-params]]
   [criterium.core :as cc]
   [clj-async-profiler.core :as prof]))


(def wrapped-echo (wrap-params identity))

(def reqs
  [
   {:query-string "foo=bar&biz=bat%25"}
   {:query-string "foo=bar"
    :headers      {"content-type" "application/x-www-form-urlencoded"}
    :body         (string-input-stream "biz=bat%25")}
   {:headers {"content-type" "application/json"}
    :body    (string-input-stream "{foo: \"bar\"}")}
   {:query-string ""
    :headers      {"content-type" "application/x-www-form-urlencoded"}
    :body         (string-input-stream "")}
   {:headers {"content-type" "application/x-www-form-urlencoded;charset=UTF-16"}
    :body (string-input-stream "hello=world" "UTF-16")}
   ])


(comment
  (doseq [req reqs]
    (pprint req)
    (cc/quick-bench (wrapped-echo req))))

Results

;;; Before

{:query-string "foo=bar&biz=bat%25"}
Evaluation count : 433878 in 6 samples of 72313 calls.
             Execution time mean : 1.396233 µs
    Execution time std-deviation : 20.126816 ns
   Execution time lower quantile : 1.379531 µs ( 2.5%)
   Execution time upper quantile : 1.428941 µs (97.5%)
                   Overhead used : 1.982107 ns

Found 1 outliers in 6 samples (16.6667 %)
	low-severe	 1 (16.6667 %)
 Variance from outliers : 13.8889 % Variance is moderately inflated by outliers


{:query-string "foo=bar",
 :headers {"content-type" "application/x-www-form-urlencoded"},
 :body
 #object[java.io.ByteArrayInputStream 0x2d15daf3 "java.io.ByteArrayInputStream@2d15daf3"]}
Evaluation count : 154662 in 6 samples of 25777 calls.
             Execution time mean : 4.193348 µs
    Execution time std-deviation : 187.656985 ns
   Execution time lower quantile : 3.961360 µs ( 2.5%)
   Execution time upper quantile : 4.388309 µs (97.5%)
                   Overhead used : 1.982107 ns


{:headers {"content-type" "application/json"},
 :body
 #object[java.io.ByteArrayInputStream 0x5e48ea15 "java.io.ByteArrayInputStream@5e48ea15"]}
Evaluation count : 833454 in 6 samples of 138909 calls.
             Execution time mean : 724.075423 ns
    Execution time std-deviation : 3.357064 ns
   Execution time lower quantile : 720.762305 ns ( 2.5%)
   Execution time upper quantile : 728.305895 ns (97.5%)
                   Overhead used : 1.982107 ns


{:query-string "",
 :headers {"content-type" "application/x-www-form-urlencoded"},
 :body
 #object[java.io.ByteArrayInputStream 0x5c50ece "java.io.ByteArrayInputStream@5c50ece"]}
Evaluation count : 166830 in 6 samples of 27805 calls.
             Execution time mean : 3.909397 µs
    Execution time std-deviation : 279.900308 ns
   Execution time lower quantile : 3.588381 µs ( 2.5%)
   Execution time upper quantile : 4.253211 µs (97.5%)
                   Overhead used : 1.982107 ns


{:headers
 {"content-type" "application/x-www-form-urlencoded;charset=UTF-16"},
 :body
 #object[java.io.ByteArrayInputStream 0x4ced476f "java.io.ByteArrayInputStream@4ced476f"]}
Evaluation count : 152184 in 6 samples of 25364 calls.
             Execution time mean : 4.427176 µs
    Execution time std-deviation : 288.467162 ns
   Execution time lower quantile : 4.100349 µs ( 2.5%)
   Execution time upper quantile : 4.755838 µs (97.5%)
                   Overhead used : 1.982107 ns


;;; After

{:query-string "foo=bar&biz=bat%25"}
Evaluation count : 579192 in 6 samples of 96532 calls.
             Execution time mean : 1.104572 µs
    Execution time std-deviation : 79.386111 ns
   Execution time lower quantile : 1.045246 µs ( 2.5%)
   Execution time upper quantile : 1.235951 µs (97.5%)
                   Overhead used : 1.982107 ns

Found 1 outliers in 6 samples (16.6667 %)
	low-severe	 1 (16.6667 %)
 Variance from outliers : 15.2662 % Variance is moderately inflated by outliers


{:query-string "foo=bar",
 :headers {"content-type" "application/x-www-form-urlencoded"},
 :body
 #object[java.io.ByteArrayInputStream 0x1c8b0676 "java.io.ByteArrayInputStream@1c8b0676"]}


Evaluation count : 166824 in 6 samples of 27804 calls.
             Execution time mean : 3.901410 µs
    Execution time std-deviation : 282.704281 ns
   Execution time lower quantile : 3.628179 µs ( 2.5%)
   Execution time upper quantile : 4.244194 µs (97.5%)
                   Overhead used : 1.982107 ns


{:headers {"content-type" "application/json"},
 :body
 #object[java.io.ByteArrayInputStream 0x637e9d41 "java.io.ByteArrayInputStream@637e9d41"]}
Evaluation count : 1675230 in 6 samples of 279205 calls.
             Execution time mean : 377.726081 ns
    Execution time std-deviation : 7.517493 ns
   Execution time lower quantile : 371.217915 ns ( 2.5%)
   Execution time upper quantile : 389.496298 ns (97.5%)
                   Overhead used : 1.982107 ns

Found 1 outliers in 6 samples (16.6667 %)
	low-severe	 1 (16.6667 %)
 Variance from outliers : 13.8889 % Variance is moderately inflated by outliers


{:query-string "",
 :headers {"content-type" "application/x-www-form-urlencoded"},
 :body
 #object[java.io.ByteArrayInputStream 0x5d6fcbe "java.io.ByteArrayInputStream@5d6fcbe"]}
Evaluation count : 189630 in 6 samples of 31605 calls.
             Execution time mean : 3.538577 µs
    Execution time std-deviation : 188.404945 ns
   Execution time lower quantile : 3.258277 µs ( 2.5%)
   Execution time upper quantile : 3.727850 µs (97.5%)
                   Overhead used : 1.982107 ns


{:headers
 {"content-type" "application/x-www-form-urlencoded;charset=UTF-16"},
 :body
 #object[java.io.ByteArrayInputStream 0x3429b033 "java.io.ByteArrayInputStream@3429b033"]}
Evaluation count : 163242 in 6 samples of 27207 calls.
             Execution time mean : 3.964294 µs
    Execution time std-deviation : 230.012813 ns
   Execution time lower quantile : 3.642308 µs ( 2.5%)
   Execution time upper quantile : 4.179469 µs (97.5%)
                   Overhead used : 1.982107 ns

[weavejester]

Thanks for the benchmarks. It looks like we can make some small improvements around the performance of these functions, but I'd like to do so in a way that doesn't adversely impact readability.

Perhaps instead we start from a function like:

(defn- assoc-param-map [req k v]
  (some-> req (assoc k (if-let [v' (req k)]
                         (reduce-kv assoc v' v)
                         v))))

And then write something like:

(-> request
    (assoc-param-map :query-params params)
    (assoc-param-map :params params))

[bsless]

How about something like this:

(defn assoc-query-params
  "Parse and assoc parameters from the query string with the request."
  {:added "1.3"}
  [request encoding]
  (let [params (if-let [query-string (:query-string request)]
                 (parse-params query-string encoding)
                 {})]
    (-> request
        (assoc-param-map :query-params params)
        (assoc-param-map :params params))))

Also, how about merge-param instead of assoc-param-map?
I'm not attached to any choice of name here, wondering what's most readable and best conveys the meaning of what happens.

[bsless]

By the way, for reference, you can take a look at the flame graphs I generated here and see which parts of wrap-params consume the most CPU.
These results are what prompted my PRs and guided my analysis.

[bsless]

@weavejester Sorry for tagging you but I wouldn't want this to fall by the wayside. Anything else I can do or provide to help this along?

[weavejester]

How about something like this:

(defn assoc-query-params
  "Parse and assoc parameters from the query string with the request."
  {:added "1.3"}
  [request encoding]
  (let [params (if-let [query-string (:query-string request)]
                 (parse-params query-string encoding)
                 {})]
    (-> request
        (assoc-param-map :query-params params)
        (assoc-param-map :params params))))

That looks fine.

Also, how about merge-param instead of assoc-param-map?

I'd prefer to keep it consistent with the naming of assoc-query-params.

[bsless]

@weavejester gentle reminder 🙂

[weavejester]

Could you change the commit message to:

Improve performance of params middleware

[bsless]

@weavejester done

[bsless]

@weavejester Changed PR title as well to align with commit message.
Any other loose ends I might have left out?

[bsless]

Hello :)
Do you have an estimate when I can expect a new release?

Thanks

[weavejester]

Probably by the end of the year. This patch seems non-critical.

[bsless]

Absolutely, just wanted to know so I could plan towards running the next batch of benchmarks, those experiments take long times to complete so I want to batch the updates to the libraries I'm using, too.