Issue 386: Remove unneeded memory operations from ProductionNode #387
Conversation
| @@ -337,17 +337,17 @@ | |||
| ILeftActivate | |||
| (left-activate [node join-bindings tokens memory transport listener] | |||
|
|
|||
| ;; Provide listeners information on all left-retract calls for passivity, | |||
There was a problem hiding this comment.
Just a minor thing, but I think it is more important than just preserving old behavior. I think it is really useful behavior to be able to listen for this activity in the ProductionNode still.
| @@ -1766,6 +1767,11 @@ | |||
| ;; Therefore, the reordering of retractions and insertions should have no impact | |||
| ;; assuming that the evaluation of rule conditions is pure, which is a general expectation | |||
| ;; of the rules engine. | |||
| (l/fire-activation! listener | |||
| activation | |||
There was a problem hiding this comment.
These extra deref calls are going to incur runtime cost. That's sort of an unfortunate part of the listener calls in the engine all being functions (as opposed to macros that could try to mitigate the cost when they are "off").
In this case, I think You could just bind to the derefed values here and use them in the places you need them. They are needed outside of the l/fire-activation! call anyways.
| (defn remove-listeners | ||
| [session pred] | ||
| (let [{:keys [listeners] :as components} (components session) | ||
| matching-listeners (filterv pred listeners)] |
There was a problem hiding this comment.
I think using some here would make more sense. No reason to build a vector of all the matches that aren't used later.
| (if (seq matching-listeners) | ||
| (assemble (assoc components | ||
| :listeners | ||
| (remove pred listeners))) |
There was a problem hiding this comment.
This seems like something that'd be better off not lazy. It just seems questionable when it will be realized later.
(into [] (remove pred) listeners)
| (deftype TransientRuleActivationListener [activations] | ||
| l/ITransientEventListener | ||
| (fire-activation! [listener activation resulting-operations] | ||
| (reset! (.-activations listener) (conj @(.-activations listener) {:activation activation |
There was a problem hiding this comment.
This is a slower version of swap! I believe
(swap! (.-activations listener) conj {:activation activation :resulting-operations resulting-operations})
| (fire-rules! [listener node] | ||
| listener)) | ||
|
|
||
| (deftype PersistentRuleActivationListener [activations] |
There was a problem hiding this comment.
I wonder if "rule" is needed in these names. Could it all just be "activation listener" instead of "rule activation listener"?
| (remove pred listeners))) | ||
| session))) | ||
|
|
||
| (defn listeners-matching-pred |
There was a problem hiding this comment.
Perhapsfind-listeners?
|
I've pushed some more commits up that address previous comments and generally clean this up. I'll want to go over it once more before merging but my objective was to clean it up to a mergeable state. I think we can delay adding more keys to session inspection to a future PR. |
|
+1 to this change. (And thanks to Will for giving me a heads up here.) |
This is an incomplete draft but demonstrates the design of the changes.
TODOs: