Issue #402: [performance] Remove usage of get-in in compiled rules #404
Conversation
|
Further explanation of my comment above: The clj test runs in ~400ms the cljs test runs in ~490000ms |
| @@ -378,10 +378,16 @@ | |||
| ~'?__bindings__ (atom ~initial-bindings)] | |||
| ~(compile-constraints constraints))))) | |||
|
|
|||
| (defn build-token-assingment | |||
| "A helper function to build variable assignment forms for tokens." | ||
| [binding-key] | ||
| (list (symbol (name binding-key)) | ||
| (list '-> '?__token__ :bindings binding-key))) |
There was a problem hiding this comment.
It's safer to refer to the fully qualified symbol clojure.core/-> here.
You can do that by using the syntax quote instead
`->
There was a problem hiding this comment.
+1 ; this is in case someone defines their own -> in the namespace of the production. We'd still want this to be the clojure.core version.
|
|
|
||
| (def-rules-test test-get-in-perf | ||
| {:rules [rule [[[?parent <- ParentFact] | ||
| [?as <- (acc/all) :from [AFact (contains? (:a-ids ?parent) id)]] |
There was a problem hiding this comment.
Why do we have the contains? calls here? If the idea is to measure the performance of Clara I think we'd want to remove irrelevant work (i.e. user code from the perspective of the rules engine) as much as possible.
|
|
||
| (def fire-rules-counter (atom 0)) | ||
|
|
||
| (def-rules-test perfomance-test |
There was a problem hiding this comment.
Minor: since it is a test and the thing under test is named "test", I think "test-performance-test" would be a more clear name. Also "performance" not "perfomance"
| "A helper function to build variable assignment forms for tokens." | ||
| [binding-key] | ||
| (list (symbol (name binding-key)) | ||
| (list '-> '?__token__ :bindings binding-key))) |
There was a problem hiding this comment.
+1 ; this is in case someone defines their own -> in the namespace of the production. We'd still want this to be the clojure.core version.
| (defn build-token-assingment | ||
| "A helper function to build variable assignment forms for tokens." | ||
| [binding-key] | ||
| (list (symbol (name binding-key)) |
There was a problem hiding this comment.
You could experiment with accessing the :bindings by the literal field as well if this is a performance hotspot. I don't know if it will matter in any siginificant way, but the keyword lookup does have overhead versus a Java field member access at least on the JVM. I think (not sure offhand) the syntax for that to work in both clj and cljs is .-fieldname.
There was a problem hiding this comment.
Note: not needed for merge, just an observation/food for thought.
| @@ -0,0 +1,39 @@ | |||
| (ns clara.performance.test-rule-execution | |||
There was a problem hiding this comment.
Why is this Clojure (JVM)-only?
There was a problem hiding this comment.
I put this in a JVM only location because the cljs test took 490 seconds, and it didn't seem that it would provide much value.
There was a problem hiding this comment.
I think that this is the only outstanding comment, @WilliamParker do you feel strongly that this should also run for CLJS?
There was a problem hiding this comment.
Not particularly strongly. My suggestion would be to use conditionals to set the number of facts lower in cljs to a point where it runs in a reasonable amount of time then perhaps log a performance improvement issue to address it. Maybe someone more knowledgeable about cljs perf will take a look. I find it a bit concerning that the cljs runtime is that high but that shoudn't block this as it won't make it worse.
There was a problem hiding this comment.
Agreed that this is mergeable @EthanEChristian . I made a minor suggestion on the cljs perf tests but that doesn't need to block this. Sorry for the delay on a simple question, I've been busy over the last week.
| @@ -0,0 +1,39 @@ | |||
| (ns clara.performance.test-rule-execution | |||
There was a problem hiding this comment.
Not particularly strongly. My suggestion would be to use conditionals to set the number of facts lower in cljs to a point where it runs in a reasonable amount of time then perhaps log a performance improvement issue to address it. Maybe someone more knowledgeable about cljs perf will take a look. I find it a bit concerning that the cljs runtime is that high but that shoudn't block this as it won't make it worse.
|
@WilliamParker I have updated the PR to cover the changes requested, if there is nothing further i will try to get around to merging this one. |
|
This looks fine to merge @EthanEChristian We should probably set up a separate ClojureScript performance build but that's a task for another time. |
|
Logged #406 to setup the cljs build for performance. |
I have incorporated some of the performance test helpers created with #381, I placed them where both cljs and clj could leverage them. However running benchmarks on cljs on the scale of some of the clj benchmarks is considerably worse, thus i have created a benchmark only on the clj side.
I am not really sure how to go about profiling cljs, but it might be worth looking into how we could improve the execution of rules.