weavejester / medley

A lightweight library of useful Clojure functions
Eclipse Public License 1.0
865 stars 66 forks source link

Cleanup for partition fns & interleave-all, perf fix for assoc-some #68

Closed tomdl89 closed 1 year ago

tomdl89 commented 1 year ago

I haven't squashed this PR as the commits are not much related. They could be separate PRs but I thought it may be easier as one. lmk if 3 would be preferable.

weavejester commented 1 year ago

Thanks for the PR. Keeping the changes as separate commits is good, as they are distinct improvements. We can probably do this all as one PR under the subject of performance improvements and refactoring.

Can you edit the commit messages so that the subject (i.e. the first line) is <= 50 characters (see the seven rules of a great Git commit message as mentioned in CONTRIBUTING.md).

Can you also summarize the Criterium results. Something like:

Improve polyadic performance of assoc-some

Tests with Criterium suggest a ~6x performance increase.

When looking back through the commit log, too much information can be almost as bad as too little. In this case the relevant result is that it is significantly more performant. If we add the Criterium results to the PR as a comment, then a reader of the commit log can check the merge commit and investigate the PR for more information.

tomdl89 commented 1 year ago

Criterirum results for assoc-some's polyadic form:

medley.core> (def alphabet '[a b c d e f g h i j k l m n o p q r s t u v w x y z])
medley.core> (def test-map (zipmap alphabet (range)))
medley.core> (def test-vec (interleave (shuffle alphabet) (repeatedly #(rand-nth [true nil]))))

medley.core> (c/bench (apply assoc-some test-map test-vec))
Evaluation count : 5050680 in 60 samples of 84178 calls.
             Execution time mean : 11.964262 µs
    Execution time std-deviation : 210.333955 ns
   Execution time lower quantile : 11.768898 µs ( 2.5%)
   Execution time upper quantile : 12.588260 µs (97.5%)
                   Overhead used : 4.176754 ns

Found 4 outliers in 60 samples (6.6667 %)
    low-severe   1 (1.6667 %)
    low-mild     3 (5.0000 %)
 Variance from outliers : 6.2922 % Variance is slightly inflated by outliers
nil

medley.core> (c/bench (apply assoc-some-new test-map test-vec))
Evaluation count : 30155580 in 60 samples of 502593 calls.
             Execution time mean : 2.026170 µs
    Execution time std-deviation : 28.266784 ns
   Execution time lower quantile : 1.980817 µs ( 2.5%)
   Execution time upper quantile : 2.073131 µs (97.5%)
                   Overhead used : 4.176754 ns

Found 1 outliers in 60 samples (1.6667 %)
    low-severe   1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil
tomdl89 commented 1 year ago

Sorry I missed you reply before. Should all be done. Let me know if there's anything else needed.