Avoid AOT compilation of project code

[MichaelBlume]

This pull request reduces AOT compilation when compiling wars or jars. It AOT compiles only the generated servlet/listener namespaces, which act as shims, only importing the user's code or ring.util.servlet at runtime.

The service method gets stuffed in a var at init time and is read from there after that.

This more or less incidentally sorts out https://github.com/ring-clojure/ring/issues/166 -- the thing that gets stuffed in the var is the result of calling make-service-method.

[MichaelBlume]

May be relevant to #122 and #121

[MichaelBlume]

On second thought, this isn't quite right yet -- this resolves the handler symbol on every request, which is less than ideal.

[MichaelBlume]

Updated to avoid per-request resolution. This does deref a delay on each request, but that's significantly faster than resolving a symbol.

[MichaelBlume]

Hmm, looks like this is probably duplicated by #130 , dropping in to kibitz there instead

[MichaelBlume]

Ok, I think this is in good shape.

[weavejester]

I'm moving most of your comment to the description of the PR.

[weavejester]

Rather than testing an atom each time, why not use the :init method to set the root of a predeclared var?

[MichaelBlume]

Good idea, done =)

[MichaelBlume]

Just fixed a thing that was bothering me -- if there was an error loading the user's code, the user had no way of finding out what happened.

I think this should be ready to merge.

[MichaelBlume]

Is there anything I can do to help get this merged?

[weavejester]

Hi Michael,

I'm afraid it's just a case of pestering me until I find time to review it. Reviewing PRs is pretty time consuming, and it's hard to find time to do so for a project I no longer use. But the good news is that I'll add you to the growing list of contributors when this PR is merged.

First, a minor point: when submitting code to other people's projects, it helps to follow the standard of existing commit messages, which in this case means capitalised first letter. This isn't enough to reject the PR, but just be aware for next time.

Regarding the changes themselves, you've pulled make-service-method out, but I don't think you need to. I think you could save a whole lot of code just by using a shim function. For example:

`(do (ns ~servlet-ns
       (:require ring.util.servlet)
       (:gen-class :extends javax.servlet.http.HttpServlet))
     (defn ~'ring-handler [request]
       (throw (Exception. ~(str "Could not initiate with handler " handler-sym))))
     (ring.util.servlet/defservice ~'ring-handler))

So by default this is going to generate a servlet that's just going to blow up, but in the contextInitialized method we can use alter-var-root to replace the exception-generating handler with the real handler:

(defn ~'-contextInitialized [this# ~servlet-context-event]
  ~(if init-sym
     (generate-resolve init-sym))
  (alter-var-root
    (var ~(symbol (str servlet-ns) "ring-handler"))
    (generate-resolve handler-sym)))

Incidentally, the require-and-resolve function should be named something like generate-resolve to get across the idea that it's generating code.

Also, rather than checking the var resolves at run-time and throwing an error if something happens, we could check it at compile time instead:

(defn check-var-exists! [project var-sym]
  (eval-in-project project var-sym `(require ~(symbol (namespace var-sym)))))

I haven't tried it, but I'd imagine that will blow up if the contents of var-sym don't exist in the project.

You've looked at this longer than I have. Do you see any issues with this approach I haven't thought of?

[MichaelBlume]

Cool, will remember about commit messages.

I definitely like the change to generate-resolve and will try out the check-var-exists! change -- I'm still familiarizing myself with leiningen internals.

A minor concern using defservice in the generated code is that it results in AOT-compiling ring.util.servlet into the project. That's probably not that big a deal, ring.util.servlet is not a big namespace, it's not going to make the user run out of file descriptors or generate classfiles that differ only in case or anything pathological like that, but it's still writing bytecode to disk that we don't really need.

I like the idea of having an implementation of ring-handler that exists before the listener swaps it out, even if it only exists to throw an exception. Unfortunately, when I've experimented with creating wars and serving them on tomcat, simply changing from (def ~'ring-handler) to (defn ~'ring-handler ...) results in the servlet namespace somehow being loaded twice, and the alter-var-root affecting a different Var object than the one the handler is reading from. Adding (eval '~servlet-class) to the beginning of the init method somehow magically resolves the problem. I'm assuming this arises from some interaction between Clojure's AOT, Clojure's classloader, and Tomcat's classloader, but I don't have enough knowledge of the internals of any of the above to make a good guess.

[weavejester]

Ah, now I understand the reasoning behind moving make-service-method out.

Okay, so that leaves a couple of changes to require-and-resolve. Rename that function to generate-resolve or something like that, and get rid of the runtime error handling. Instead, let's use a var-exists? function to check the var exists at compile time. So something like:

(defn var-exists? [project var-sym]
  (eval-in-project
    project
    `(boolean (resolve '~var-sym))
    `(try
       (require '~(symbol (namespace var-sym)))
       (catch Exception _ nil))))

(defn generate-resolve [project var-sym]
  (assert (var-exists? project var-sym))
  `(do (require '~(symbol (namespace var-sym)))
       (resolve '~var-sym)))

Hopefully that should mean that any typos in the variable names are caught at compile time, rather than runtime.

[MichaelBlume]

Cool, playing with this now.

It doesn't look like eval-in-project actually returns a value, so var-exists? really needs to be more of an assert-var-exists? -- just evaling the symbol explodes if the var doesn't exist, and eval-in-project sees the bad exit code and throws.

The problem is that each call opens a brand new JVM and Clojure runtime, and all told this adds well over a minute to the war build time for my (non-trival, compojure-api-based) test project.

I think I'll try narrowing things to one var-exists? call for the entire build.

[MichaelBlume]

OK, so that works, it still adds like 30 seconds to the build, I'm wondering if it's worth making this optional, either by adding a 'lein ring check' command or adding a hook to the existing 'lein check' command.

[MichaelBlume]

Ok, I think this is ready to go. It adds some time to the build compared to not checking anything but not compared to AOT, so I think that's fine.

[weavejester]

Okay, I've checked this out, and it all looks fine to merge.

[jumarko]

Is there still an option to AOT compile everything instead of copying JARs into WEB-INF/lib/?

Right now, with this configuration we seem to be getting both class files and plain JARs:

  :uberwar {:aot :all
            :omit-source true
...

For instance classes for ring/core:

➜  uberwar git:(juraj-develop) ✗ ll WEB-INF/classes/ring/core
total 112
drwxr-xr-x  16 jumar  staff   512B Dec 22 17:32 .
drwxr-xr-x   6 jumar  staff   192B Dec 22 17:32 ..
drwxr-xr-x   3 jumar  staff    96B Dec 22 17:32 protocols
-rw-r--r--   1 jumar  staff   1.7K Dec 22 17:03 protocols$fn__88318.class
-rw-r--r--   1 jumar  staff   1.8K Dec 22 17:03 protocols$fn__88322.class
-rw-r--r--   1 jumar  staff   1.9K Dec 22 17:03 protocols$fn__88325$G__88320__88334.class
-rw-r--r--   1 jumar  staff   859B Dec 22 17:03 protocols$fn__88325$G__88321__88329.class
-rw-r--r--   1 jumar  staff   967B Dec 22 17:03 protocols$fn__88325.class
-rw-r--r--   1 jumar  staff   1.4K Dec 22 17:03 protocols$fn__88342.class
-rw-r--r--   1 jumar  staff   2.8K Dec 22 17:03 protocols$fn__88344.class
-rw-r--r--   1 jumar  staff   1.4K Dec 22 17:03 protocols$fn__88352.class
-rw-r--r--   1 jumar  staff   1.3K Dec 22 17:03 protocols$fn__88354.class
-rw-r--r--   1 jumar  staff   755B Dec 22 17:03 protocols$fn__88356.class
-rw-r--r--   1 jumar  staff   2.4K Dec 22 17:03 protocols$loading__6789__auto____88316.class
-rw-r--r--   1 jumar  staff   1.6K Dec 22 17:03 protocols$response_writer.class
-rw-r--r--   1 jumar  staff   7.4K Dec 22 17:03 protocols__init.class
...

and also the JAR:

$ ll WEB-INF/lib/ring-core*
-rw-r--r--  1 jumar  staff    31K Dec 22 17:03 WEB-INF/lib/ring-core-1.7.1.jar

At minimum, I want to avoid packaging clj files into the uberwar for a few specific libraries.