Enhancements to dom rendering functions

[awkay]

It was suggested that we add an assertion to the dom rendering functions to give an error when #js is forgotten. That seemed reasonable, but I've always wondered if the #js requirement was worth the benefit of the optimization, so I decided to try to quantify it.

I did the following experiment:

Render 10 divs containing 90 spans at 60fps, while measuring the time to render.

BEFORE code changes: nil props: Early iterations at about 1.95ms/frame, converging towards 0.67ms/frame 3k/v pairs in #js obj as props: Early iterations 2.5ms/frame, converging to 1ms/frame

AFTER code changes: nil props: no measurable effect JS obj 3k/v pairs: no measurable difference CLJ obj 3k/v pairs: Early iterations 6-7ms/frame, converging to 2.07ms/frame

To ground-truth this I ran a few simple timings on map conversions, and a single k/v map seems to take about 3-4 microseconds to convert. A map with 3 entries takes about 10 microseconds (when done in units of 100 iterations). Thus, I would expect the overhead for clj->js in this benchmark to consume about 1ms/frame, which is exactly what I did measure.

I then measured the overall browser performance flame chart to get an idea of how long the frame was actually taking from the complete React standpoint (all DOM was needing a refresh because I was changing content).

NO (nil) props: User code execution: 2.1ms Overall time including browser paint: 3.5ms

CLJ props: User code: 4.3ms Overall time including browser paint: 6ms

So, now we can gain some perspective if we relate this to a target frame time max of 16ms as the target number to maintain full 60fps animation rates.

Fulcro, of course, has overhead of the query engine. That is a relatively heavy task, so let's say we allocate half of this time to query engine concerns. This leaves our rendering task 8ms.

If we assume an average update on the order of 100 DOM elements (which is generous considering shouldComponentUpdate should prevent non-essential changes), then using clj maps for properties should represent no more than about 1/8th of our usable time (assuming all nodes needed props, which again is quite generous).

User code execution in the flame graph indicates that React consumes about 1.3 to 2ms/frame for this task.

One can conclude that most common web applications will not see a significant performance impact from using clj maps; however, if they do these measurements also show that the inclusion of the suggested cond in the DOM generation functions has such a small effect as to be immeasurable in the face of performance deviations.

Thus, I'm proposing we add support for all DOM tag functions to support auto-conversion of clj maps. The cond I'm suggesting check the cheapest conditions first: nil and object?, followed by map?. If neither is found we can either return what we were passed or throw an exception. Not sure which is best yet, as I'm worried there may be a way to pass props that would not be detected as object? but would be acceptable to react.

[currentoor]

@thheller has some pretty good ideas in his repo about converting maps to js objects at compile time whenever possible. https://github.com/thheller/shadow/blob/master/src/main/shadow/markup/react.clj#L147

We port that code into Fulcro. We were also thinking about supporting some additional macro convenience.

(dom/div :#some-id.some-class {:style {:background-color "red"}}
  ...)

Would be converted at compile time to

(dom/div #js {:id        "some-id"
              :className "some-class"
              :style     #js {:backgroundColor "red"}}
  ...)

However, if there are symbols anywhere, then we'd need to run an interpreter at runtime. Also this needs to work nicely with server side rendering.

This means there are a bunch of use cases we'd have to support Allow nil

(dom/div nil
  (dom/div ...)
  (dom/div ...))

Allow just the attr keyword

(dom/div :#some-id.some-class
  (dom/div ...))

Allow just the js object

(dom/div #js {...}
  ...)

Runtime interpreter

(let [attr {:on-click (fn [] ...)}]
  (dom/div attr
    ...))

Omit attrs

(dom/h1 "Title")

We can test client side using dev cards and for the server side we can test using dom/render-to-str.

[awkay]

@currentoor You have a typo in the "just the attr keyword"...you forgot the colon.

[currentoor]

@awkay edited, thanks.

Also I was thinking since we can only ever have one id, another options is :some-id/some-class.other-class vs :#some-id.some-class.other-class. Thoughts?

[awkay]

The only problem I see with that is the thing before the slash has to look like a valid ns...can ids have things in them that are not valid in a ns? Also, it sorta makes the first class a bit ambiguous, since it no longer has a dot, and it confuses your eye a bit because you've trained it to read keywords with nses differently.

[awkay]

Most of this is working really well. @currentoor is planning to add support for pulling symbols into classes as an additional bit of sugar, which will be helpful with fulcro-css.

[awkay]

In 2.4.0. Could still use the symbol classes, but less worried about them. Everything else seems pretty good.