Reaction equality

[scttnlsn]

I was running into a scenario where a reaction was running when I did not expect it to. Here's a minimal example demonstrating the issue:

(def db (reagent/atom {:foo ["test"]
                       :bar ["hello" "world"]}))

(def bar (reaction (map upper-case (:bar @db))))

(run! (print "bar changed:" @bar))

(defn update-foo [val]
  (swap! db assoc :foo [val]))

In this case, running (update-foo "test") was causing "bar changed" to be printed to the console. By switching identical? to = the behavior became more intuitive and and the reaction was only run when the value was not = to the previous value.

Is there a reason that identical? is being used? I think this change will even improve performance since components will need to be re-rendered less.

[idibidiart]

@scttnlsn

Check this out too:
savaki@b7d0d0d

I'm not the author of that but a little familiar with it. It attempts to fix issue #135 by removing (not dirty?) from the equality check on same line where you've made your change. The assumption there was that (not dirty?) was part of an over-optimization gone wrong. AFAIK, all tests pass with this change.

I do know that identical? works differently in Clojure than ClojureScript. I wonder if @holmsand could get to the bottom of this. Do we need an explicit dirty? optimization when we are dealing with immutables? and could we use = instead of identical? These may not be exactly the right questions but I'm curious regardless :)

Thanks for making this tweak. Let's see what Dan says.

[mike-thompson-day8]

identical? is the fastest check possible. It is just a "pointer" check.

= will be slower, perhaps much slower because it involves a (potentially) deep comparison.

Your example involves a simple, one element array, but imagine instead that the comparison involved a vector of 100 maps, and one of them, towards the end, was deleted. Checking by = could potentially involve a lot of computation only to come up with a false answer (which identical? would have told us instantly)

There are always trade-offs, but using identical? gives very good overall performance (stellar in some cases) but, most importantly, guards against the worst case.

So, I think I can channel Dan here and say this isn't going to change.

[scttnlsn]

@mike-thompson-day8 Yeah, that's a good point. On the flip side, we're potentially re-rendering unchanged components in other worse case scenarios (suppose in my example above that :bar is a 100 element vector and only :foo is changed). I think you're right that the identical? check is a better optimization, however, I'm concerned about the unintuitive behavior like I demonstrated in the example above. Do you have suggestions about alternative ways to fix that?

FWIW, I believe Om switched from using indentical? to = in 0.6.1 (https://github.com/omcljs/om/blob/master/CHANGES.md#enhancements-5). I'd be curious to hear the rationale there.

[mike-thompson-day8]

Remember that there's multiple levels of opimisation happening.

Above, we are discussing how to not rerun renderers (which produce new hiccup, and eventually VDOM). But even after that, React will be working with the VDOM to ensure no unneeded DOM updates are done. And it is the DOM updates that really take the time.

Regarding "unintuitive behavior": that's probably a documentation issue.

[darwin]

I ran into this issue with pretty complex tree of components (thousands). I wanted to prevent their re-rendering into VDOM at all costs.

I came up with a careful atom updating scheme which pays for equality test only once during write:
https://github.com/darwin/plastic/blob/56b38ae176d387a589bd4e69d7ae10ce6217c76c/cljs/src/main/plastic/util/helpers.cljs#L99-L136

This solved unwanted re-rendering. Just have to use overwrite-map instead of assoc, when updating my app-db ratom:
https://github.com/darwin/plastic/blob/56b38ae176d387a589bd4e69d7ae10ce6217c76c/cljs/src/main/plastic/cogs/editor/model.cljs#L354

A side note: overwrite-map could be generalised to work recursively. For my use-case root-level equality checks were enough.

[GetContented]

@scttnlsn @mike-thompson-day8 I'm curious... does a cursor behave the same way?

[mike-thompson-day8]

Okay, so this issue of using identical? vs = when checking for changed reaction "inputs" gets raised a bit.

I have a proposal:

1. stick with identical? as the default (see my justification above - Reaction equality #143 (comment))
2. Provide a named parameter to reaction which allows you to supply an alternative comparison function, for example =.

So I'm imagining being able to do this:

(reaction  (some-fn @some-ratom)  :comparison-fn =)   ;; use = for comparisons, not identical?

Update the open problem here is how to tell reagent that a render function (wrapped in an invisible reaction) wants to use = and not identical?

[timothypratley]

I support using equality:
a. The semantics of equality are preferable to identity for users.
b. Equality is a performance saving except in pathological cases.
c. Pathological cases should be handled by creating a reaction that matches what is being rendered, which will result in better performance than identity.

Below is an example of some timings:

Equality comparison of slightly different vectors of 1000 maps with 1000 keys check takes 1ms
Rendering a single div can take between 1-6ms
Re-rendering the same div can take 1-2ms
Rendering 1000 divs takes 4500ms
Re-rendering 1000 divs takes 4500ms

(time
 (reagent/render-component [:div]
                           (. js/document (getElementById "app"))))
(time
 (reagent/render-component [:div]
                           (. js/document (getElementById "app"))))

(def a (vec (repeat 1000 (zipmap (range 1000) (range 1000)))))
(def b (assoc a 999 {:foo :bar :baz 1}))
(defn component [xs]
  (into [:div]
        (for [x xs]
          [:div (str x)])))

(time
 (reagent/render-component [component a]
                           (. js/document (getElementById "app"))))
(time
 (reagent/render-component [component b]
                           (. js/document (getElementById "app"))))
(time (= a b))


"Elapsed time: 6 msecs"
"Elapsed time: 2 msecs"
"Elapsed time: 4527 msecs"
"Elapsed time: 4599 msecs"
"Elapsed time: 1 msecs"

[mike-thompson-day8]

@timothypratley switching from identical? to = would be a significant breaking change. Currently working code might stop working efficiently. So IMO it just can't happen.

The interesting question is: how can we introduce the option of using something else like = (or other comparison functions) to both a reaction you manually create and/or the automagic implicit/hidden reaction surrounding each renderer.

[ZabelTech]

Hi,
i got around that issue lately, too.

The interesting question is: how can we introduce the option of using something else >like = (or other comparison functions) to both a reaction you manually create and/or >the automagic implicit/hidden reaction surrounding each renderer.'

One way i can think of is to pass the comparison function into the deferrable and then in
'-reset!' only call '-notify-watches' if that comparison function not matches.

(defn atom
  "Like clojure.core/atom, except that it keeps track of derefs."
  ([x] (RAtom. x nil nil nil nil))
  ([x & {:keys [meta validator cmp-fn]}] (RAtom. x meta validator nil cmp-fn)))


(deftype RAtom [^:mutable state meta validator ^:mutable watches cmp-fn]
...
  IReset
  (-reset! [a new-value]
    (when-not (nil? validator)
      (assert (validator new-value) "Validator rejected reference state"))
    (let [old-value state]
      (set! state new-value)
      (when-not (and (nil? watches))
        (if (or (nil? cmp-fn) (not (cmp-fn old-value new-value)))
          (-notify-watches a old-value new-value)))
      new-value))
...

What do you think about this?

[holmsand]

@ZabelTech That's an interesting idea.

But I hope it won't be necessary in the next version of Reagent (0.6.0, probably).

What I'm planning to do is to make Reaction compare using = before it calls -notify-watches. At the same time, Reaction will become properly lazy, so that comparison will never be done on swap!/reset!, hopefully minimizing the cost of using = (as it is now, Reactions are evaluated immediately on swap!/reset!).

I'm also planning to add a new function, track, that turns a function call into a reactive value (i.e something derefable, like reagent.core/atom). This uses Reaction behind the scene, and thus the = comparison.

So to get "a layer of = comparison" you could simply use @(r/track deref foo) instead of @foo.

That said, it might be convenient to add something like a :cmp-fn option as well in the future...

[ZabelTech]

Hi,
Thanks, I'm kind of new to Clojure, so I'm just curious.
I hope you can share some insights with me.

If you reset! the value of a Reaction to be a value that gets lazily evaluated.
How do you determine if that value has changed without realizing it?
And if you do realize it, what did you win over realizing it immediately?
And how to know, when to realize and compare (if not triggered by reset!) ?

[mike-thompson-day8]

@holmsand I think there's a simple way to get what we want here, and have backward compatibility to boot.

At the moment, within a reaction (and a component renderer) you create an input signal by derefing a ratom - (deref ratom). The input signal created is guarded by the "changed test" of identical?. The reaction doesn't run unless the incoming value is different when judged by identical?.

How about we add a new way to deref a ratom (within a reaction). Call it say signal. Use would look like (signal ratom =) where = would be the "change test" function. In effect, using (signal ratom identical?) would match existing use of @ratom.

For brevity, I'd propose that (signal ratom) defaults to using =.

[holmsand]

@mike-thompson-day8 That is a somewhat nifty idea – except that I don't know how to implement that efficiently :)

I think the right way forward is to use = based equality in reactions by default – so that is what is in master right now. I think that the performance downside should be very limited – so I don't think we should add options to not use = until someone can show that it is necessary...

Reactions are now lazy, so they won't run on every reset!/swap! as before – and that reduces the cost of equality tests a lot.

[mike-thompson-day8]

@holmsand I'm staring at the code (the v5.1 version which I know better) and it seems to me it could be done as efficiently as existing (v5.1) code. Are you concerned that v5.1 is too slow?

BTW, I've imagining that:

1. notify-deref-watcher! would have to be given both the dereferable and the change-fn (identical? by default). Both would have to be stored in .-cljsCaptured
2. when, later, update-watching was processing and doing an add-watch on all the captured ratoms, it would include the change-fn (via partial? ... dunno something)

So it certainly appears as if it could be done efficiently to me. (But I am looking at v5.1 code, not the latest).

[holmsand]

@mike-thompson-day8 There are two efficiency problems, as far as I can tell.

For one, as you mention, instead of keeping track of watched atoms, you would have to keep track of "atom + equality-fn" tuples everywhere. That could add up to substantial overhead, I imagine.

More importantly, it attaches the equality function to the deref-site, instead of to the atom/reaction. Since = can be more expensive than identical? you only want to call it once per atom/reaction (given that the same reaction can be used in many places).

[mike-thompson-day8]

Closing this. Use of =, instead of identical? is in version 6.

[timothypratley]

Awesome, looking forward to it :)
Thank you for a great library, the longer I use it the more I am convinced Reagent is the best way to build UIs available today.

[mike-thompson-day8]

I agree completely about Reagent .. an absolutely delight to work with ... but all praise and thanks should be directed to Dan Holmsand. :-)