tools.nrepl

set stack size for Android

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: 0.0.5
  • Fix Version/s: 0.2.0-beta9
  • Component/s: None
  • Labels:
  • Environment:
    Android

Description

If you want to use nREPL on Android (currently only possible with sattvik's fork of Clojure) then you need bigger stack sizes than Android's default 8k to eval even moderately complex expressions. For comparison: A regular JVM on a PC has default stack sizes around 256k ... 512k depending on platform.

In nrepl.clj, modify

(doto (Thread. r)
(.setDaemon true))))))

to read

(doto (Thread. (.getThreadGroup (Thread/currentThread)) r "nREPL" 524288) ; 512k generous stack size
(.setDaemon true))))))

Note: There are warnings here http://bit.ly/u86tF1 about choosing a good stack size but since in practice nREPL seems to have <10 threads running with 1 active client connection we can be generous here.

The more advanced version would be to make this somehow configurable but that'd probably be over-engineering things.

Activity

Hide
Chas Emerick added a comment -

Noted, thank you. I'll try to make sure a provision for this gets into the next significant release.

My first inclination is to ensure that the stack size is configurable; a hardcoded size would likely be just as inappropriate in some environments as the system default.

Show
Chas Emerick added a comment - Noted, thank you. I'll try to make sure a provision for this gets into the next significant release. My first inclination is to ensure that the stack size is configurable; a hardcoded size would likely be just as inappropriate in some environments as the system default.
Hide
Chas Emerick added a comment -

nREPL is now using Clojure's built-in agent threadpools. This should theoretically simplify things for you, correct? i.e. Isn't a patched build of Clojure necessary for use on Android for this stack size issue, among other reasons?

Show
Chas Emerick added a comment - nREPL is now using Clojure's built-in agent threadpools. This should theoretically simplify things for you, correct? i.e. Isn't a patched build of Clojure necessary for use on Android for this stack size issue, among other reasons?
Hide
Hank added a comment -

The patched build is only necessary for the JVM byte code that the Clojure compiler dynamically generates to be converted to Dalvik bytecode. However adding additional android patches such as configuring the agent threadpool with a larger stack size do make sense. I'm not the official Android Clojure maintainer though – I guess there isn't an official one really – so my opinion on this only goes so far.

Show
Hank added a comment - The patched build is only necessary for the JVM byte code that the Clojure compiler dynamically generates to be converted to Dalvik bytecode. However adding additional android patches such as configuring the agent threadpool with a larger stack size do make sense. I'm not the official Android Clojure maintainer though – I guess there isn't an official one really – so my opinion on this only goes so far.
Hide
Chas Emerick added a comment -

Well, I think you have a solid case for expanding the scope of the android fork, or even for a change to parameterize the threadpools in Clojure itself.

I'm going to close this; there's really nothing to be done from within nREPL given that it doesn't maintain its own threadpool anymore.

Show
Chas Emerick added a comment - Well, I think you have a solid case for expanding the scope of the android fork, or even for a change to parameterize the threadpools in Clojure itself. I'm going to close this; there's really nothing to be done from within nREPL given that it doesn't maintain its own threadpool anymore.
Hide
Chas Emerick added a comment -

Reopening; since the use of agents was a fool's errand, parameterizing stack size needs to happen.

Show
Chas Emerick added a comment - Reopening; since the use of agents was a fool's errand, parameterizing stack size needs to happen.
Hide
Chas Emerick added a comment -

Now that I look at this again, there are a number of options in nREPL to solve this.

First, you can provide an :executor argument to the interruptible-eval middleware. This could be any java.util.concurrent.Executor, so you can configure it to use whatever threads you want.

Second, and if you want to minimize work and retain as much of nREPL's default configuration otherwise, you can patch in your own thread pool by binding #'clojure.tools.nrepl.middleware.interruptible-eval/configure-thread-factory like so:

(def thread-group ...)
(def stack-size ...)

(defn android-thread-factory
  "Returns a new ThreadFactory suitable for use with android. This implementation
   generates daemon threads, with names that include the session id."
  []
  (let [session-thread-counter (AtomicLong. 0)]
    (reify ThreadFactory
      (newThread [_ runnable]
        (doto (Thread. thread-group
                runnable
                (format "nREPL-worker-%s" (.getAndIncrement session-thread-counter))
                stack-size)
          (.setDaemon true))))))

(with-bindings {#'clojure.tools.nrepl.middleware.interruptible-eval/configure-thread-factory
                android-thread-factory}
  (clojure.tools.nrepl.server/start-server ...))

The above totally untested, but I hope you get the idea. Give it a shot, and let me know how it goes. I'm going to take off this issue's fix target for now.

Show
Chas Emerick added a comment - Now that I look at this again, there are a number of options in nREPL to solve this. First, you can provide an :executor argument to the interruptible-eval middleware. This could be any java.util.concurrent.Executor, so you can configure it to use whatever threads you want. Second, and if you want to minimize work and retain as much of nREPL's default configuration otherwise, you can patch in your own thread pool by binding #'clojure.tools.nrepl.middleware.interruptible-eval/configure-thread-factory like so:
(def thread-group ...)
(def stack-size ...)

(defn android-thread-factory
  "Returns a new ThreadFactory suitable for use with android. This implementation
   generates daemon threads, with names that include the session id."
  []
  (let [session-thread-counter (AtomicLong. 0)]
    (reify ThreadFactory
      (newThread [_ runnable]
        (doto (Thread. thread-group
                runnable
                (format "nREPL-worker-%s" (.getAndIncrement session-thread-counter))
                stack-size)
          (.setDaemon true))))))

(with-bindings {#'clojure.tools.nrepl.middleware.interruptible-eval/configure-thread-factory
                android-thread-factory}
  (clojure.tools.nrepl.server/start-server ...))
The above totally untested, but I hope you get the idea. Give it a shot, and let me know how it goes. I'm going to take off this issue's fix target for now.
Hide
Chas Emerick added a comment -

I hear good things at this point about people connecting to nREPL endpoints running on Android. Please file another issue if this or other Android-specific problems arise in the future.

Show
Chas Emerick added a comment - I hear good things at this point about people connecting to nREPL endpoints running on Android. Please file another issue if this or other Android-specific problems arise in the future.
Hide
Jürgen Hötzel added a comment -

clojure.tools.nrepl.middleware.interruptible-eval/configure-thread-factory is a private non-dynamic var.

so i had to use with-redefs.

Are there any concerns making this dynamic?

Show
Jürgen Hötzel added a comment - clojure.tools.nrepl.middleware.interruptible-eval/configure-thread-factory is a private non-dynamic var. so i had to use with-redefs. Are there any concerns making this dynamic?
Hide
Chas Emerick added a comment -

Not particularly, but with-redefs is a perfectly reasonable option (and likely safer, if you're really looking to patch in a completely different ThreadFactory.

What is prompting you to do so?

Show
Chas Emerick added a comment - Not particularly, but with-redefs is a perfectly reasonable option (and likely safer, if you're really looking to patch in a completely different ThreadFactory. What is prompting you to do so?
Hide
Jürgen Hötzel added a comment -

Yes. No need to use dynamic vars.

Show
Jürgen Hötzel added a comment - Yes. No need to use dynamic vars.
Hide
Alexander Yakushev added a comment -

OK, I finally integrated the fix into Neko. Thank you for help, Chas and Jürgen!

Show
Alexander Yakushev added a comment - OK, I finally integrated the fix into Neko. Thank you for help, Chas and Jürgen!

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: