weavejester / medley

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

Modify dissoc-in to accept variable args #25

Closed rackdon closed 6 years ago

rackdon commented 6 years ago

This pr solves issue #23 Through this change we can use the dissoc-in function like in the following example

(medley.core/dissoc-in {:a {:b 3 :c 4 :d 5}} [:a :b] [:a :c])
weavejester commented 6 years ago

You need to keep the original [m ks] arity around for performance purposes. Also, can you run a few criterium benchmarks before and after?

weavejester commented 6 years ago

Also, is reduce going to be faster, or is an explicit recur?

rackdon commented 6 years ago

Why do you say it? You need to keep the original [m ks] arity around for performance purposes. I mean, I save that inside each iteration. On the other hand I'll obtain the different benchmarks using reduce and recur because I think reduce is faster but I have to ensure that

weavejester commented 6 years ago

In your changes, you've removed the [m ks] arity, which means that if someone runs (dissoc-in m ks) it will fall to the [m & ks] arity instead. This means that Clojure needs to wrap ks in a seq, and then perform a reduce on it. We can avoid both of these things by having an explicit [m ks] arity.

rackdon commented 6 years ago

With the following element {:a {:b {:c 1} :d 2} :b {:c {:d 2 :e 3}}} using dissoc-in with [:a :b :c] obtaining the average value in miliseconds over 1000000 iterations and executing five times each trial.

old dissoc in -> 0.005182, 0.005265, 0.005219, 0.005197, 0.005223 new dissoc in with reduce -> 0.006803, 0.006727, 0.006853, 0.007013, 0.006691 new dissoc in with loop recur -> 0.005548, 0.005634, 0.005491, 0.005542, 0.005499

With the following element {:a {:b {:c 1} :d 2} :b {:c {:d 2 :e 3}}} using dissoc-in with [:a :b :c] [:b :c :d] obtaining the average value in miliseconds over 1000000 iterations and executing five times each trial.

new dissoc in with reduce -> 0.012029, 0.011852, 0.011736, 0.011868, 0.011746 new dissoc in with loop recur -> 0.011544, 0.011476, 0.011493, 0.011362, 0.01154

weavejester commented 6 years ago

Thanks for the tests. It looks like it's more performant to use a loop-recur, though if you could use Criterium's criterium.core/quick-bench function instead, that will give you more accurate results.

weavejester commented 6 years ago

I think you've misunderstood me. I was suggesting something like:

(defn dissoc-in
  ([m ks] 
   (if-let [[k & ks] (seq ks)]
     (if (seq ks)
       (let [v (dissoc-in (get m k) ks)]
         (if (empty? v)
           (dissoc m k)
           (assoc m k v)))
       (dissoc m k))
     m))
  ([m ks & kss] 
   (if-let [[ks' & kss] (seq kss)]
     (recur (dissoc-in m ks) ks' kss)
     (dissoc-in m ks))))
weavejester commented 6 years ago

Thanks! Could you squash your commits? Perhaps into something like:

Add variable arity to dissoc-in function
rackdon commented 6 years ago

done

weavejester commented 6 years ago

Thanks! Can you update the commit message, too?

rackdon commented 6 years ago

done