Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RuleOrderedActivation Fressian Handler #395

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix RuleOrderedActivation Fressian Handler #395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5480524
Fix RuleOrderedActivation Fressian Handler
mmotter Jun 7, 2018
3b86cf9
- implement protocol function use-token-identity? for proctol Identit…
mmotter Jun 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ This is a history of changes to clara-rules.

### 0.19.0-SNAPSHOT
* Remove a warning about `qualified-keyword?` being replaced when using Clojure 1.9.
* Fix RuleOrderedActivation Fressian Handler. See [issue 394](https://github.com/cerner/clara-rules/issues/394)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we indicate directly in the changelog that the functional impact here is very limited as discussed at #394 (comment) and #395 (review) ? I'd like to avoid unnecessary user concern. Something like "The bug is only exposed if reusing the Clara Fressian handlers outside of Clara's direct APIs"


### 0.18.0
* Remove unnecessary memory operations from ProductionNode to optimize performance. Remove :rule-matches in session inspection that did not cause logical insertions and add a new optional feature to return all rule matches, regardless of what their RHS did or whether they were retracted. Add a new listener and tracing method fire-activation!. These changes are moderately non-passive with respect to listening, tracing, and session inspection but are otherwise passive. See [issue 386](https://github.com/cerner/clara-rules/issues/386) for details.
Expand Down
1 change: 1 addition & 0 deletions CONTRIBUTORS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Cerner Corporation
- William Parker [@WilliamParker]
- Ethan Christian [@EthanEChristian]
- Pushkar Kulkarni [@kulkarnipushkar]
- Matt Motter [@mmotter]

Community

Expand Down
4 changes: 3 additions & 1 deletion src/main/clojure/clara/rules/durability/fressian.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,11 +492,13 @@
(.writeObject w (.-node-id ^RuleOrderedActivation c) true)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we recap the discussion about the non-usage of this handler in a sentence or two of comments inline here?

(.writeObject w (.-token ^RuleOrderedActivation c))
(.writeObject w (.-activation ^RuleOrderedActivation c))
(.writeInt w (.-rule-load-order ^RuleOrderedActivation c))))
(.writeInt w (.-rule-load-order ^RuleOrderedActivation c))
(.writeBoolean w (mem/use-token-identity? ^RuleOrderedActivation c))))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you have the type hint on c here? Type hints are normally used so that Clojure can resolve a Java interop call to a particular function at compile time. With protocol functions the workflow in the end is something like:

  • Clojure calls the protocol function like any other Clojure function. The function has a method like invoke(Object arg1 .. Object argN) if you look at the emitted bytecode.
  • That function searches for the implementation on the type in question; see here if you're curious.

This will actually be a bit more performant if you invoke the JVM method directly; it will be mangled a bit for the ? I think but it will exist. The reason to use the protocol function instead is if you extend the protocol outside the definition of the type, for example with extend-protocol, but that isn't the case here. If you do this you'll want the type hint. Might be good to do for the sake of the pattern although it doesn't really matter here unless a user reuses the handler in a hotspot. That sort of thing could matter a bit if you were doing it on a type here that is actually used though and it is an easy optimization to make.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hint isn't useful. I think it is just left over from when the field was being directly accessed here in the first revision of this PR (that wouldn't work though since it is a private field and that's why this change was made).

I don't think that direct interop with a protocol is going to be important here. The inline caching would likely be just about the same and this isn't a particularly "hot spot" (not even used by durability at this point and not used elsewhere since it was broken before this). Best to stay basic when it can be.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal either way as you say. I'm OK with using the protocol here if we make this change, I mainly just wanted to raise awareness for cases where it might matter.

:readers {"clara/ruleorderactivation"
(reify ReadHandler
(read [_ rdr tag component-count]
(mem/->RuleOrderedActivation (.readObject rdr)
(.readObject rdr)
(.readObject rdr)
(.readObject rdr)
(.readObject rdr))))}}
Expand Down
7 changes: 6 additions & 1 deletion src/main/clojure/clara/rules/memory.cljc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,8 @@

#?(:clj
(defprotocol IdentityComparable
(using-token-identity! [this bool])))
(using-token-identity! [this bool])
(use-token-identity? [this])))

#?(:clj
(deftype RuleOrderedActivation [node-id
Expand All @@ -340,6 +341,10 @@
(using-token-identity! [this bool]
(set! use-token-identity? bool)
this)

(use-token-identity? [_]
use-token-identity?)

Object
;; Two RuleOrderedActivation instances should be equal if and only if their
;; activation is equal. Note that if the node of two activations is the same,
Expand Down
7 changes: 6 additions & 1 deletion src/test/clojure/clara/test_fressian.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
[clara.rules.durability.fressian :as df]
[clojure.data.fressian :as fres]
[clara.rules.platform :as pform]
[clara.rules.memory :as mem]
[clojure.test :refer :all])
(:import [org.fressian
FressianWriter
Expand Down Expand Up @@ -106,5 +107,9 @@
(test-serde-with-meta sm-custom sm-custom)
(is (thrown? Exception
(serde (with-meta sm-custom {})))
"cannot serialized custom sort comparators without name given in metadata")))))
"cannot serialized custom sort comparators without name given in metadata"))))

(testing "RuleOrderActivation"
(let [act (mem/->RuleOrderedActivation 1 nil nil 1 false)]
(test-serde act act))))