Issue 344: Add a function for reporting potential performance problem… #416
Conversation
…s in a traced session
| (map second)) | ||
| (eng/get-conditions-and-rule-names node)))) | ||
|
|
||
| (defn traced-session->ranked-productions |
There was a problem hiding this comment.
It might also be interesting to have something similar to pair with this. Once we have the production name it might be nice to be able to easily get the nodes and number of interactions that make up a specific production. This could probably be added in the future if we see the need though.
There was a problem hiding this comment.
Agreed, there's lots of interesting views on the tracing data that one could provide.
| (map second)) | ||
| (eng/get-conditions-and-rule-names node)))) | ||
|
|
||
| (defn traced-session->ranked-productions |
There was a problem hiding this comment.
I think I'd probably prefer the name to just beranked-productions.
I'm not sure there is a reason to express "conversion" that the x->y tends to imply.
I don't 100% agree [1] with the suggestions of this post [2], but do think it has some fairly good guidance in this regard
[1] It does mention
There are exceptions to every rule.
So it covers that base.
[2] https://stuartsierra.com/2016/01/09/how-to-name-clojure-functions
There was a problem hiding this comment.
That name is OK with me, will change.
| that this will provide useful information for a first pass at rules | ||
| performance problem debugging. This should not be used to drive user logic. | ||
|
|
||
| This currently returns a Clojure array map currently in order to conveniently have the rules |
There was a problem hiding this comment.
"currently" is repeated in this sentence.
There was a problem hiding this comment.
+1 good catch, will change
| @@ -114,3 +114,48 @@ | |||
| (first))] | |||
| (.-trace ^PersistentTracingListener listener) | |||
| (throw (ex-info "No tracing listener attached to session." {:session session})))) | |||
|
|
|||
| (defn ^:private node-id->productions | |||
There was a problem hiding this comment.
Similar to my comment for traced-session->ranked-productions,
I'd prefer node-productions since this doesn't seem like a "conversion" operation.
It's subjective of course.
There was a problem hiding this comment.
Perhaps "productions-for-id"? In any case this is an internal private function so it probably doesn't matter much.
| "Given a session and a node ID return a list of the rule and query names associated | ||
| with the node." | ||
| [session id] | ||
| (let [node (-> session .-rulebase :id-to-node (get id))] |
There was a problem hiding this comment.
It'd be preferable to remove the reflective access here. It doesn't matter perf-wise in this case of course, but the less reflection warnings in the codebase the less noise there is to find the important ones.
There was a problem hiding this comment.
We can just use the components function on the session actually and avoid direct field access entirely. It's better to use the protocols on the session anyway in case there was another implementation at some point. https://github.com/cerner/clara-rules/blob/0.19.0/src/main/clojure/clara/rules/engine.cljc#L1990
|
|
||
| production-name->interactions (frequencies production-names) | ||
|
|
||
| ranked-tuples (reverse (sort-by second production-name->interactions))] |
There was a problem hiding this comment.
The reverse could be avoided by using the 3 arity sort-by.
I believe that'd be
ranked-tuples (sort-by second > production-name->interactions)or could generalize as
ranked-tuples (sort-by second #(compare %2 %1) production-name->interactions)There was a problem hiding this comment.
Agreed that that would work. IMO this is a little easier to understand though and performance doesn't seem like a concern here. I'm inclined to leave it as is.
|
|
||
| ranked-tuples (reverse (sort-by second production-name->interactions))] | ||
|
|
||
| (apply array-map (into [] cat ranked-tuples)))) |
There was a problem hiding this comment.
Side note: It could be a sorted-map-by with the value-info tied to metadata or something like that.
However, this is simpler and seems fine for now.
There was a problem hiding this comment.
Yeah, that could work if we used symbols as the keys. Since strings don't support metadata you'd need to do something more complicated involving storing the "metadata" elsewhere and make sure you didn't create a memory leak in the process. Alternatively the keys could be symbols instead. We'd also need to make sure the compared value was never equal, perhaps by adding a tiebreak on alphabetical order, since sorted-map-by will consider equal values on the comparator to be equal keys.
|
I'm going to go ahead and merge this; my last commit addressed review comments. If there's anything outstanding to address I'm open to making further changes but it seems like we've covered everything of importance and this is not a change to the core compiler/engine/memory code demanding higher scrutiny. |
…s in a traced session
Pull request for issue 344
@mrrodriguez this is one of the tracing helpers we've discussed before.
@EthanEChristian this is probably of interest to you. Possibly @eraserhd as well if I recall some discussions on the Slack channel in recent months correctly.
Some remaining items to address: