diff --git a/src/main/clojure/clara/rules/compiler.clj b/src/main/clojure/clara/rules/compiler.clj index 142f9de6..bf103dec 100644 --- a/src/main/clojure/clara/rules/compiler.clj +++ b/src/main/clojure/clara/rules/compiler.clj @@ -73,6 +73,12 @@ ;; Function that takes facts and determines what alpha nodes they match. get-alphas-fn]) +(defn- is-variable? + "Returns true if the given expression is a variable (a symbol prefixed by ?)" + [expr] + (and (symbol? expr) + (.startsWith (name expr) "?"))) + (def ^:private reflector "For some reason (bug?) the default reflector doesn't use the Clojure dynamic class loader, which prevents reflecting on @@ -309,8 +315,7 @@ "Returns a set of the symbols in the given s-expression that start with '?' as keywords" [expression] (into #{} (for [item (flatten-expression expression) - :when (and (symbol? item) - (= \? (first (name item))))] + :when (is-variable? item)] (keyword item)))) (defn field-name->accessors-used @@ -386,7 +391,19 @@ (defn compile-action "Compile the right-hand-side action of a rule, returning a function to execute it." [binding-keys rhs env] - (let [assignments (mapcat #(list (symbol (name %)) (list 'get-in '?__token__ [:bindings %])) binding-keys) + (let [;; Avoid creating let bindings in the compile code that aren't actually used in the body. + ;; The bindings only exist in the scope of the RHS body, not in any code called by it, + ;; so this scanning strategy will detect all possible uses of binding variables in the RHS. + ;; Note that some strategies with macros could introduce bindings, but these aren't something + ;; we're trying to support. If necessary a user could macroexpand their RHS code manually before + ;; providing it to Clara. + rhs-bindings-used (variables-as-keywords rhs) + + assignments (sequence + (comp + (filter rhs-bindings-used) + (mapcat #(list (symbol (name %)) (list '-> '?__token__ :bindings %)))) + binding-keys) ;; The destructured environment, if any. destructured-env (if (> (count env) 0) @@ -609,12 +626,6 @@ node-type)) -(defn- is-variable? - "Returns true if the given expression is a variable (a symbol prefixed by ?)" - [expr] - (and (symbol? expr) - (.startsWith (name expr) "?"))) - (defn- extract-exists "Converts :exists operations into an accumulator to detect the presence of a fact and a test to check that count is diff --git a/src/test/common/clara/test_performance_optimizations.cljc b/src/test/common/clara/test_performance_optimizations.cljc new file mode 100644 index 00000000..0fccfebc --- /dev/null +++ b/src/test/common/clara/test_performance_optimizations.cljc @@ -0,0 +1,62 @@ +;; These tests validate that operations that the rules engine should optimize +;; away are in fact optimized away. The target here is not the actual execution time, +;; which will vary per system, but verification that the action operations in question are not performed. +#?(:clj + (ns clara.test-performance-optimizations + (:require [clara.tools.testing-utils :refer [def-rules-test + side-effect-holder] :as tu] + [clara.rules :refer [fire-rules + insert + insert! + query]] + + [clara.rules.testfacts :refer [->Cold ->ColdAndWindy]] + [clojure.test :refer [is deftest run-tests testing use-fixtures]] + [clara.rules.accumulators] + [schema.test :as st]) + (:import [clara.rules.testfacts + Cold + ColdAndWindy])) + + :cljs + (ns clara.test-performance-optimizations + (:require [clara.rules :refer [fire-rules + insert + insert! + query]] + [clara.rules.testfacts :refer [->Cold Cold + ->ColdAndWindy ColdAndWindy]] + [clara.rules.accumulators] + [cljs.test] + [schema.test :as st] + [clara.tools.testing-utils :refer [side-effect-holder] :as tu]) + (:require-macros [clara.tools.testing-utils :refer [def-rules-test]] + [cljs.test :refer [is deftest run-tests testing use-fixtures]]))) + +(use-fixtures :once st/validate-schemas #?(:clj tu/opts-fixture)) +(use-fixtures :each tu/side-effect-holder-fixture) + +#?(:clj + (defmacro true-if-binding-absent + [] + (not (contains? &env '?unused-binding)))) + +;; See issue https://github.com/cerner/clara-rules/issues/383 +;; This validates that we don't create let bindings for binding +;; variables that aren't used. Doing so both imposes runtime costs +;; and increases the size of the generated code that must be evaluated. +(def-rules-test test-unused-rhs-binding-not-bound + + {:rules [cold-windy-rule [[[ColdAndWindy (= ?used-binding temperature) (= ?unused-binding windspeed)]] + (when (true-if-binding-absent) + (insert! (->Cold ?used-binding)))]] + + :queries [cold-query [[] [[Cold (= ?c temperature)]]]] + + :sessions [empty-session [cold-windy-rule cold-query] {}]} + + (is (= [{:?c 0}] + (-> empty-session + (insert (->ColdAndWindy 0 0)) + fire-rules + (query cold-query)))))