Testing for 0.5.0 release

[danielcompton]

Hi folks

We've released https://github.com/Day8/re-frame/releases/tag/v0.5.0-alpha1. Try it out in your apps and let us know what you think. There are two changes that need particular attention:

• Dynamic subscriptions - Add dynamic subscriptions to re-frame #108. Try them out and let us know if the API feels right for this. I'm pretty sure that we don't have any memory leaks in our implementation of dynamic subscriptions, but let us know if something seems funky. More docs for this will be forthcoming
• Switching from (core.async/timeout 0) to (yield) in the router dispatch loop - Use yield instead of (timeout 0) #121. We need to give control back to the browser while we're processing for it to do Browser Things. We previously used (timeout 0) to do this. The time taken for the (timeout 0) channel to close is browser specific, but in general takes at least 4 ms. This meant we couldn't process more than 250 events/second. The new yield implementation, helpfully shared by https://github.com/pkobrien is much faster. Again, we think this is implemented correctly, but let us know if you see anything funky.

Thanks!

[scgilardi]

summary: re: (yield) change: I see a nice improvement in my app's performance, but perhaps we can get an even better balance between the needs of the browser and the needs of the router.

I'm writing an app that uses svg to allow the user to draw items onto a drawing surface. Currently I have one entry in the db when a new item is being created by a mouse drag and a separate entry for the list of items that already exist. In the "mouse up" handler for a creation drag, I dispatch two re-frame events: :end-drag, and :create-item. Before the yield change this gave a significant visual glitch as the drawing surface was redrawn on each yield: first without the new item (after :end-drag was handled) and then again with the new item in the existing items list (after :create-item was handled).

In this case, the effect of the yielding to the browser before handling each event was a visual flicker ("there" "not there" "there") during item creation. The duration of "not there" is greatly reduced in my setup after the change to use (yield) instead of waiting for (timeout 0). Sweet!

It does appear though that even with the new faster yield, software down the stack from re-frame (reagent/react/chrome) is rendering my svg every time the router loop yields. That seems like too much deference to UI handling over router event loop handling.

I see in the comments that the purpose of the yield to make sure the browser gets some time in case the handler hogs the cpu for a long time. I suspect many handlers are extremely fast (manipulations of the db atom) and it seems to me we should try to handle as many as we can in some reasonably small time slice rather than yielding before every event.

I tried this modification to the router yielding algorithm: when there is a new event to handle, check if it's been at least 5 ms since we most recently returned from a yield. If yes, yield now and start a new 5 ms timer when the yield returns. If no, handle the event without yielding first.

This removed the visual glitch in my case and I think makes a better tradeoff between the needs of the browser and the needs of the router. Fast handlers will execute much faster on average and even if a slow handler slips in and starts at just under 5 ms after a yield returned, the effect on the responsiveness of the browser will be worse by at most 5 ms.

Here's a sketch of some code that's working for me locally:

(defn yield-chan
  [flush? yield-time now]
  (cond flush?
        (do (flush) (timeout 20))
        (>= now yield-time)
        (yield)))

(defn router-loop
  []
  (go-loop [yield-time 0]
    (let [event-v (<! event-chan)
          yield-time (if-let [ch (yield-chan (:flush-dom (meta event-v))
                                             yield-time
                                             (system-time))]
                       (do (<! ch)
                           (+ (system-time) 5))
                       yield-time)]
      (try
        (handle event-v)
        (catch js/Object e
          (do
            (purge-chan)
            (router-loop)
            (throw e))))
      (recur yield-time))))

[danielcompton]

@scgilardi I've been thinking about this and have an approach that we want to try involving running handler functions immediately. Do you have a public example of your SVG app we can use as a test case? Don't worry if you don't have it available, it's not critical.

[Quantisan]

I updated and my app works as usual. It's a drop-in upgrade so far. Good stuff!

[scgilardi]

@danielcompton Thanks! I extracted a public example out here: https://github.com/scgilardi/re-frame-svg-demo . It lets you draw rectangles and demonstrates the flicker I'm talking about.

[kamn]

Updated my app and didn't have any issues. Good work! I will try to play with dynamic subscriptions in the next few days and see how it goes.

[mbertheau]

No issues as well. Just less noise in the JS console! Thanks :)

[mike-thompson-day8]

@scgilardi there's a new version of the router loop in develop.

I've dumped using core.async completely. Each processing cycle, ALL events in the queue are processed without yielding. But no NEW events are processed. They will be done next cycle. This seems to strike the right balance between draining the queue ASAP, and being a good CPU citizen.

Obviously this is experimental but I have high expectations.

It is not deployed to clojars yet (next couple of days). In the meantime, to use it, you'll have to:

• checkout develop
• lein install to put it your local maven repo
• change your deps to be for [re-frame "0.5.0-alpha2"]

[mike-thompson-day8]

[re-frame "0.5.0-alpha2"] is now in clojars. Could you test it please?

There's a new event processing (router) loop. I replaced use of core.async with a hand-rolled solution. As a result:

• events will be handled more quickly after their dispatch (<1ms vs 5ms??)
• groups of events queued up will be handled in a batch, one after the other, without yielding to the browser (previously re-frame yielded to the browser before handling every single event)

I doubt this will affect normal apps. But it could affect games which depend on existing timings. Maybe. It could affect apps which dispatch large volumes of events (telemetry?) very quickly. Maybe.

I'm not expecting any problems, just being a little paranoid.

[scgilardi]

@mike-thompson-day8 thanks, it looks like a nice improvement. We can now think in terms of a packet of events being handled without yield. I updated my demo at https://github.com/scgilardi/re-frame-svg-demo and it's beautifully flicker-free after being made compatible with the new router policy.

Nicely done!

I have a few thoughts based on reviewing the code:

• I think this portion of the state machine is buggy: [:do-paused :add-event] -> [:paused #(-add-event this arg1)]. Except when the current state is :quiescent, adding an event should not change the state. In particular with the current code, adding an event while processing the event that previously caused a pause will result in a subsequent illegal state transition [:paused :done-paused]. The buggy portion should be changed to [:do-paused :add-event] -> [:do-paused #(-add-event this arg1)] .
• I suggest a couple of name changes:
  • ":do-paused" -> ":resuming" for easier understanding. I think it's a general improvement, but also by being more distinct from :paused, it may have helped avoid the bug above.
  • ":done-paused" -> ":finish-resume" (for consistency with :finish-run)
  • ":resume-run" -> ":begin-resume" (for consistency with :begin-run)
• It seems unfortunate that the entire queue is flushed on any exception. I can see the reasoning and it would certainly be a good policy for a re-frame app never to throw out of a handler to avoid the issue entirely.
  • I wonder if at some point a notion like "a packet of events" could be made more concrete and only the members of the packet could be discarded on an exception. This scenario comes to mind: I handle event A by generating events B, C, and D and the new router will execute all of those without pause in the next router time slice. But into the mix comes event W which is unrelated to processing A--maybe it's a reminder whose time has arrived. With the current router, if W ends up at the end of the same router time slice as B, C, and D and C throws, D will not be processed (probably good), but also W will not be processed (unfortunate).
  • it might be useful to be able to mark B, C, and D as being derived from processing events in the previous router time slice and only flush marked events on an exception in one of them.
    • unmarked events (perhaps events that were queued from outside a handler) would still be processed.
  • It's probably more complexity than it's worth to try to add this kind of nuance to how exceptions are handled by the router, but I thought I'd mention it in case someone can think of an elegant way to throw away only the bathwater in a case like this.

[mike-thompson-day8]

@scgilardi Thanks! I greatly appreciate that thoughtful review.

You are 100% right about the bug. Thanks! Also, I've adopted your name changes. Good suggestions. I'll release an alpha3.

Regarding the handling of events post an exception, it seems to me there is no strategy which will work generally, so sticking with something utterly simple seems right. For example, what happens if W (from a websocket) gets interleaved with your C D. Mind you, I have a selfish angle here in not looking too hard for solutions. In our apps, when an Exception is thrown, we do an "undo" to put app-db back into a known-good-state and try to let the user continue on. So clearing events works in well with that.

[scgilardi]

@mike-thompson-day8 you're welcome! I'm glad it was helpful.

Another quick one: The condp = in the -fsm-trigger body can be replaced with case with the benefit of constant time, hash-based dispatch. (I'm not sure it's faster in a way that matters, but it is an ideal use-case for case, order of comparisons doesn't matter and all the test values are constants)

[mike-thompson-day8]

see alpha3 about to appear in clojars

[mike-thompson-day8]

v0.5.0 now released.