Eval routes and options given to context at initialization time

[frenchy64]

Close https://github.com/advthreat/iroh/issues/6619

Compojure{-api}'s context wraps its body in a function. This causes anything inside the route to be evaluated on every call.

We have a very expensive call in the POST /bundle/import route that recreates the entire Bundle schema on every request. @devn found a request where this took 10% of the request time.

Additionally, since everything under "/ctia" is under a context, I believe many of the routes themselves are being recreated on every request.

To comprehensively prevent this issue, I've restricted our context macro to only allow options that don't bind variables. This way, we can evaluate the routes and the options separately and then close the context over the value of routes. This seems to substantially decrease the number of values reinitialized during a request.

CI seems three times faster after the change. Most of the speedup is from tests for individual entities, where typically each namespaces takes 2-3 minutes, and now takes 30 seconds. Compare these metrics, where slowest tests are first:

Before (master): 1 hour 28 minutes
After: 40 minutes

I have not tested individual route performance.

§ QA

No QA is needed.

§ Release Notes

intern: Stage initialization code under context

§ Squashed Commits

[brookeswanson]

I'd warn UI folks before merging this, but this is a good find. Thank you @devn and @frenchy64

[yogsototh]

I do not see anything wrong, and I might not be familiar enough with CTIA.

That being said, from the problem description it looks like maybe the issue might be in ctia.bundle.routes/bundle-routes (here https://github.com/threatgrid/ctia/blob/master/src/ctia/bundle/routes.clj#L99):

The (let [bundle-schema (prep-bundle-schema services) ,,, is inside the form, I think this let should be put on top of route like:

(let [bundle-schema (prep-bundle-schema services)]
  (routes 
    (context ,,,,

And I think this might generate the schema not for every request but only once during the route init.

[yogsototh]

This is very difficult to really understand precisely what is going on. But I think the main problem comes from the auth middleware that set a :lets. By doing so, the context macro detects we have a lets and consider the context to be treated as dynamic (https://github.com/metosin/compojure-api/blob/master/src/compojure/api/meta.clj#L674) and this is AFAIU where the schema is potentially rebuilt.

Thus this put the let form just on top of the body and run for every request and not just at route init time.

So I am not sure this macro will be enough if you still allow the :capabilities param declaration at the context level.

[frenchy64]

But I think the main problem comes from the auth middleware that set a :lets.

Yes, this approach is incompatible with any middleware that sets :lets (other than _). It throws a compile-time error. That's why things like :auth-identity and :identity-map are pushed into the HTTP verbs.

By doing so, the context macro detects we have a lets and consider the context to be treated as dynamic (https://github.com/metosin/compojure-api/blob/master/src/compojure/api/meta.clj#L674) and this is AFAIU where the schema is potentially rebuilt.

Ah I see. We use compojure-api 1.1.13 which doesn't have this optimization. The difference between my macro and compojure-api master is that mine throws a compile-time error on a dynamic route.

So I am not sure this macro will be enough if you still allow the :capabilities param declaration at the context level.

FWIW here's the expansion:

(macroexpand
  `(context "" []
     :capabilities #{:foo :bar}
     (GET "" req# {:status 200})))
(let* [routes__207013__auto__ (compojure.api.core/routes
                                (ctia.lib.compojure.api.core/GET
                                  "" req__207089__auto__
                                  {:status 200}))
       capabilities207092 #{:bar :foo}]
  (compojure.api.core/context
    "" []
    :capabilities capabilities207092
    routes__207013__auto__))

[frenchy64]

Perhaps even with the optimizations from compojure-api master, it would ok? This macro takes advantage of the fact that most arguments to context are expressions (as opposed to values like :tags and :scopes).

[frenchy64]

That being said, from the problem description it looks like maybe the issue might be in ctia.bundle.routes/bundle-routes (here https://github.com/threatgrid/ctia/blob/master/src/ctia/bundle/routes.clj#L99):

@yogsototh I initially did that, then I realized that everything is under a context. The bundle routes are also under /ctia. The current compojure-api we use doesn't have the optimization you pointed out: https://github.com/metosin/compojure-api/blob/1.1.13/src/compojure/api/meta.clj#L310

[yogsototh]

LGTM!

Thanks for the tests they are beautiful.