add batch update logic to subs

[dehli]

Hi again 👋 Rather than using next-tick like I did in my last PR (#8), this introduce a start-batch-update! and an end-batch-update! function that can be used from the subs namespace. The default :db effect was updated to wrap the reset! call with these functions so that all the subs would be updated before the listeners are notified. This allows React to include all state changes in a single render.

Going to do some more QA with these changes on my app to confirm that it's working as desired but wanted to create the PR to gather feedback. Let me know if you'd like any changes or additions!

Closes #7

PS: I'm in refx channel on Clojurians slack if you'd like to discuss any of this there.

[dehli]

Thanks @ferdinand-beyer for your feedback! Hadn't though of this code being executed in multiple threads but I agree that it would be better if it was designed to support it. Additionally I like the idea of moving that batch logic into the subs namespace.

Will create another commit (or commits) to address the changes needed 👍

[dehli]

I've gone ahead and made the code thread safe I believe. Let me know if you see any more issues with that part! Will address your other comment next about moving that logic into the sub's namespace.

This would also allow to include state from other atoms in the batch update.

That would be great! We are actually using two atom's in our refx app so that's very appealing.

[dehli]

Looking through the code, I'm a bit stumped on how to integrate the start/end-batch-update! into the Atom's signal implementation. I'll try to explain my thought process :)

The fundamental issue is that calling (reset! app-db new-db) in the :db effect is triggering all of the listeners (or add-watch callbacks) one after the other. In the previous code, the first callback would run and call (.notify listeners) which would result in a re-render before the next callback had run.

With this PR, we now have a way to defer the notification calls until all the add-watch callbacks complete.

The solution I proposed was:

(subs/start-batch-update!) ;; Mark batch processing
(reset! app-db db) ;; Side effect of this is that add-watch is triggered for all listeners updating subs
(subs/end-batch-update!) ;; Notifies listeners that were queued from previous line

My first though was modifying the -add-listener implementation and wrapping the callback with a check to see whether batch processing should start. The issue is that I wouldn't be able to then know when all the callbacks had finished and I should call end-batch-update! to actually trigger notifications.

We could introduce a reset! wrapper that includes the batch logic, but that feels like it's the same coupling as we have now.

Did you already have an idea as to how it could work? Thanks!

[ferdinand-beyer]

Hm, I think you are right. I was naively thinking that we could do that in a watch function for the atom, but missed that we have multiple ones of those.

I had another look at your idea and I'm not sure if that works as expected. We queue notification thunks and defer their execution. However, executing them will cause more notification callbacks to be enqueued. Wouldn't that cause multiple renders again?

Looks like your solution works to some extend if we have multiple listeners on the app-db atom, but subscriptions that use other subscriptions as their input would be updated in a next render?

[dehli]

Ahhh, good point! That would be the dynamic sub case. Let me test and follow up.

[dehli]

Even though the dynamic subs are updated after end-batch-update! is called , I'm still seeing just a single render. It must be due to how React batches internal updates.

I have a branch on my fork with an example showcasing the behavior (https://github.com/dehli/refx/tree/dev/dehli/wip) but I'll include the code here as well.

Added the following event / subs:

(reg-event-db :debug/update-keys
  (fn [db]
    (let [id (js/Date.now)]
      (assoc db
             :key-a id
             :key-b id))))

(reg-sub :key-a
  :-> :key-a)

(reg-sub :key-b
  :-> :key-b)

(reg-sub :key-c
  :<- [:key-a]
  :<- [:key-b]
  (fn [[key-a key-b]]
    (+ key-a key-b)))

(reg-sub :key-d
  :<- [:key-c]
  (fn [key-c]
    (inc key-c)))

Created a view component like so:

(defnc debug-app
  []
  (let [[sub-vals set-sub-vals!] (use-state [])
        key-a (use-sub [:key-a])
        key-b (use-sub [:key-b])
        key-c (use-sub [:key-c])
        key-d (use-sub [:key-d])]

    (use-effect
     [key-a key-b key-c key-d]
     (set-sub-vals! #(conj % {:key (random-uuid)
                              :key-a key-a
                              :key-b key-b
                              :key-c key-c
                              :key-d key-d
                              :time (js/Date.now)})))

    ($ :div
       ($ :button
          {:on-click #(dispatch [:debug/update-keys])}
          "Click me")

       (for [x sub-vals]
         ($ :div {:key (:key x)}
            (str {:a (:key-a x)
                  :b (:key-b x)
                  :c (:key-c x)
                  :d (:key-d x)}))))))

and the view rendered the following after a single click (or dispatch).

With start-batch-update! and end-batch-update! calls:

I also added logs to the subs and batch-update logic to track ordering (which does support what you said). The last line doesn't have a batch-notify call since the value wasn't updated when it was rerun.

Without start-batch-update! and end-batch-update! calls:

I'll look through the repo some more (unless you already have ideas 😄 ) to see how else we can introduce batching since this current solution feels brittle and possible to break if React were to change when they trigger rerenders.

[dehli]

Looking at last screenshot I shared above bit closer, it is odd that the :key-c sub is called before :key-b sub since c depends on a & b. I wonder if that's the real bug and we don't need any of this batching logic (just React's internal batching would be enough).

Will continue investigating.

[ferdinand-beyer]

Looking at last screenshot I shared above bit closer, it is odd that the :key-c sub is called before :key-b sub since c depends on a & b. I wonder if that's the real bug and we don't need any of this batching logic (just React's internal batching would be enough).

I can help you with this.

The signal implementation in its current form is pretty naive. It actually was the first thing I came up with to replace Reagent in re-frame. I was planning on re-visiting this some day, or maybe replace this with some existing library to not re-invent the wheel.

Ideally, signals could form a dependency graph, and then be updated in one pass. Given your example:

flowchart LR
db --> :key-a
db --> :key-b
:key-a --> :key-c
:key-b --> :key-c
:key-c --> :key-d

When db is updated, valid topological orders would be db :key-a :key-b :key-c :key-d and db :key-b :key-a :key-c :key-d.

However, this is not how it is implemented right now. Instead, every subscription will notify its listeners, and they will eagerly re-compute and notify their listeners as well. So in this case, db updates :key-a, which updates :key-c, which updates :key-d. Then db updates :key-b, which will update :key-c again (and consequently :key-d as well).

This is not ideal, but it was good enough for the app I was developing back then, and I was not sure if we could afford re-computing a dependency graph again and again. There are probably some tricks to implement that efficiently, but I'm by no means an expert in this area.

[dehli]

Thanks for sharing the detailed explanation! I will continue working on this over the next few weeks when I have some free time.

[dehli]

Going to go ahead and close this and create a new PR when it's good to go 👍