Add new-thread-fn parameter to manifold.executor/thread-factory

[arnaudgeiser]

Description

This adds a parameter called thread-class to manifold.executor/thread-factory to be able to pass custom implementation of a java.lang.Thread. This feature will be particularly useful for Aleph to be able to construct thread-pools with io.netty.util.concurrent.FastThreadLocalThread as mentioned on the following PR https://github.com/clj-commons/aleph/pull/595.

I'm unsure about the proposed implementation and I'm wondering whether including this complexity on manifold.executor is really worth.

Open to comments!

[KingMob]

We're overthinking this (or at least I am). Instead of passing a Thread class, let's pass a thread-constructor-calling function. That way we don't have to bother with eval or reflection, both of which are slower, and will cause problems with graal. We'll define a default one using Thread in Manifold, and Aleph can pass in one using FastThreadLocalThread.

ORIGINAL COMMENT BELOW >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

Hmmm, so eval will engage the whole clojure compiler, and I don't think it's graal-compatible at all, while reflection is at least partially graal-compatible, albeit requiring some support to let it know what to expect.

There are old, but still relevant-looking, SO questions on the topic at https://stackoverflow.com/questions/9167457/in-clojure-how-to-use-a-java-class-dynamically and https://stackoverflow.com/questions/3748559/clojure-creating-new-instance-from-string-class-name/3748621#3748621

In the second link, Chris Houser says eval is slower at first (60x slower according to user mikera), but faster than reflection from then on if you reuse the result.

We can't wrap the Runnable, since the ClassLoader can't be set after the thread is started, and it would probably get the wrong ClassLoader if it ran from Netty's thread factory, anyway.

I thought this would be straightforward, but now I'm rethinking this. One possibility is to leave the thread-factory params alone and use manifold.utils/when-class to see if FastThreadLocalThread exists, and carve out a special exception for Aleph. It's a bit hacky, but who else needs this behavior?

We could also move the thread-creating code into its own fn, and then alter the var root from Aleph. That won't involve any reflection or eval, unlike all the other options

;; in manifold.executor
(defn make-thread
  [^Runnable r some-more-params-here]
  (Thread. r blah blah blah))

;;; inside thread-factory:
        ...
           (if stack-size
             (make-thread nil f name stack-size)
             (make-thread nil f name))
        ...

;; in aleph.netty

(manifold.utils/when-class io.netty.util.concurrent.FastThreadLocalThread
  (alter-var-root #'make-thread
                  (fn [_]
                    (fn [^Runnable r some-more-params-here]
                      (io.netty.util.concurrent.FastThreadLocalThread. r blah blah blah)))))

[arnaudgeiser]

We're overthinking this (or at least I am). Instead of passing a Thread class, let's pass a thread-constructor-calling function. That way we don't have to bother with eval or reflection, both of which are slower, and will cause problems with graal. We'll define a default one using Thread in Manifold, and Aleph can pass in one using FastThreadLocalThread.

I've added a parameter called new-thread-fn which is supposed to be either a three or a four arguments function (depending of the presence of the stack-size) that returns a java.lang.Thread.

I don't think we want to pass a map here instead, do we?