Skip to content

Give each Dispatcher worker its own Supervisor#2875

Closed
wjoel wants to merge 1 commit intotypelevel:series/3.xfrom
wjoel:dispatch-supervisor-per-cpu
Closed

Give each Dispatcher worker its own Supervisor#2875
wjoel wants to merge 1 commit intotypelevel:series/3.xfrom
wjoel:dispatch-supervisor-per-cpu

Conversation

@wjoel
Copy link
Copy Markdown
Contributor

@wjoel wjoel commented Mar 14, 2022

In programs that use the same dispatcher a lot, the stateRef in Supervisor ends up being a hotspot, and a significant amount of time is spent in spin attempting 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 Dispatcher per 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 Dispatcher by creating one Supervisor for 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:

DispatcherBenchmark.scheduling    1000  thrpt   20  167.852 ± 1.801  ops/min

This PR:

DispatcherBenchmark.scheduling    1000  thrpt   20  600.486 ± 6.002  ops/min

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 Dispatcher a tiny bit less cheap.

@wjoel
Copy link
Copy Markdown
Contributor Author

wjoel commented Mar 14, 2022

I think I'll need some help with that test failure.

@djspiewak
Copy link
Copy Markdown
Member

Holy crap. Those numbers are legit.

@djspiewak
Copy link
Copy Markdown
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.

@wjoel wjoel closed this Mar 14, 2022
@wjoel wjoel deleted the dispatch-supervisor-per-cpu branch March 14, 2022 19:52
@wjoel
Copy link
Copy Markdown
Contributor Author

wjoel commented Mar 14, 2022

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.

Woops, didn't mean to close this one, but ended up with #2876 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants