lynaghk / cljs-react-perf

Performance experiments w/ CLJS React libraries and techniques.
40 stars 1 forks source link

On attribute vs. children ambiguity (avoiding Sablono interpretation) #5

Open lynaghk opened 7 years ago

lynaghk commented 7 years ago

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*.
roman01la commented 7 years ago

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

piranha commented 7 years ago

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.

lynaghk commented 7 years ago

@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.

thheller commented 7 years ago

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.

lynaghk commented 7 years ago

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?

thheller commented 7 years ago

{} is a constant and doesn't allocate.

lynaghk commented 7 years ago

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

thheller commented 7 years ago

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.

lynaghk commented 7 years ago

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

roman01la commented 7 years ago

@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.

lynaghk commented 7 years ago

@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 = )

roman01la commented 7 years ago

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)

lynaghk commented 7 years ago

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

roman01la commented 7 years ago

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)))))
lynaghk commented 7 years ago

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: