cljs-react-perf icon indicating copy to clipboard operation
cljs-react-perf copied to clipboard

On attribute vs. children ambiguity (avoiding Sablono interpretation)

Open lynaghk opened this issue 8 years ago • 15 comments

The sablono wiki suggests that Sablono can emit faster code when an element has an unambiguous attribute map. That:

[:div {} (foo)]

is faster than

[:div (foo)]

It'd be great to:

  1. Measure this difference
  2. See if there's a way to have Sablono tell us when it's emitting sub-optimal code at compile time --- something like clojure's *warn-on-reflection*.

lynaghk avatar Apr 10 '17 19:04 lynaghk

I guess @piranha has some experience with this, maybe he could help to answer your questions

roman01la avatar Apr 11 '17 08:04 roman01la

It will, because in case of [:div (foo)] sablono generates code which tries to check if result of (foo) is an attribute map for a :div. It's not only slower, but also bigger.

piranha avatar Apr 11 '17 08:04 piranha

@r0man mentioned to me via email that you can use metadata to tell Sablono about the return type of forms.

So

[:div ((constantly {:class "a"}))]

is slow because the result has to be interpreted at runtime, but

[:div ^:attrs ((constantly {:class "a"}))]

should be faster because Sablono can emit code with fewer runtime checks. I should measure this too.

lynaghk avatar Apr 11 '17 18:04 lynaghk

You should also consider the approach chosen by om.dom (and others) where you'd write (dom/div nil "foo") instead of [:div {} "foo"]. The dom/* functions can directly create a ReactElement and never enter interpreted mode which would allocate vectors first.

The problem with sablono (and hiccup) is that the compiler cannot guarantee that everything is compiled and must fallback to interpreted mode (which is a lot slower).

If performance is your only goal it is easier to just use dom/*. type-hints or html calls are easier to forget and you never notice because the interpreted mode covers for you.

thheller avatar Apr 11 '17 19:04 thheller

I was really surprised about the results on this one. These are the scenarios:

(defn make-attrs
  []
  {:data-some-attrs "a"})

(rum/defc app-13
  [state]
  [:.app (for [i (range 1000)]
           [:.child (make-attrs)])])

(rum/defc app-14
  [state]
  [:.app (for [i (range 1000)]
           ;;This isn't documented, but R0man told me about it in an email
           ;;https://github.com/r0man/sablono/blob/fb5d756c4201598fe8737ae2877e76f9c25a96f1/src/sablono/compiler.clj#L150
           [:.child ^:attrs (make-attrs)])])

(rum/defc app-15
  [state]
  [:.app (for [i (range 1000)]
           [:.child i])])

(rum/defc app-16
  [state]
  [:.app (for [i (range 1000)] 
           [:.child {} i])])

and the results:

# timing (ms)
13 56 ± 11
14 58 ± 11
15 5 ± 2
16 3 ± 2

So it looks like the function call to get the static attribute map is the real cost here, no the ambiguity of interpretation.

Ya'll notice anything fishy about the way I'm trying to test this?

lynaghk avatar Apr 11 '17 21:04 lynaghk

{} is a constant and doesn't allocate.

thheller avatar Apr 11 '17 22:04 thheller

@thheller Are you referring to the {} in app-16? I'm not sure what you're getting at, can you elaborate?

lynaghk avatar Apr 11 '17 22:04 lynaghk

Yes, you are doing an unfair comparison. 13+14 call a function which allocates a new map 1000 times. The app-16 doesnt do that.

thheller avatar Apr 11 '17 22:04 thheller

Right, sorry if I wasn't clear. The comparisons I'm making are 13 vs. 14 (hinting attribute map) and 15 vs. 16 (ambiguity of children).

So the surprise for me is that those differences are within the measurement error in both cases, which is not what I expected given the tone of the Sablono wiki about the performance benefits of such hinting/disambiguation.

Re: allocations, I'm a bit surprised that ClojureScript and/or Closure doesn't realize that the function returns an immutable constant. But then again, I'm still longing for that "sufficiently smart compiler" =P

lynaghk avatar Apr 11 '17 22:04 lynaghk

@lynaghk Interesting to see that 15 and 16 are almost identical. However as it was said already, when there's no explicit attr map e.g. [:.child i] Sablono will wrap it into interpret call which effectively means that it will interpret a vector at runtime. I guess this introduces much more notable difference at scale, when you have 100+ components. Also having interpret calls adds some KB's into the output.

roman01la avatar Apr 12 '17 09:04 roman01la

@roman01la when you say the difference will be more notable when you have 100+ components, do you mean as children of the ambiguous parent? E.g., is your hypothesis that:

[:.child [:.foo] [:.bar] [:.baz]]

will be slower than

[:.child {} [:.foo] [:.bar] [:.baz]]

because in the former case the three children will be interpreted, where in the latter case there will be no runtime interpretation?

Can you think of a better example that demonstrates the point that interpretation is slow? Everyone thinks that it's slow, so I'd love to have a conclusive example = )

lynaghk avatar Apr 12 '17 18:04 lynaghk

A good benchmark might be a recursive UI structure. Interpretation is for sure slower than plain React.createElement call :) (here's Sablono's interpreter code https://github.com/r0man/sablono/blob/master/src/sablono/interpreter.cljc)

roman01la avatar Apr 12 '17 19:04 roman01la

@roman01la Can you provide an example with code, similar to app-15 and app-16 above?

lynaghk avatar Apr 12 '17 21:04 lynaghk

Don't have time for this, sorry. Here's a quick output:

;; in
(macroexpand '(html [:.app (for [i (range 100)] [:.child i])]))

;; out
(js/React.createElement
 "div"
 #js {:className "app"}
 (into-array
  (clojure.core/for
   [i (range 100)]
    (clojure.core/let
     [attrs35913 i]
      (clojure.core/apply
       js/React.createElement
       "div"
       (if
        (clojure.core/map? attrs35913)
         (sablono.interpreter/attributes
          (sablono.normalize/merge-with-class {:class ["child"]} attrs35913))
         #js {:className "child"})
       (if
        (clojure.core/map? attrs35913)
         nil
         [(sablono.interpreter/interpret attrs35913)]))))))
;; in
(macroexpand '(html [:.app (for [i (range 100)] [:.child {} i])]))

;; out
(js/React.createElement
 "div"
 #js {:className "app"}
 (into-array
  (clojure.core/for
   [i (range 100)]
    (js/React.createElement
     "div"
     #js {:className "child"}
     (sablono.interpreter/interpret i)))))

roman01la avatar Apr 13 '17 08:04 roman01la

I tried a recursive approach (similar to nested scenarios 9 and 10) but couldn't find anything significant. I updated the number of children of scenarios 13/14 and 15/16 in c83b6a5 but any interpretation costs for that simple markup is negligible.

Perhaps:

  • these scenarios have markup that is too simple, and interpretation is slower with more complex markup
  • the looping nature of the benchmarks makes the JIT happy, and obscures perf issues that would appear in a real application

lynaghk avatar Apr 14 '17 05:04 lynaghk