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

Pick fn arity independent of written order #532

Merged
merged 2 commits into from
Feb 17, 2021

Conversation

GreshamDanielStephens
Copy link
Contributor

In pure clojure the fn arity chosen seems to be

  1. The matching non-variadic fn first
  2. Otherwise the matching variadic fn
((fn ([x & xs] "variadic") ([x] "otherwise")) 1)
=> "otherwise"
((fn ([x] "otherwise") ([x & xs] "variadic")) 1)
=> "otherwise"

Whereas in sci, the choice is based on the order that the arities are written in

((fn ([x & xs] "variadic") ([x] "otherwise")) 1)
=> "variadic"
((fn ([x] "otherwise") ([x & xs] "variadic")) 1)
=> "otherwise"

This fixes the issue by actively ordering the different fn arities to choose variadic arities last, thereby preferring exact matches.

@borkdude
Copy link
Collaborator

@GreshamDanielStephens Thanks. I agree with the issue, but your implementation will sort on every function invocation, which can be improved by moving the sort outside of the function body.

Potentially we can improve things even more by making a map of fixed-arity -> fn for the fixed arities and invoking the varargs body if it can't be found.

@GreshamDanielStephens
Copy link
Contributor Author

ahh very true, I'll take a look, thanks for the feedback.

@GreshamDanielStephens
Copy link
Contributor Author

updated with the map for fixed arities, and falling back on checking if the variadic-fn is viable.
I've force pushed since it's a small change and not much of the original remains, I hope that's okay.

@@ -242,10 +251,14 @@
single-arity? (= 1 (count fn-bodies))
f (if single-arity?
(fun ctx interpret (first fn-bodies) fn-name macro? false)
(let [arities (map #(fun ctx interpret % fn-name macro? true) fn-bodies)]
(let [arities (map #(fun ctx interpret % fn-name macro? true) fn-bodies)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@GreshamDanielStephens Can we maybe collapse all of this processing into one reduce for more performance?

(let [by-arity (reduce (fn [acc body] (assoc acc ...)) fn-bodies)

We can reserve a special key for the var-args body or return a tuple from the reduce.

@GreshamDanielStephens
Copy link
Contributor Author

Reworked it a bit and I think it's looking better now, hope that's what you had in mind!

@borkdude
Copy link
Collaborator

@GreshamDanielStephens I did some perf measurements on master and this branch:

$ clj -J-Dclojure.compiler.direct-linking=true
user=> (time (sci/eval-string "(defn foo ([a b] b) ([a] a) ([a b c d] d)) (dotimes [i 10000000] (foo 1 2 3 4))"))

"Elapsed time: 9074.377857 msecs"
user=> (time (sci/eval-string "(defn foo ([a b] b) ([a] a) ([a b c d] d)) (dotimes [i 10000000] (foo 1 2 3 4))"))
"Elapsed time: 6403.763242 msecs"

So that seems to be nice speed up.

@borkdude borkdude merged commit cdd500b into babashka:master Feb 17, 2021
borkdude added a commit that referenced this pull request Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants