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.

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

Remove unused let-bindings in compiled RHS action functions #383

Closed
WilliamParker opened this issue Feb 12, 2018 · 3 comments
Closed

Remove unused let-bindings in compiled RHS action functions #383

WilliamParker opened this issue Feb 12, 2018 · 3 comments

Comments

@WilliamParker
Copy link
Collaborator

Take the following rule:

(defrule cold-rule
        [ColdAndWindy (= ?temperature temperature) (= ?windspeed windspeed)]
        =>
        (insert! (->Cold ?temperature)))

When we create a session with this rule and inspect the compiled RHS function, noting that the the unevaluated expression is added as metadata to the ProductionNode for later use in durability :

(-> (mk-session [cold-rule]) .rulebase :alpha-roots (get ColdAndWindy) first :children first :children first meta)

we retrieve the following:

{:action-expr
 (clojure.core/fn
  [?__token__ ?__env__]
  (clojure.core/let
   [?temperature
    (get-in ?__token__ [:bindings :?temperature])
    ?windspeed
    (get-in ?__token__ [:bindings :?windspeed])]
   (do (insert! (->Cold ?temperature)))))}

Note that a let-binding for ?windspeed is created even though it is not used in the user's RHS code. It does not appear that the code which creates these RHS let-binding makes any attempt to determine what bindings are used in the RHS. As a result unnecessary bindings are created, consuming time to create at runtime and increasing the amount of Clojure code evaluated at session creation time, thus likely increasing total session compilation time.

In order to fix this we can apply a similar fix to what was done in #180 for let bindings of fields that aren't actually used in constraints. We'd basically just walk the user's RHS code and detect what bindings are present in it. Since the bindings have to be literals in the code (rather than in outside user functions called by the RHS, where they aren't resolvable by Clojure's compiler) this can be done safely.

@WilliamParker
Copy link
Collaborator Author

I've created a preliminary PR for this at #384. Please don't merge it yet though.

Further items to address:

  • Is eliminating support for macros that would splice in bindings to the rule RHS a problem? This seems like a highly abusive pattern but I think it is currently possible. Note that it isn't supported in the LHS though after the similar changes for Only accessed used fields in constraints #180. This probably isn't a big deal but I wanted to point it out.
  • Better quantified benchmarks. My preliminary tests show nontrivial impacts on bytecode size from additional unused let-bindings in toy examples but getting hard numbers would be better.

@WilliamParker WilliamParker self-assigned this Feb 27, 2018
@mrrodriguez
Copy link
Collaborator

@WilliamParker

Is eliminating support for macros that would splice in bindings to the rule RHS a problem?

It sounds like a nasty pattern in the RHS to have macros result in bindings used like that. If someone is doing that, I think having to do the workaround would be appropriate.
I really think it'd be best in general, due to code generation size and bloat to keep most complex code forms isolated to natural functions that just take the bindings as arguments and return facts to insert/retract.

Doing a bunch of (crazy) operations inline should be considered an anti-pattern. It also makes it more difficult to do optimizations like done here.

Better quantified benchmarks. My preliminary tests show nontrivial impacts on bytecode size from additional unused let-bindings in toy examples but getting hard numbers would be better.

Do you have any sort of rough numbers on that? I'm curious what you've seen.

@WilliamParker
Copy link
Collaborator Author

Examples like this produce 1023 byte class .class files on my machine, using the *compile-files* option to force dumping of the generated bytecode to disk.

(def simple-form `(fn [a#] (+ a# 1)))

Examples like the following produce 2444 byte .class files on my machine.

(def complex-form `(fn [a#] (let [b# (:b a#) c# (:c a#) d# (:d a#) e# (:e a#)] (+ a# 1))))

Getting consistent eval time comparisons is proving difficult, but it doesn't seem likely that simply removing code that doesn't interact with anything else in the body being evaluated would increase the time to eval the remaining code. In any case reducing the bytecode size is beneficial in itself.

I've merged the PR. I don't think there is anything left to do here but we can reopen if others disagree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants