-
Notifications
You must be signed in to change notification settings - Fork 115
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.
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
Issue #381 Reduce the number of Eval calls #396
Changes from 8 commits
9a14587
9f995cc
3cf249e
6aa5781
df30ddf
bc6cb19
ef22488
b0f07f3
4f2d8c2
4988d57
4ae6611
a5a5064
5d6e8c2
ce9a2db
d24f8a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -196,10 +196,11 @@ | |
(sc/defn ^:always-validate compile-alpha-nodes | ||
[alpha-nodes :- [schema/AlphaNode]] | ||
(vec | ||
(for [{:keys [condition beta-children env]} alpha-nodes | ||
(for [{:keys [id condition beta-children env]} alpha-nodes | ||
:let [{:keys [type constraints fact-binding args]} condition]] | ||
|
||
{:type (com/effective-type type) | ||
{:id id | ||
:type (com/effective-type type) | ||
:alpha-fn (com/compile-condition type (first args) constraints fact-binding env) | ||
:children (vec beta-children)}))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems odd to me that we build a map here and then use it to construct alpha nodes later in assemble-session while building the beta nodes directly in this namespace, but changing that is probably out of scope here. Maybe there's a reason for this that I'm not remembering offhand. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that is a little weird the only thing i think that is forcing this is, the access to the compiled beta nodes. At this point the beta-children are just ids of the nodes that will be constructed later. |
||
|
||
|
@@ -209,11 +210,14 @@ | |
;;; as a ClojureScript DSL may call productions->session-assembly-form if that DSL | ||
;;; has its own mechanism for grouping rules which is different than the clara DSL. | ||
(com/validate-names-unique productions) | ||
(let [beta-graph (com/to-beta-graph productions) | ||
(let [id-counter (atom 0) | ||
create-id-fn (fn [] (swap! id-counter inc)) | ||
|
||
beta-graph (com/to-beta-graph productions create-id-fn) | ||
;; Compile the children of the logical root condition. | ||
beta-network (gen-beta-network (get-in beta-graph [:forward-edges 0]) beta-graph #{}) | ||
|
||
alpha-graph (com/to-alpha-graph beta-graph) | ||
alpha-graph (com/to-alpha-graph beta-graph create-id-fn) | ||
alpha-nodes (compile-alpha-nodes alpha-graph)] | ||
|
||
`(let [beta-network# ~beta-network | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this live in the main Clara project seems reasonable to me, but there is an existing (old) clara-benchmark project. FYI @rbrush in case you have suggestions, objections, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favor of including this in the main Clara project. The test selectors are a nice solution to performance concerns here.