Give each Dispatcher worker its own Supervisor#2875
Closed
wjoel wants to merge 1 commit intotypelevel:series/3.xfrom
Closed
Give each Dispatcher worker its own Supervisor#2875wjoel wants to merge 1 commit intotypelevel:series/3.xfrom
wjoel wants to merge 1 commit intotypelevel:series/3.xfrom
Conversation
Contributor
Author
|
I think I'll need some help with that test failure. |
Member
|
Holy crap. Those numbers are legit. |
Member
|
I'll take a look at the failure in a bit. Would you mind targeting this at series/3.3.x though? It's forward compatible so we can release it much sooner this way. |
Contributor
Author
Woops, didn't mean to close this one, but ended up with #2876 instead. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In programs that use the same dispatcher a lot, the
stateRefinSupervisorends up being a hotspot, and a significant amount of time is spent inspinattempting to update it.While the documentation says that dispatchers are cheap and should be constructed often, web servers like blaze-server, jetty-server, http4s-netty, and fs2-netty, use one
Dispatcherper server.I don't think that's wrong, and I've also experienced problems with "dispatcher already shutdown" when trying to use shorter-lived ones, as reported in #2727
Given that, we can fix the hotspot in
Dispatcherby creating oneSupervisorfor each CPU/worker, which is what this PR does.Results (from a Ryzen 5900X, I expect the improvement to scale with the number of cores) look good:
Baseline on
series/3.x:This PR:
I'm also doing some work with Techempower HTTP benchmarks, and this change results in a 5% increase in performance on plaintext using blaze (but interestingly, gets the best result on 16384 concurrent requests while it otherwise peaks at 4096 concurrent requests), and an even greater increase when using a modified http4s-netty backend.
It's possible that there's a better fix for the underlying issue, but this one is simple, and a 3x improvement in (benchmark) performance makes it worth considering, in my opinion. Even if it does making constructing a
Dispatchera tiny bit less cheap.