Making rule engine execution order more deterministic #207
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Looks good to me after an early review. I'll merge later today unless there are unexpected problems. |
|
I'll plan to look at this today. |
| ;; by the :node-id of their :children. | ||
| new-nodes (sort-by #(mapv :node-id (:children %)) | ||
| (into [] | ||
| (comp (map #(get (get merged-rules :alpha-roots) %)) |
There was a problem hiding this comment.
nit: I think this might be a bit more readable if you let-bound
(get merged-rules :alpha-roots)
|
+1 apart from my comments. |
mrrodriguez
force-pushed
the
stable-inserts
branch
from
b90c1a4 to
21be09b
Compare
|
I've addressed all of the review comments made by @WilliamParker and updated this PR |
- Added `platform/group-by-seq` to (mostly) replace `platform/tuned-group-by` - Made ancestors-fn contribution to the alphas-fn map behave deterministically
mrrodriguez
force-pushed
the
stable-inserts
branch
from
21be09b to
7839e1d
Compare
|
+1 |
|
Merged. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Clara currently uses
clara.rules.platform/tuned-group-byin a number of places. The grouping that this function does is unordered and often can vary from one JVM to the next. In particular I've noticed this happening in theclara.rules.engine/flush-updateswhich was causing non-deterministic insertion order to happen across rules. This makes it harder for us to track bad performance cases since they keep fluctuating as we run in different processes.A side-effect of
flush-updatesnot behaving deterministically was that insertion order across rules of the same salience group level were not deterministic and didn't necessarily respect rule load order as was implemented in #192. From my preliminary testing of this, I see a slight improvement in our execution times with this change in place. This is likely due to the rule order now also being the insertion order used. I've demonstrated this with a test that fails before this change and now passes.My proposal is to (on the Java CLJ side) introduce a new
clara.rules.platform/group-by-seqfunction that replaces the usages ofplatform/tuned-group-by. Currently, I've leftplatform/tuned-group-byin case it ends up having some usages later. It is also used in one CLJS place still, which I'm not wanting to change with this.platform/group-by-seqreturns a seq of tuples, which is the same as seq'ing over the map returned byplatform/tuned-group-bybefore. In all cases where we usedplatform/tuned-group-bywe didn't use the map, but rather a seq over it.I use a java.util.LinkedHashMap to maintain insertion order while building the groupings. Then I convert this into an immutable seq structure. If I tried to retain the usage of a map here, I'd have to return the java.util.LinkedHashMap or come up with some (expensive) way to port it into a Clojure sorted map. This was not worthwhile to me, since we do not need a map anyways. A java.util.LinkedHashMap would be harder to work with across the rest of the codebase since there are issues with the default Clojure seq implementations across mutable object iterators http://dev.clojure.org/jira/browse/CLJ-1738 that I see coming up when running this on JDK6 (oddly not on JDK8, which makes no sense and I want to investigate more for my own benefit).
Along with this, since I was messing with the
clara.rules.compiler/create-get-alphas-fnalready, I sorted the result of the ancestors-fn so that it would behave deterministically. This would be more of an edge case, but since we cache this result and infrequently should hit this path, I figured it'd still be worthwhile.