Use FastThreadLocalThread

arnaudgeiser commented 2 years ago

Description

~This PR makes use of the io.netty.util.concurrent.DefaultThreadFactory from Netty instead of using the implementation of manifold.executor. The reason for this is that DefaultThreadFactory is creating FastThreadLocalThread instead of JDK Thread [1]. As demonstrated on the following code, it's supposed to be faster (fastGet) [2].~

This PR ensures we are using io.netty.util.concurrent.FastThreadLocalThread when creating the Aleph executor.

Here are some (interesting?) links on Netty:

[1] : https://github.com/netty/netty/blob/f17b5fe3cd7155a0281d081edabe223e131ba332/common/src/main/java/io/netty/util/concurrent/DefaultThreadFactory.java#L121 [2] : https://github.com/netty/netty/blob/69e1d36cb19c034dcdcaaa1085c0387435c06975/common/src/main/java/io/netty/util/internal/InternalThreadLocalMap.java#L100-L124

Testing done

Exercised code

(require '[clj-async-profiler.core :as prof])

(deftest test-compressed-response
  (prof/profile (with-compressed-handler basic-handler
                  (doseq [[index [path result]] (map-indexed vector expected-results)
                          :let [resp @(http-get (str "http://localhost:" port "/" path)
                                                {:headers {:accept-encoding "gzip"}})
                                unzipped (try
                                           (bs/to-string (GZIPInputStream. (:body resp)))
                                           (catch ZipException _ nil))]]
                    (is (= "gzip" (get-in resp [:headers :content-encoding])) 'content-encoding-header-is-set)
                    (is (some? unzipped) 'should-be-valid-gzip)
                    (is (= result unzipped) 'decompressed-body-is-correct)))))

Before

02-cpu-flamegraph-2022-04-26-23-52-57

After

01-cpu-flamegraph-2022-04-26-23-49-01

KingMob commented 2 years ago

Hmm, I'm concerned about breaking things.

enumerating-thread-factory doesn't promise to be anything more than a ThreadFactory based on its type hint, so I don't think swapping concrete classes will be the issue. The thread names will not be exactly the same (DefaultThreadFactory adds pool IDs), but hopefully nobody's got mission-critical logging regexes on thread names.

No, the only thing I'm actually concerned about is losing the classloader behavior @kachayev added to m.executor/thread-factory in https://github.com/clj-commons/manifold/commit/26038e03642ff8b0e125a7e35b333153b295c46f. See https://github.com/clj-commons/aleph/pull/425 and https://github.com/clj-commons/manifold/pull/161 for more background, but IIUC, Netty threads won't have the Clojure classloader set, so if a Clojure class has to be loaded for the first time on a Netty thread, it fails.

The fix would then be one of:

Copy m.executor/thread-factory to Aleph and replace Thread. with FastThreadLocalThread.
Add a thread-class param to m.executor/thread-factory that allows us to override Thread with FastThreadLocalThread
...proxy DefaultThreadFactory in Aleph?

I like option 2 the most. I hoped there might be a way to globally tell Netty to use the Clojure classloader for its threads, but given how much it messes with classloaders itself, I think that's risky.

Thoughts?

arnaudgeiser commented 2 years ago

The thread names will not be exactly the same (DefaultThreadFactory adds pool IDs), but hopefully nobody's got mission-critical logging regexes on thread names.

Good catch, I didn't notice the pool-id on the name

No, the only thing I'm actually concerned about is losing the classloader behavior @kachayev added to m.executor/thread-factory in https://github.com/clj-commons/manifold/commit/26038e03642ff8b0e125a7e35b333153b295c46f. See https://github.com/clj-commons/aleph/pull/425 and https://github.com/clj-commons/manifold/pull/161 for more background, but IIUC, Netty threads won't have the Clojure classloader set, so if a Clojure class has to be loaded for the first time on a Netty thread, it fails.

I was not able to reproduce the issue with my changes while it's reproducible with Aleph 0.4.6.

(defn handler [req]
  {:status 200
   :headers {"content-type" "text/plain"}
   :body "hello!"})

(def foo (http/start-server handler {:port 8080}))

;; after a sec

(.close foo)

Anyway, you are right, I think @kachayev poured a lot of thought on this change.

I like option 2 the most. I hoped there might be a way to globally tell Netty to use the Clojure classloader for its threads, but given how much it messes with classloaders itself, I think that's risky.

All for it. I will work on a PR on manifold to be able to pass a classname as the last argument of manifold.executor/thread-factory.

KingMob commented 2 years ago

At some point, we should consider destructuring maps, possibly with some fast destructuring lib. A lot of Aleph/Manifold fns have way too many params already.

clj-commons / aleph