From fe73144075ed81437b4c01d375c82c66d9001fa4 Mon Sep 17 00:00:00 2001 From: Juho Teperi Date: Thu, 8 Oct 2015 22:42:06 +0300 Subject: [PATCH 1/5] Fix map comparisons and rename test case Two tests had the same name so only the latter was ran --- test/monger/test/conversion_test.clj | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/monger/test/conversion_test.clj b/test/monger/test/conversion_test.clj index 67c94bd..c989574 100644 --- a/test/monger/test/conversion_test.clj +++ b/test/monger/test/conversion_test.clj @@ -112,20 +112,20 @@ (.put "name" name) (.put "age" age)) output (from-db-object input false)] - (is (= (output { "name" name, "age" age }))) + (is (= output { "name" name, "age" age })) (is (= (output "name") name)) (is (nil? (output :name))) (is (= (output "age") age)) (is (nil? (output "points"))))) -(deftest convert-flat-db-object-to-map-without-keywordizing +(deftest convert-flat-db-object-to-map-with-keywordizing (let [name "Michael" age 26 input (doto (BasicDBObject.) (.put "name" name) (.put "age" age)) output (from-db-object input true)] - (is (= (output { :name name, :age age }))) + (is (= output { :name name, :age age })) (is (= (output :name) name)) (is (nil? (output "name"))) (is (= (output :age) age)) From c26ae0835d7bef1b8a06a126abdcbfc2499d20e3 Mon Sep 17 00:00:00 2001 From: Juho Teperi Date: Thu, 8 Oct 2015 22:53:23 +0300 Subject: [PATCH 2/5] Optimize from-db-object performance for DBObjects Creating temporary sequence from DBObject is quite slow. Using .keySet for reduce collection and calling .get inside reduce is considerably faster. The common code between Map and DBObject implementations can't be shared as reflection would completely kill the performance and function can't be type hinted as DBObject doesn't implement Map interface. Added a simple test case for the from-db-object performance. I'm seeing performance increase of 20% on this (170ms -> 140ms). --- src/clojure/monger/conversion.clj | 34 +++++++++++++++---------------- test/monger/test/stress_test.clj | 11 ++++++++-- 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/src/clojure/monger/conversion.clj b/src/clojure/monger/conversion.clj index 2185f44..3d6f6f9 100644 --- a/src/clojure/monger/conversion.clj +++ b/src/clojure/monger/conversion.clj @@ -105,7 +105,6 @@ -(declare associate-pairs) (defprotocol ConvertFromDBObject (from-db-object [input keywordize] "Converts given DBObject instance to a piece of Clojure data")) @@ -118,7 +117,12 @@ Map (from-db-object [^Map input keywordize] - (associate-pairs (.entrySet input) keywordize)) + (reduce (if keywordize + (fn [m ^String k] + (assoc m (keyword k) (from-db-object (.get input k) true))) + (fn [m ^String k] + (assoc m k (from-db-object (.get input k) false)))) + {} (.keySet input))) List (from-db-object [^List input keywordize] @@ -136,22 +140,16 @@ (from-db-object [^DBObject input keywordize] ;; DBObject provides .toMap, but the implementation in ;; subclass GridFSFile unhelpfully throws - ;; UnsupportedOperationException. This part is taken from congomongo and - ;; may need revisiting at a later point. MK. - (associate-pairs (for [key-set (.keySet input)] [key-set (.get input key-set)]) - keywordize))) - - -(defn- associate-pairs [pairs keywordize] - ;; Taking the keywordize test out of the fn reduces derefs - ;; dramatically, which was the main barrier to matching pure-Java - ;; performance for this marshalling. Taken from congomongo. MK. - (reduce (if keywordize - (fn [m [^String k v]] - (assoc m (keyword k) (from-db-object v true))) - (fn [m [^String k v]] - (assoc m k (from-db-object v false)))) - {} (reverse pairs))) + ;; UnsupportedOperationException. + ;; This is the same code as with Map. The code can't be shared using a + ;; function because reflection would kill the performance and DBObject + ;; and Map don't share a interface. + (reduce (if keywordize + (fn [m ^String k] + (assoc m (keyword k) (from-db-object (.get input k) true))) + (fn [m ^String k] + (assoc m k (from-db-object (.get input k) false)))) + {} (.keySet input)))) diff --git a/test/monger/test/stress_test.clj b/test/monger/test/stress_test.clj index 131f48b..ea1a06f 100644 --- a/test/monger/test/stress_test.clj +++ b/test/monger/test/stress_test.clj @@ -1,7 +1,7 @@ (ns monger.test.stress-test (:require [monger.core :as mg] [monger.collection :as mc] - [monger.conversion :refer [to-db-object]] + [monger.conversion :refer [to-db-object from-db-object]] [clojure.test :refer :all]) (:import [com.mongodb WriteConcern] java.util.Date)) @@ -30,4 +30,11 @@ (mc/remove db collection) (println "Inserting " n " documents...") (time (mc/insert-batch db collection docs)) - (is (= n (mc/count db collection))))))) + (is (= n (mc/count db collection)))))) + + (deftest ^{:performance true} convert-large-number-of-dbojects-to-maps + (doseq [n [10 100 1000 20000 40000]] + (let [docs (map (fn [i] + (to-db-object {:title "Untitled" :created-at (Date.) :number i})) + (take n (iterate inc 1)))] + (time (doall (map (fn [x] (from-db-object x true)) docs))))))) From 15edf63ffd7638ce1efc0264ace4d75684c8d09b Mon Sep 17 00:00:00 2001 From: Juho Teperi Date: Fri, 9 Oct 2015 00:24:29 +0300 Subject: [PATCH 3/5] Fix test cases with bad = forms --- test/monger/test/authentication_test.clj | 6 +++--- test/monger/test/regular_finders_test.clj | 4 ++-- test/monger/test/updating_test.clj | 16 ++++++++-------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/test/monger/test/authentication_test.clj b/test/monger/test/authentication_test.clj index ee6eeda..e8d5447 100644 --- a/test/monger/test/authentication_test.clj +++ b/test/monger/test/authentication_test.clj @@ -12,12 +12,12 @@ (when-not (System/getenv "CI") (deftest ^{:authentication true} connect-to-mongo-via-uri-without-credentials (let [{:keys [conn db]} (mg/connect-via-uri "mongodb://127.0.0.1/monger-test4")] - (is (= (-> conn .getAddress ^InetAddress (.sameHost "127.0.0.1")))))) + (is (-> conn .getAddress (.sameHost "127.0.0.1"))))) (deftest ^{:authentication true} connect-to-mongo-via-uri-with-valid-credentials (let [{:keys [conn db]} (mg/connect-via-uri "mongodb://clojurewerkz/monger:monger@127.0.0.1/monger-test4")] (is (= "monger-test4" (.getName db))) - (is (= (-> conn .getAddress ^InetAddress (.sameHost "127.0.0.1")))) + (is (-> conn .getAddress (.sameHost "127.0.0.1"))) (mc/remove db "documents") ;; make sure that the database is selected ;; and operations get through. @@ -27,7 +27,7 @@ (if-let [uri (System/getenv "MONGOHQ_URL")] (deftest ^{:external true :authentication true} connect-to-mongo-via-uri-with-valid-credentials (let [{:keys [conn db]} (mg/connect-via-uri uri)] - (is (= (-> conn .getAddress ^InetAddress (.sameHost "127.0.0.1"))))))) + (is (-> conn .getAddress (.sameHost "127.0.0.1")))))) ;; diff --git a/test/monger/test/regular_finders_test.clj b/test/monger/test/regular_finders_test.clj index 7785f2c..bd03da1 100644 --- a/test/monger/test/regular_finders_test.clj +++ b/test/monger/test/regular_finders_test.clj @@ -147,14 +147,14 @@ doc-id (mu/random-uuid) doc { :data-store "MongoDB", :language "Clojure", :_id doc-id }] (mc/insert db collection doc) - (is (= (doc (mc/find-by-id db collection doc-id)))))) + (is (= doc (mc/find-by-id db collection doc-id))))) (deftest find-full-document-by-object-id-when-document-does-exist (let [collection "libraries" doc-id (ObjectId.) doc { :data-store "MongoDB", :language "Clojure", :_id doc-id }] (mc/insert db collection doc) - (is (= (doc (mc/find-by-id db collection doc-id)))))) + (is (= doc (mc/find-by-id db collection doc-id))))) (deftest find-full-document-map-by-string-id-when-document-does-exist (let [collection "libraries" diff --git a/test/monger/test/updating_test.clj b/test/monger/test/updating_test.clj index 3459d33..7897417 100644 --- a/test/monger/test/updating_test.clj +++ b/test/monger/test/updating_test.clj @@ -33,9 +33,9 @@ doc { :created-at date, :data-store "MongoDB", :language "Clojure", :_id doc-id } modified-doc { :created-at date, :data-store "MongoDB", :language "Erlang", :_id doc-id }] (mc/insert db collection doc) - (is (= (doc (mc/find-by-id db collection doc-id)))) + (is (= doc (mc/find-by-id db collection doc-id))) (mc/update db collection { :_id doc-id } { :language "Erlang" }) - (is (= (modified-doc (mc/find-by-id db collection doc-id)))))) + (is (= modified-doc (mc/find-by-id db collection doc-id))))) (deftest ^{:updating true} update-document-by-id-without-upsert-using-update-by-id (let [collection "libraries" @@ -44,9 +44,9 @@ doc { :created-at date, :data-store "MongoDB", :language "Clojure", :_id doc-id } modified-doc { :created-at date, :data-store "MongoDB", :language "Erlang", :_id doc-id }] (mc/insert db collection doc) - (is (= (doc (mc/find-by-id db collection doc-id)))) + (is (= doc (mc/find-by-id db collection doc-id))) (mc/update-by-id db collection doc-id { :language "Erlang" }) - (is (= (modified-doc (mc/find-by-id db collection doc-id)))))) + (is (= modified-doc (mc/find-by-id db collection doc-id))))) (deftest ^{:updating true} update-nested-document-fields-without-upsert-using-update-by-id (let [collection "libraries" @@ -55,9 +55,9 @@ doc { :created-at date :data-store "MongoDB" :language { :primary "Clojure" } :_id doc-id } modified-doc { :created-at date :data-store "MongoDB" :language { :primary "Erlang" } :_id doc-id }] (mc/insert db collection doc) - (is (= (doc (mc/find-by-id db collection doc-id)))) + (is (= doc (mc/find-by-id db collection doc-id))) (mc/update-by-id db collection doc-id { $set { "language.primary" "Erlang" }}) - (is (= (modified-doc (mc/find-by-id db collection doc-id)))))) + (is (= modified-doc (mc/find-by-id db collection doc-id))))) (deftest ^{:updating true} update-multiple-documents @@ -151,7 +151,7 @@ (is (= 1 (mc/count db collection))) (is (mr/updated-existing? (mc/update db collection { :language "Clojure" } modified-doc {:multi false :upsert true}))) (is (= 1 (mc/count db collection))) - (is (= (modified-doc (mc/find-by-id db collection doc-id)))) + (is (= modified-doc (mc/find-by-id db collection doc-id))) (mc/remove db collection))) (deftest ^{:updating true} upsert-a-document-using-upsert @@ -165,5 +165,5 @@ (is (= 1 (mc/count db collection))) (is (mr/updated-existing? (mc/upsert db collection {:language "Clojure"} modified-doc {:multi false}))) (is (= 1 (mc/count db collection))) - (is (= (modified-doc (mc/find-by-id db collection doc-id)))) + (is (= modified-doc (mc/find-by-id db collection doc-id))) (mc/remove db collection)))) From cf3d8f2ad34a220bd412f3de73b9ad2e743b3926 Mon Sep 17 00:00:00 2001 From: Juho Teperi Date: Fri, 9 Oct 2015 00:33:24 +0300 Subject: [PATCH 4/5] Fix test cases cases trying to compare db-objects and maps --- test/monger/test/regular_finders_test.clj | 15 ++++++++------- test/monger/test/updating_test.clj | 20 ++++++++++---------- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/test/monger/test/regular_finders_test.clj b/test/monger/test/regular_finders_test.clj index bd03da1..3db0ab2 100644 --- a/test/monger/test/regular_finders_test.clj +++ b/test/monger/test/regular_finders_test.clj @@ -8,7 +8,8 @@ [monger.result :as mgres] [monger.conversion :as mgcnv] [clojure.test :refer :all] - [monger.operators :refer :all])) + [monger.operators :refer :all] + [monger.conversion :refer [to-db-object]])) (let [conn (mg/connect) db (mg/get-db conn "monger-test")] @@ -147,36 +148,36 @@ doc-id (mu/random-uuid) doc { :data-store "MongoDB", :language "Clojure", :_id doc-id }] (mc/insert db collection doc) - (is (= doc (mc/find-by-id db collection doc-id))))) + (is (= (to-db-object doc) (mc/find-by-id db collection doc-id))))) (deftest find-full-document-by-object-id-when-document-does-exist (let [collection "libraries" doc-id (ObjectId.) doc { :data-store "MongoDB", :language "Clojure", :_id doc-id }] (mc/insert db collection doc) - (is (= doc (mc/find-by-id db collection doc-id))))) + (is (= (to-db-object doc) (mc/find-by-id db collection doc-id))))) (deftest find-full-document-map-by-string-id-when-document-does-exist (let [collection "libraries" doc-id (mu/random-uuid) doc { :data-store "MongoDB", :language "Clojure", :_id doc-id }] (mc/insert db collection doc) - (is (= (doc (mc/find-map-by-id db collection doc-id)))))) + (is (= doc (mc/find-map-by-id db collection doc-id))))) (deftest find-full-document-map-by-object-id-when-document-does-exist (let [collection "libraries" doc-id (ObjectId.) doc { :data-store "MongoDB", :language "Clojure", :_id doc-id }] (mc/insert db collection doc) - (is (= (doc (mc/find-map-by-id db collection doc-id)))))) + (is (= doc (mc/find-map-by-id db collection doc-id))))) (deftest find-partial-document-by-id-when-document-does-exist (let [collection "libraries" doc-id (mu/random-uuid) doc { :data-store "MongoDB", :language "Clojure", :_id doc-id }] (mc/insert db collection doc) - (is (= ({ :language "Clojure" } - (mc/find-by-id db collection doc-id [ :language ])))))) + (is (= (to-db-object { :_id doc-id :language "Clojure" }) + (mc/find-by-id db collection doc-id [ :language ]))))) (deftest find-partial-document-as-map-by-id-when-document-does-exist diff --git a/test/monger/test/updating_test.clj b/test/monger/test/updating_test.clj index 7897417..318bca9 100644 --- a/test/monger/test/updating_test.clj +++ b/test/monger/test/updating_test.clj @@ -33,9 +33,9 @@ doc { :created-at date, :data-store "MongoDB", :language "Clojure", :_id doc-id } modified-doc { :created-at date, :data-store "MongoDB", :language "Erlang", :_id doc-id }] (mc/insert db collection doc) - (is (= doc (mc/find-by-id db collection doc-id))) - (mc/update db collection { :_id doc-id } { :language "Erlang" }) - (is (= modified-doc (mc/find-by-id db collection doc-id))))) + (is (= (to-db-object doc) (mc/find-by-id db collection doc-id))) + (mc/update db collection { :_id doc-id } { $set { :language "Erlang" } }) + (is (= (to-db-object modified-doc) (mc/find-by-id db collection doc-id))))) (deftest ^{:updating true} update-document-by-id-without-upsert-using-update-by-id (let [collection "libraries" @@ -44,9 +44,9 @@ doc { :created-at date, :data-store "MongoDB", :language "Clojure", :_id doc-id } modified-doc { :created-at date, :data-store "MongoDB", :language "Erlang", :_id doc-id }] (mc/insert db collection doc) - (is (= doc (mc/find-by-id db collection doc-id))) - (mc/update-by-id db collection doc-id { :language "Erlang" }) - (is (= modified-doc (mc/find-by-id db collection doc-id))))) + (is (= (to-db-object doc) (mc/find-by-id db collection doc-id))) + (mc/update-by-id db collection doc-id { $set { :language "Erlang" } }) + (is (= (to-db-object modified-doc) (mc/find-by-id db collection doc-id))))) (deftest ^{:updating true} update-nested-document-fields-without-upsert-using-update-by-id (let [collection "libraries" @@ -55,9 +55,9 @@ doc { :created-at date :data-store "MongoDB" :language { :primary "Clojure" } :_id doc-id } modified-doc { :created-at date :data-store "MongoDB" :language { :primary "Erlang" } :_id doc-id }] (mc/insert db collection doc) - (is (= doc (mc/find-by-id db collection doc-id))) + (is (= (to-db-object doc) (mc/find-by-id db collection doc-id))) (mc/update-by-id db collection doc-id { $set { "language.primary" "Erlang" }}) - (is (= modified-doc (mc/find-by-id db collection doc-id))))) + (is (= (to-db-object modified-doc) (mc/find-by-id db collection doc-id))))) (deftest ^{:updating true} update-multiple-documents @@ -151,7 +151,7 @@ (is (= 1 (mc/count db collection))) (is (mr/updated-existing? (mc/update db collection { :language "Clojure" } modified-doc {:multi false :upsert true}))) (is (= 1 (mc/count db collection))) - (is (= modified-doc (mc/find-by-id db collection doc-id))) + (is (= (to-db-object modified-doc) (mc/find-by-id db collection doc-id))) (mc/remove db collection))) (deftest ^{:updating true} upsert-a-document-using-upsert @@ -165,5 +165,5 @@ (is (= 1 (mc/count db collection))) (is (mr/updated-existing? (mc/upsert db collection {:language "Clojure"} modified-doc {:multi false}))) (is (= 1 (mc/count db collection))) - (is (= modified-doc (mc/find-by-id db collection doc-id))) + (is (= (to-db-object modified-doc) (mc/find-by-id db collection doc-id))) (mc/remove db collection)))) From 1299e5e5ff777ebd807c6afb103f2cd47f97bfc7 Mon Sep 17 00:00:00 2001 From: Juho Teperi Date: Fri, 9 Oct 2015 00:35:02 +0300 Subject: [PATCH 5/5] Remove from-db-object implementation for Map It should be enough that from-db-object is implemented for DBObject --- src/clojure/monger/conversion.clj | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/clojure/monger/conversion.clj b/src/clojure/monger/conversion.clj index 3d6f6f9..8ed2c5f 100644 --- a/src/clojure/monger/conversion.clj +++ b/src/clojure/monger/conversion.clj @@ -115,15 +115,6 @@ Object (from-db-object [input keywordize] input) - Map - (from-db-object [^Map input keywordize] - (reduce (if keywordize - (fn [m ^String k] - (assoc m (keyword k) (from-db-object (.get input k) true))) - (fn [m ^String k] - (assoc m k (from-db-object (.get input k) false)))) - {} (.keySet input))) - List (from-db-object [^List input keywordize] (vec (map #(from-db-object % keywordize) input))) @@ -141,9 +132,6 @@ ;; DBObject provides .toMap, but the implementation in ;; subclass GridFSFile unhelpfully throws ;; UnsupportedOperationException. - ;; This is the same code as with Map. The code can't be shared using a - ;; function because reflection would kill the performance and DBObject - ;; and Map don't share a interface. (reduce (if keywordize (fn [m ^String k] (assoc m (keyword k) (from-db-object (.get input k) true)))