Why do ClojureScript atoms not implement full protocol?

Question

The swap! function, one of the most idiomatic tools in the Clojure toolbox, does instance? checking. We are told in programming to avoid implementing conditionals around type checking, to prefer polymorphism (protocols). It seems odd that ClojureScript does not implement the ISwap protocol directly against atoms and does instead in the public swap! api falling back on the protocol only after checking if the subject is an atom.

I assume this tactic must have been used for performance reasons since atoms are the primary use case for swap! and numerous other atomic methods. Is this right?

I would have preferred to implement an atom's api as part of the actual protocol so that this sort of thing would have been unnecessary.

(defn swap!
  "Atomically swaps the value of atom to be:
  (apply f current-value-of-atom args). Note that f may be called
  multiple times, and thus should be free of side effects.  Returns
  the value that was swapped in."
  ([a f]
     (if (instance? Atom a)
       (reset! a (f (.-state a)))
       (-swap! a f)))
  ([a f x]
     (if (instance? Atom a)
       (reset! a (f (.-state a) x))
       (-swap! a f x)))
  ([a f x y]
     (if (instance? Atom a)
       (reset! a (f (.-state a) x y))
       (-swap! a f x y)))
  ([a f x y & more]
     (if (instance? Atom a)
       (reset! a (apply f (.-state a) x y more))
       (-swap! a f x y more))))

Answer 1

It looks like it is performance related: http://dev.clojure.org/jira/browse/CLJS-760

Add an IAtom protocol with a -reset! method and a fast path for Atom in cljs.core/reset!.

See jsperf here - http://jsperf.com/iatom-adv

Latest chrome and firefox versions suffer ~20-30% slowdown. Older firefox versions suffer up to 60-70%.

Further down the ticket, it was decided to split IAtom into two protocols: IReset and ISwap. But this was the implementation that David went with, which does the type checking, and I imagine is was done to get the speed back up.

Unfortunately, it's not clear why Atom wasn't made to implement IReset (and ISwap) for that matter, nor why those things weren't looked for instead. And it's not clear how the original patch worked either. It basically took the implementation of reset! and put it under an instance check, and added the -reset! path for it:

diff --git a/src/cljs/cljs/core.cljs b/src/cljs/cljs/core.cljs
index 9fed929..c6e41ab 100644
--- a/src/cljs/cljs/core.cljs
+++ b/src/cljs/cljs/core.cljs
@@ -7039,6 +7039,9 @@ reduces them without incurring seq initialization"

 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; Reference Types ;;;;;;;;;;;;;;;;

+(defprotocol IAtom
+  (-reset! [o new-value]))
+
 (deftype Atom [state meta validator watches]
   IEquiv
   (-equiv [o other] (identical? o other))
@@ -7088,14 +7091,16 @@ reduces them without incurring seq initialization"
   "Sets the value of atom to newval without regard for the
   current value. Returns newval."
   [a new-value]
+  (if (instance? Atom a)
     (let [validate (.-validator a)]
       (when-not (nil? validate)
-      (assert (validate new-value) "Validator rejected reference state")))
+        (assert (validate new-value) "Validator rejected reference state"))
       (let [old-value (.-state a)]
         (set! (.-state a) new-value)
         (when-not (nil? (.-watches a))
-      (-notify-watches a old-value new-value)))
-  new-value)
+          (-notify-watches a old-value new-value))
+        new-value))
+    (-reset! a new-value)))

 (defn swap!
   "Atomically swaps the value of atom to be:

This was committed in 33692b79a114faf4bedc6d9ab38d25ce6ea4b295 (or at least something very close to it). And then the other protocol changes were done in 3e6564a72dc5e175fc65c9767364d05af34e4968:

commit 3e6564a72dc5e175fc65c9767364d05af34e4968
Author: David Nolen <[email protected]>
Date:   Mon Feb 17 14:46:10 2014 -0500

    CLJS-760: break apart IAtom protocol into IReset & ISwap

diff --git a/src/cljs/cljs/core.cljs b/src/cljs/cljs/core.cljs
index 25858084..e4df4420 100644
--- a/src/cljs/cljs/core.cljs
+++ b/src/cljs/cljs/core.cljs
@@ -7061,9 +7061,12 @@ reduces them without incurring seq initialization"

 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; Reference Types ;;;;;;;;;;;;;;;;

-(defprotocol IAtom
+(defprotocol IReset
   (-reset! [o new-value]))

+(defprotocol ISwap
+  (-swap! [o f] [o f a] [o f a b] [o f a b xs]))
+
 (deftype Atom [state meta validator watches]
   IEquiv
   (-equiv [o other] (identical? o other))
@@ -7124,21 +7127,33 @@ reduces them without incurring seq initialization"
         new-value))
     (-reset! a new-value)))

+;; generic to all refs
+;; (but currently hard-coded to atom!)
+(defn deref
+  [o]
+  (-deref o))
+
 (defn swap!
   "Atomically swaps the value of atom to be:
   (apply f current-value-of-atom args). Note that f may be called
   multiple times, and thus should be free of side effects.  Returns
   the value that was swapped in."
   ([a f]
-     (reset! a (f (.-state a))))
+     (if (instance? Atom a)
+       (reset! a (f (.-state a)))
+       (-swap! a (deref a))))
   ([a f x]
-     (reset! a (f (.-state a) x)))
+     (if (instance? Atom a)
+       (reset! a (f (.-state a) x))
+       (-swap! a (f (deref a) x))))
   ([a f x y]
-     (reset! a (f (.-state a) x y)))
-  ([a f x y z]
-     (reset! a (f (.-state a) x y z)))
-  ([a f x y z & more]
-     (reset! a (apply f (.-state a) x y z more))))
+     (if (instance? Atom a)
+       (reset! a (f (.-state a) x y))
+       (-swap! a (f (deref a) x y))))
+  ([a f x y & more]
+     (if (instance? Atom a)
+       (reset! a (apply f (.-state a) x y more))
+       (-swap! a (f (deref a) x y more)))))

 (defn compare-and-set!
   "Atomically sets the value of atom to newval if and only if the
@@ -7149,13 +7164,6 @@ reduces them without incurring seq initialization"
     (do (reset! a newval) true)
     false))

-;; generic to all refs
-;; (but currently hard-coded to atom!)
-
-(defn deref
-  [o]
-  (-deref o))
-
 (defn set-validator!
   "Sets the validator-fn for an atom. validator-fn must be nil or a
   side-effect-free fn of one argument, which will be passed the intended

It doesn't help that the ticket is dual-natured: performance is a problem (though not clear in what way: "atoms are not operating fast enough", or "other things using reset! are not running fast enough"?) and a design issue ("we want an IAtom protocol"). I think the issue was that other implementations would have to suffer the validation and notifying watchers costs even if they aren't really atoms. I wish it were clearer.

One thing I've not liked about the commits in Clojure/Script is that they are not very descriptive. I wish they were more kernel-like with appropriate background information so that folks (like us) trying to figure out how things came to be had more useful information about it.