Reuse recursive ref-transformers

[alexander-yakushev]

This addresses a similar problem to #1245 but this time for transformers, not validators. The issue was discovered very recently in Metabase, where a decoder for a recursive schema definiton would grow infinitely, depending on the provided data (and from the look of it, the growth is quadratic). Here's a simplified reproducer:

(require '[clj-memory-meter.core :as mm])

(def my-sch
   [:schema
    {:registry {"hiccup" [:orn
                          [:node
                           [:catn
                            [:name keyword?]
                            [:props [:map-of keyword? any?]]
                            [:children [:* [:schema [:ref "hiccup"]]]]]]
                          [:primitive any?]]}}
    "hiccup"])

(def my-decoder (decoder my-sch (malli.transform/default-value-transformer)))

(mm/measure my-decoder) ;; 15.2 KiB - before any data has passed through the decoder

(defn generate-nested-hiccup [n]
   (if (zero? n)
     [:p {} "Hello, world of data"]
     [:div {} (generate-nested-hiccup (dec n))]))

(my-decoder (generate-nested-hiccup 100))
(mm/measure my-decoder)  ;; 157.3 KiB

(my-decoder (generate-nested-hiccup 200))
(mm/measure my-decoder) ;; 297.9 KiB - linear growth here but I've seen quadratic too

The solution is similar and inspired by @frenchy64's #1245, however, here I had the luxury of options map and so could avoid dynvar shenanigans. However, I would still need feedback on whether this approach is valid. Anyway, after the patch:

(def my-decoder (decoder my-sch (malli.transform/default-value-transformer)))

(mm/measure my-decoder) ; 42.0 KiB - initial footprint is bigger,
                        ; probably because it hangs onto a few more clojure.core vars

(my-decoder (generate-nested-hiccup 100))
(mm/measure my-decoder) ; 3.1 KiB - dropped down because I also benchmark with
                        ; https://github.com/metosin/malli/pull/1249, so you
                        ; can observe thunk cleaning at work

(my-decoder (generate-nested-hiccup 200))
(mm/measure my-decoder) ; 3.1 KiB - doesn't grow at all

[dragonsahead]

Hi @opqdonut, this seems to be ready to merge, what's the process to make that happen?

[opqdonut]

I'll try to have time to review this this friday.

Feedback from @frenchy64 would be welcome as well, since he's thought a lot about this topic.

[frenchy64]

This looks good, and should work with lazy and eager refs.

There might be further optimization opportunities for eager refs. For those, I think we wouldn't need -memoize or extra thunks, we can just create the transformer eagerly. If you want to investigate that, we should be able to contain the difference to a single branch like the if lazy test in the validator.

[frenchy64]

This looks like a testing point. Could you add a unit test that counts how many times -value-transformer is called for a custom transformer that records this info? Then call the transformer and ensure the number is constant no matter the depth of the input?

[alexander-yakushev]

I've just shamelessly stolen your test case from #1254 for this, it is accurate down to comments 😄. Hope it's OK.

[frenchy64]

Here we don't include transformer (I assume) because it's constant as we recur. Is that also true of method?

[alexander-yakushev]

Good point. I'm not sure if the transformer can be changed mid-flight in the call chain. If it can, then it is possible to include transformer into the cache key too – it won't be expensive because most (all) of the time the transformers will be pointer-equal.

[alexander-yakushev]

Added the test you've requested. I'm not sure if this is still relevant after the discussions on Slack and in:

• https://gist.github.com/wotbrew/6bc413291e2c667d66d35b78868dc2d2
• https://gist.github.com/wotbrew/e5af4d05655f70fe4cbf21b6820021f1

However, this approach seems complexity-cheap and still brings decent benefit in the absence of a more holistic solution.

[opqdonut]

@frenchy64 Do you think it makes sense to merge this or are you working on something that will have the same effect?

[frenchy64]

This PR is still relevant, I'll take a final look over and leave a review. Maybe there's a better approach but this seems like a great starting point, especially with a test.