Routes are recompiled on every call to non-blocking handler

[bsless]

As can be seen here: https://github.com/metosin/reitit/blob/master/modules/reitit-ring/src/reitit/ring.cljc#L337 This compilation has measurable overhead. Not much, but every bit helps. It can probably be replaced by or, no?

[miikka]

It's not correct to replace it with or: handler is going to return the result by calling respond or raise, not by returning a value, so looking at the return value is not correct way to do it.

[bsless]

In any case it can/should be inlined, no?

[miikka]

Yeah, inlining it would work and probably give minor performance benefit

[bsless]

certainly would. I see it in flame graphs

[bsless]

Started exploring an alternative solution, sketched an initial unrolled version of reitit.ring/routes:

(defn routes'
  "Create a ring handler by combining several handlers into one."
  ([handler]
   (when handler
     (fn
       ([request]
        (handler request))
       ([request respond raise]
        (handler request respond raise)))))
  ([h0 h1]
   (when (or h0 h1)
     (fn
       ([request]
        (or (and h0 (h0 request))
            (and h1 (h1 request))))
       ([request respond raise]
        (if h0
          (h0 request #(if % (respond %) (h1 request respond raise)) raise)
          (respond nil))))))
  ([h0 h1 h2] (routes' h0 (routes' h1 h2)))
  ([h0 h1 h2 h3] (routes' h0 (routes' h1 h2 h3)))
  ([h0 h1 h2 h3 h4] (routes' h0 (routes' h1 h2 h3 h4))))

I think I got the async arity right, but check me on that. Once we have the 2-arity, higher arities can be implemented by composition

wdyt?

[opqdonut]

Could you restate the original problem? The code link in the original message probably points to the wrong line.

[bsless]

@opqdonut updated the link, the culprit is the call to routes here: https://github.com/metosin/reitit/blob/master/modules/reitit-ring/src/reitit/ring.cljc#L337 The implementation is: https://github.com/metosin/reitit/blob/master/modules/reitit-ring/src/reitit/ring.cljc#L107 The proposed unrolled implementation is already faster, but worth considering is just running routes once during compile time to update all the handlers building the new handler+default handler which happens at run-time at line 337

[opqdonut]

Right, now I see what you mean. Arities >2 (or perhaps >5 or something if you want more fast cases) could be written with reduce.

I'd be fine with a PR with your sketch, as long as there's good test coverage also for the async case. I haven't looked at what the current coverage is, though.

[opqdonut]

Oh, btw, it looks like routes' returns nil if one of the handlers is nil, while routes ignores nil handlers in the non-async case, and I guess throws a NPE in the async case.

EDIT: no, scratch that, routes' seems to have at-least-as-correct nil behaviour compared to routes.

[bsless]

If we update the handlers with (routes handler default-handler) for each handler at compile time there is no need for the code change

[opqdonut]

Or could you wrap the fn returned by ring-handler with (routes .. default-handler)? I mean something like this:

(defn ring-handler [router default-handler ...]
  (let [default-handler ...
        wrap ...
        ...]
    (-> (fn ([request] ...) ([request respond raise] ...))
        (routes default-handler)
        (wrap)
        (with-meta {...}))))