-
Notifications
You must be signed in to change notification settings - Fork 527
Add Jetty options for acceptor/selector threads #534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Jetty options for acceptor/selector threads #534
Conversation
e303a27
to
e2fa56e
Compare
@weavejester for when you have a chance to review/consider this MR, I think because I changed the Github workflow it did not run automatically, but it passed here: https://github.com/k13gomez/ring/actions/runs/17280405265 so all the tests should be passing. |
Thanks for the PR. I'll try to review it soon. Could you explain why not use Jetty's default values for the acceptors and selectors when using virtual thread pools? I'm guessing Jetty doesn't take into account the pool being used? |
Having taken a look through it, I think we probably need to break this PR into two: one that adds options to configure acceptor/selector thread counts, and one that adds a virtual pool. The acceptor/selector PR should be fairly straightforward, but I'm not yet convinced a If the acceptor/selector values are -1, then Jetty will use its defaults. This should cut down on some of the code. |
Sounds good, I will remove the virtual pool changes for now, which are less important, and focus on acceptor/selector portion of it. The acceptor/selector options can severely affect how Jetty 12 behaves under a purely async system using virtual thread pools (which can be passed via :thread-pool), so it will help to be able to set them without having to rebuild the connector via a custom configurator. I will put together an example project with some load testing, this is something we recently experienced in our prod environment when moving from Ring 1.13.0 to 1.14.2, but should be able to duplicate it in isolation. |
e2fa56e
to
276431e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor changes/questions, but overall looks good.
:keystore-scan-interval - if not nil, the interval in seconds to scan for an | ||
updated keystore | ||
:thread-pool - custom thread pool instance for Jetty to use | ||
:virtual-pool? - use a VirtualThreadPool (requires Java 19+) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:virtual-threads?
is a better name for the option I think:
:virtual-pool? - use virtual threads (requires Java 19+)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I like it better, will open a separate PR for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I wasn't expecting that comment to get through to this review. That was from my previous (incomplete and pending) review.
(defn- create-virtual-threadpool [options] | ||
(let [max-threads (options :max-threads 50) | ||
pool (VirtualThreadPool. max-threads)] | ||
pool)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be more concisely written as:
(defn- create-virtual-threadpool [options]
(VirtualThreadPool. (:max-threads options 50)))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll keep this in mind for a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! That was from my previous (incomplete) review. I wasn't expecting it to still have those comments after you updated the code.
(let [acceptors (options :acceptor-threads -1) | ||
selectors (options :selector-threads -1)] | ||
(ServerConnector. server (int acceptors) (int selectors) | ||
^{:tag "[Lorg.eclipse.jetty.server.ConnectionFactory;"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ^{:tag "..."}
and not ^"..."
?
:acceptor-threads - the number of acceptor threads to use (default -1, i.e. defer to Jetty) | ||
:selector-threads - the number of selector threads to use (default -1, i.e. defer to Jetty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines go over 80 characters. Probably better to have:
:acceptor-threads - the number of acceptor threads to use
:selector-threads - the number of selector threads to use
If these options aren't set, I think a sensible default is implied.
.github/workflows/test.yml
Outdated
strategy: | ||
matrix: | ||
java_version: [17, 19, 21, 24] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes to test.yml
aren't necessary for the acceptor/selector options, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct, I'll remove these for now
276431e
to
bc58578
Compare
The scope of the PR is now reduced to just adding the acceptor/selector threads configuration capability, maintaining prior behavior but making these values now more easily configurable. |
Thanks! Finally, can you change the commit message to:
This puts the commit message subject under 50 characters. Once that's done, I'll merge and cut a new 1.15 beta release. Regarding the |
bc58578
to
7e63583
Compare
I just changed the commit message. Regarding the |
Merged. I'll cut a new beta release soon. Thanks for the PR! |
Java's virtual threads (Project Loom) provide concurrency improvements with lower memory overhead compared to platform threads. This is particularly beneficial for I/O-heavy applications where thousands of concurrent connections are common.Different workloads benefit from different acceptor/selector thread configurations. High-throughput scenarios may need more acceptors, while I/O-intensive workloads benefit from tuned selector counts.
This PR makes it very easy to
use VirtualThreadPool as well astune the acceptor/selector threads, otherwise the user has to rely in configuring the server via configurator and re-create the ServerConnector from scratch with custom selector/acceptor values.Changes included:
- Virtual Thread Pool Support: New :virtual-pool? option creates a VirtualThreadPool instead of QueuedThreadPool (requires Java 19+)- VirtualThreadPool Defaults: Virtual thread pools benefit from fewer acceptor threads (1) since virtual threads have lower overhead. Default selector count matches available processors for optimal I/O handling if not specified.- CI Test Matrix: Extended test matrix to cover Java 17, 19, 21, and 24 with proper preview feature handling, since VirtualThreadPool is supported in Java 19 (preview), or Java >= 21, but not on Java 17.