Closed bsless closed 2 years ago
Can I see the benchmarks?
I'll rerun them and share the results here, both criterium and clj-async-profiler
(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
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))
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.
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.
@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?
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 ofassoc-param-map
?
I'd prefer to keep it consistent with the naming of assoc-query-params
.
@weavejester gentle reminder :slightly_smiling_face:
Could you change the commit message to:
Improve performance of params middleware
@weavejester done
@weavejester Changed PR title as well to align with commit message. Any other loose ends I might have left out?
Hello :) Do you have an estimate when I can expect a new release?
Thanks
Probably by the end of the year. This patch seems non-critical.
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.
Improve performance of assoc-query-params and assoc-form-params