Refactor advance-prop-map

[bsless]

I started using Reveal recently (thanks!) and out of curiosity wanted to profile its behavior.

This led me to some poking around, and I found that advance-prop-map can be rewritten more efficiently.

If I understand the implementation correctly, when advancing properties they can:

• exist only in the old "generation" (-remove, only side effects)
• exist only in the new "generation" (novelty, -add)
• exist in both (-update)

An "invariant" is that the resulting property map contains only the keys of the new generation, and all the keys only in the old generation translate to side effects, not returned data.

I tried to extract this logic out and separate it from the actions themselves to test the implementation out:

• for old props, perform side effects for all keys not in new props.
• for new props, add new props or update if they exist in the old group as well.

This led to a decision to iterate twice, which was happening anyway, so no performance impact on that regard, once on the old set performing only side effects, once on the new set performing side effects and building up the new props.
Transients are useful here because all the updates are "in-place"

Attached below are the performance comparisons, also available in this gist

[bsless]

(ns user
  (:require
   [clj-async-profiler.core :as prof]
   [clojure.set :as set]
   [criterium.core :as cc]))

(defn advance-map
  [-old -new -update -add -remove]
  (reduce-kv
   (fn [_ k v]
     (if (contains? -new k)
       nil
       (-remove k v)))
   nil
   -old)
  (persistent!
   (reduce-kv
    (fn [m k v]
      (if-let [e (find -old k)]
        (assoc! m k (-update k (val e) v)) ;; in both
        (assoc! m k (-add k v))))
    (transient -new)
    -new)))

(defn advance-map-old
  [-old -new -update -add -remove]
  (reduce
   (fn [acc k]
     (let [old-e (find -old k)
           new-e (find -new k)]
       (cond
         (and (some? old-e) (some? new-e)) ;; in both
         (assoc acc k (-update k (val old-e) (val new-e)))

          (some? old-e) ;; only in old
          (do
            (-remove k (val old-e))
            (dissoc acc k))

          :else ;; only in new
          (assoc acc k (-add k (val new-e))))))
    -old
    (set/union (set (keys -old)) (set (keys -new)))))

(defn advance-case
  [f -old -new]
  (f
   -old
   -new
   (fn -update
     ^long [_k ^long x ^long y]
     (unchecked-add x y))
   (fn -add
     ^long [_k ^long x]
     (unchecked-inc x))
   (fn -remove
     ^long [_k ^long v]
     (unchecked-dec v))))

(cc/quick-bench (advance-case advance-map-old {:a 1 :b 2 :d 4} {:a 1 :b 2 :c 3}))
;;; Evaluation count : 568806 in 6 samples of 94801 calls.
;;;              Execution time mean : 1.050974 µs
;;;     Execution time std-deviation : 13.713781 ns
;;;    Execution time lower quantile : 1.037821 µs ( 2.5%)
;;;    Execution time upper quantile : 1.066690 µs (97.5%)
;;;                    Overhead used : 2.071590 ns

(cc/quick-bench (advance-case advance-map {:a 1 :b 2 :d 4} {:a 1 :b 2 :c 3}))
;;; Evaluation count : 4628184 in 6 samples of 771364 calls.
;;;              Execution time mean : 129.309025 ns
;;;     Execution time std-deviation : 0.583299 ns
;;;    Execution time lower quantile : 128.772354 ns ( 2.5%)
;;;    Execution time upper quantile : 130.155832 ns (97.5%)
;;;                    Overhead used : 2.071590 ns


(cc/quick-bench (advance-case advance-map-old {} {:a 1 :b 2 :c 3}))
;;; Evaluation count : 1077996 in 6 samples of 179666 calls.
;;;              Execution time mean : 553.993109 ns
;;;     Execution time std-deviation : 2.825371 ns
;;;    Execution time lower quantile : 550.535939 ns ( 2.5%)
;;;    Execution time upper quantile : 557.015551 ns (97.5%)
;;;                    Overhead used : 2.071590 ns

(cc/quick-bench (advance-case advance-map {} {:a 1 :b 2 :c 3}))
;;; Evaluation count : 5706240 in 6 samples of 951040 calls.
;;;              Execution time mean : 103.337558 ns
;;;     Execution time std-deviation : 0.440638 ns
;;;    Execution time lower quantile : 102.913090 ns ( 2.5%)
;;;    Execution time upper quantile : 103.796798 ns (97.5%)
;;;                    Overhead used : 2.071590 ns


(cc/quick-bench (advance-case advance-map-old {:a 1 :b 2 :d 4} {}))
;;; Evaluation count : 996450 in 6 samples of 166075 calls.
;;;              Execution time mean : 596.655091 ns
;;;     Execution time std-deviation : 8.359428 ns
;;;    Execution time lower quantile : 589.086021 ns ( 2.5%)
;;;    Execution time upper quantile : 609.474819 ns (97.5%)
;;;                    Overhead used : 2.071590 ns

(cc/quick-bench (advance-case advance-map {:a 1 :b 2 :d 4} {}))
;;; Evaluation count : 8320632 in 6 samples of 1386772 calls.
;;;              Execution time mean : 75.640499 ns
;;;     Execution time std-deviation : 4.649587 ns
;;;    Execution time lower quantile : 70.531759 ns ( 2.5%)
;;;    Execution time upper quantile : 80.068583 ns (97.5%)
;;;                    Overhead used : 2.071590 ns

[bsless]

There is very similar logic in advance-composite-component which could benefit from similar optimization

[vlaaad]

Hi @bsless! Have you tried profiling this when used with real javafx components (e.g. for lifecycles that also call setters etc.)? I remember trying profiling cljfx a long time ago and IIRC JavaFX mutations were always the bottleneck, taking 90% or so of the execution time. If performing JavaFX "DOM" updates take milliseconds, there is not a lot of benefit from reducing cljfx "VDOM" updates from microseconds scale to nanoseconds scale... But that was a long time ago, maybe I don't remember this correctly...

[bsless]

Hi @vlaaad, thank you for taking a look
I agree my profiling method was a big weak. I profiled a running Reveal application, but didn't set up a rigorous, isolated test case.
How would you suggest I go about creating a test case programmatically?
I wouldn't mind doing so for several use cases and contributing them. Even if this PR doesn't get merged in the end, at least I'll have ended up improving visibility.

[vlaaad]

I usually wrap criterium in clj-async-profiler call, e.g.

(clj-async-profiler.core/profile
  (criterium.core/quick-bench (fx/advance (fx/create {... some-props}) {... some-props})))

That way I can see profiling results in "2 dimensions": both absolute timings between different algorithms from criterium and relative timings of parts of the profiled code from clj-async-profiler.

[bsless]

How does this test case look? Based on e16

(defn step
  [tree-view]
  (fx/advance-component
   @c
   {:fx/type :stage
    :showing true
    :title "Cell factory examples"
    :scene {:fx/type :scene
            :root {:fx/type :tab-pane
                   :pref-width 960
                   :pref-height 540
                   :tabs [{:fx/type :tab
                           :text "Table View"
                           :closable false
                           :content table-view}
                          {:fx/type :tab
                           :text "List View"
                           :closable false
                           :content list-view}
                          {:fx/type :tab
                           :text "Combo Box"
                           :closable false
                           :content combo-box}
                          {:fx/type :tab
                           :text "Tree Table View"
                           :closable false
                           :content tree-table-view}
                          {:fx/type :tab
                           :text "Tree View"
                           :closable false
                           :content tree-view}
                          {:fx/type :tab
                           :text "Date Picker"
                           :closable false
                           :content date-picker}]}}}))

(defn toggle
  [tree-view]
  (update tree-view :root update :expanded (fnil not false)))

(defn run
  [n]
  (let [times (long n)]
    (loop [i 0
           tree-view tree-view]
      (when (< i times)
        ;; (Thread/sleep 10)
        (step tree-view)
        (recur (inc i) (toggle tree-view))
        ))))

[vlaaad]

Looks good, thought I would probably avoid anything stage-related as it usually requires to be run on UI thread, so I'd use tab pane as a root component.

[bsless]

Slightly more generalized:

(defn profile
  [props step n]
  (let [component (fx/create-component props)
        times (long n)]
    (prof/profile
     (loop [i 0 component component props props]
       (when (< i times)
         (let [props (step props)]
           (recur (inc i) (fx/advance-component component props) props)))))))

(time
 (profile tree-view toggle 1e5))

[bsless]

@vlaaad please correct me if I'm wrong here:

• advance is just a sort of postwalk transition
• it does work for all components in a tree even if they're unchanged

Is it possible to take advantage of how Clojure collections work to short circuit walks on branches which are identical?? Perhaps it already happens and I missed it, but it could be a significant algorithmic improvement, while I'm here poking at maps

[vlaaad]

Unfortunately, advancing to identical description does not mean there will be no changes, here is an example:

;; advancing from
{:fx/type fx/ext-let-refs
 :refs {:node {:fx/type :label}}
 :desc {:fx/type :v-box
        :children [{:fx/type fx/ext-get-ref
                    :ref :node}]}}
;; advancing to
{:fx/type fx/ext-let-refs
 :refs {:node {:fx/type :text-field}} ;; <- replacing label with text-field
 :desc {:fx/type :v-box
        :children [{:fx/type fx/ext-get-ref
                    :ref :node}]}}

As you can see, the :v-box component description is identical in both instances, but since :node ref changes component type from Label to TextField, VBox's children need to be updated.