clj-commons / secretary

A client-side router for ClojureScript.
773 stars 64 forks source link

Remove use of merge-with with seq of seqs #101

Closed rsslldnphy closed 5 years ago

rsslldnphy commented 5 years ago

This is no longer supported in the latest Clojurescript.

Fixes #100.

mfikes commented 5 years ago

This is a cool solution. The one being used here appears to be faster, with these speedups:

            V8: 1.48
  SpiderMonkey: 1.73
JavaScriptCore: 1.91

Tested with ClojureScript 1.10.439 under :advanced comparing

(simple-benchmark [data (->> (interleave [1 2 2 3] [7 8 9 10]) (partition 2))] (->> data (map (partial apply hash-map)) (apply merge-with vector {})) 1e6)

and

(simple-benchmark [data (->> (interleave [1 2 2 3] [7 8 9 10]) (partition 2))] (->> data (map (fn [[k v]] (first {k v}))) (merge-with vector {})) 1e6)

(It is not clear to me whether this code even needs to be performant; I'm not sure when it executes.)

borkdude commented 5 years ago

@mfikes this is route matching code, so each time the address bar changes

rsslldnphy commented 5 years ago

Thanks @mfikes! Yes this is definitely a case where I'd optimise for expression of intent over performance but... to be honest I'm not sure which version does that better. I have no strong opinion so would be happy to change the implementation to whichever version should it look likely this gets merged.

The only other possibly relevant difference I can see is that the original and the more performant versions seem to rely on the fact that merge-with can be called with, instead of a map, a seq of MapEntrys which may contain duplicate keys (and in fact are expected to contain duplicate keys, hence the use of merge-with). A cursory look at https://clojuredocs.org/clojure.core/merge-with suggests this isn't documented behaviour. Should we be relying on it?

borkdude commented 5 years ago

It's guesswork, but I think something like this should be supported by merge-with:

(merge-with + {:a 1} (filter (comp odd? val) {:a 3 :b 6}))

Expressed in spec: https://github.com/slipset/speculative/blob/9ee83b1abcb04c2b917ca36ad8d11271c1b22918/src/speculative/core.cljc#L113

borkdude commented 5 years ago

This has been fixed in 1.2.4