-
-
Notifications
You must be signed in to change notification settings - Fork 89
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
Pick fn arity independent of written order #532
Conversation
@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. |
ahh very true, I'll take a look, thanks for the feedback. |
632502d
to
4b2861a
Compare
updated with the map for fixed arities, and falling back on checking if the variadic-fn is viable. |
src/sci/impl/fns.cljc
Outdated
@@ -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) |
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.
@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.
Reworked it a bit and I think it's looking better now, hope that's what you had in mind! |
da79415
to
1f5ffa4
Compare
@GreshamDanielStephens I did some perf measurements on master and this branch:
So that seems to be nice speed up. |
In pure clojure the fn arity chosen seems to be
Whereas in sci, the choice is based on the order that the arities are written in
This fixes the issue by actively ordering the different fn arities to choose variadic arities last, thereby preferring exact matches.