ISSUE-393: Use Java's hashCode to improve performance in this case (#399

) ISSUE-393: Use Java's hashCode to improve performance in this special case.
cerner · Jul 17, 2018 · 204bcfa · 204bcfa
1 parent ce65c50
commit 204bcfa
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 29 deletions.
diff --git a/src/main/clojure/clara/rules/platform.cljc b/src/main/clojure/clara/rules/platform.cljc
@@ -37,7 +37,12 @@
      [f coll]
      (let [^java.util.Map m (reduce (fn [^java.util.Map m x]
                                       (let [k (f x)
-                                            wrapper (JavaEqualityWrapper. k (hash k))
+                                            ;; Use Java's hashcode for performance reasons as
+                                            ;; discussed at https://github.com/cerner/clara-rules/issues/393
+                                            wrapper (JavaEqualityWrapper. k
+                                                                          (if (nil? k)
+                                                                            (int 0)
+                                                                            (int (.hashCode ^Object k))))
                                             xs (or (.get m wrapper)
                                                    (transient []))]
                                         (.put m wrapper (conj! xs x)))

diff --git a/src/test/clojure/clara/test_rules.clj b/src/test/clojure/clara/test_rules.clj
@@ -2724,27 +2724,3 @@
                                                                :name :clara.rules.test-rules-data/is-cold-and-windy-data
                                                                :lhs  []
                                                                :rhs  '(println "I have no meaning outside of this test")}))) {})))
-
-(defrecord OuterRecordOne [x])
-(defrecord OuterRecordTwo [x])
-
-(defrecord InnerRecordOne [num])
-(defrecord InnerRecordTwo [num])
-
-;; Test for issue 393
-(deftest test-record-equality-semantics
-  (let [inner-one (->InnerRecordOne 1)
-        inner-two (->InnerRecordTwo 1)
-
-        test-query (dsl/parse-query [] [[OuterRecordOne (= ?x x)]
-                                        [OuterRecordTwo (= ?x x)]])
-
-
-        session (-> (mk-session [test-query])
-                    (insert (->OuterRecordOne inner-one)
-                            ;; Needed to reproduce the bug.
-                            (->OuterRecordTwo inner-one)
-                            (->OuterRecordTwo inner-two))
-                    (fire-rules))]
-
-    (is (= [{:?x inner-one}] (query session test-query)))))
diff --git a/src/test/common/clara/test_bindings.cljc b/src/test/common/clara/test_bindings.cljc
@@ -59,11 +59,11 @@
   (is (= @tu/side-effect-holder 10)))
 
 (def-rules-test test-simple-join-binding
-  
+
   {:rules [same-wind-and-temp [[[Temperature (= ?t temperature)]
                                 [WindSpeed (= ?t windspeed)]]
                                (reset! tu/side-effect-holder ?t)]]
-   
+
    :sessions [empty-session [same-wind-and-temp] {}]}
 
   (let [session (-> empty-session
@@ -78,7 +78,7 @@
   {:rules [same-wind-and-temp [[[Temperature (= ?t temperature)]
                                 [WindSpeed (= ?t windspeed)]]
                                (reset! tu/side-effect-holder ?t)]]
-   
+
    :sessions [empty-session [same-wind-and-temp] {}]}
 
   (let [session (-> empty-session
@@ -220,7 +220,7 @@
                (fire-rules)
                (query temps-with-addition))))
 
-    
+
     ;; Test if not all conditions are satisfied.
     (is (empty? (-> session
                     (insert (->Temperature 10 "MCI")
@@ -299,3 +299,47 @@
       "Validate that the ?t binding from the previous accumulator result is used, rather
          than the binding in the ColdAndWindy condition that would create a ?t binding if one were
          not already present"))
+
+;; Tests for issue 393
+(defrecord OuterRecordOne [x])
+(defrecord OuterRecordTwo [x])
+
+(defrecord InnerRecordOne [num])
+(defrecord InnerRecordTwo [num])
+
+(def-rules-test test-record-equality-semantics
+
+  {:queries [test-query [[] [[OuterRecordOne (= ?x x)]
+                             [OuterRecordTwo (= ?x x)]]]]
+
+   :sessions [equality-empty-session [test-query] {}]}
+
+  (let [inner-one (->InnerRecordOne 1)
+        inner-two (->InnerRecordTwo 1)
+
+        session (-> equality-empty-session
+                    (insert (->OuterRecordOne inner-one)
+                            ;; Needed to reproduce the bug.
+                            (->OuterRecordTwo inner-one)
+                            (->OuterRecordTwo inner-two))
+                    (fire-rules))]
+
+    (is (= [{:?x inner-one}] (query session test-query)))))
+
+(def-rules-test test-nil-binding
+
+  {:queries [test-query [[] [[OuterRecordOne (= ?x x)]
+                             [OuterRecordTwo (= ?x x)]]]]
+
+   :sessions [empty-session-nil-test [test-query] {}]}
+
+  (let [inner-two (->InnerRecordTwo 1)
+
+        session (-> empty-session-nil-test
+                    (insert (->OuterRecordOne nil)
+                            ;; Needed to reproduce the bug.
+                            (->OuterRecordTwo nil)
+                            (->OuterRecordTwo inner-two))
+                    (fire-rules))]
+
+    (is (= [{:?x nil}] (query session test-query)))))