I've created two gists with test/benchmark data.

• Puma keep-alive rev2 latency summary

This contains summaries generated from the 'raw data' gist. Example below. The 'Mult' column is the number of concurrent requests sent to the server. For the below, we have 40 'bins' (8 workers * 5 threads), so each row shows behavior with various request overloads. The 'rps %' (requests per second) column is normalized to 100%, and the latency columns are normalized to 1. '00-long-tail-pr-2' is the fork branch, '74b7f010d793' is the commit that is 'Puma 'master' with changes needed to run the benchmarks.

Runs were done for both keep-alive and non-keep-alive connections. Each file contains runs for 8, 4, 2, and 0 workers. The files/docs are named by the response time, which is varied from 3 to 600 mS.

The below shows current master latency with a 3 mS response time. Note the increase in times for 95% thru 100% when Puma is overloaded (mults 1.50 thru 4.00).

Branch: 74b7f010d793     Puma: -w8  -t5:5  dly 0.003
                                ───────────────────── Hey Latency ───────────────────── Long Tail
Mult/Conn  requests   rps %     10%    25%    50%    75%    90%    95%    99%    100%   100% / 10%
4.00 160    16000      69.6     0.28   0.28   0.28   0.29   1.76  10.65  11.01  17.62     64.06
3.00 120    12000      71.2     0.37   0.37   0.38   0.40   1.52   9.86  10.39  16.12     43.97
2.00  80     8000      69.9     0.55   0.55   0.55   0.57   0.65   7.83   8.18  17.85     32.45
1.50  60     6000      71.1     0.73   0.73   0.73   0.76   0.89   6.09   6.98  14.73     20.09
1.00  40     4000      73.2     1.10   1.10   1.10   1.13   1.20   1.30   2.30  12.73     11.58
0.50  20     2000      43.1     1.10   1.10   1.13   1.13   1.17   1.20   1.47   3.10      2.82

• Puma keep-alive rev2 raw data

This gist contains the raw logs from the benchmarks. They also have sections for info about the max size of the Reactor and ThreadPool, along with detail of request spread across workers.

Branch: 00-long-tail-pr-2 Puma: -w8  -t5:5  dly 0.003    ───────────────── Worker Request Info ─────────────────
                         ── Reactor ──   ── Backlog ──    Std                       % deviation
Mult/Conn  requests       Min     Max     Min     Max     Dev                       from 12.50%
4.00 160    16000         13      16      17      27      0.49    -0.6  -0.4  -0.4  -0.2   0.0   0.1   0.8   0.8
3.00 120    12000         15      18      12      17      0.74    -1.4  -0.6  -0.1  -0.1   0.2   0.3   0.4   1.3
2.00  80     8000          6       8       6      12      0.99    -1.2  -0.8  -0.8  -0.6  -0.2   0.7   1.3   1.6
1.50  60     6000          7       8       4       8      0.85    -1.1  -0.9  -0.5  -0.4  -0.1   0.8   0.9   1.3
1.00  40     4000          3       4       2       4      4.66    -9.0  -5.0  -2.4   1.4   2.2   4.0   4.2   4.6
0.50  20     2000          2       3       2       2     25.98   -50.0 -20.0 -10.0 -10.0  10.0  20.0  30.0  30.0

@schneems

Good day.

I think I may pull two of the last commits (client.rb - continue reading request body if readable and CI: test_puma_server.rb - add tests for proper request body reading) from this. I noticed it while testing.

Currently, if a request is longer than 64 kb, Puma will push the request into the reactor. I thought that was odd behavior. If the socket is readable, Puma should continue to read the socket and see if the complete request body has been sent.

Fixed the issue, but the test often fails. It passes on retry on Ubuntu, but often fails completely on macOS. I've seen the same locally when running tests parallel.

A few other items:

1. The code will close a keep-alive connection after 25 requests. It can be adjusted thru config. Not sure if 25 is a good default value.
2. As you may recall, there have been some previous iterations of this regarding the select-loop delay. I settled on this as the minimal amount that seems to work with various app response times. I tested from 3 mS to 600 mS.
3. Since I've only tested with my local systems, some real world testing would be helpful.
4. Most of the testing I've done uses hey. When running ab or wrk locally, I get intermittent exceeding long responses at the high end (95% is okay). But, I see the same with several Puma versions, along with this PR. Maybe something odd with my system. not sure. I also use Ruby head for almost all my testing.

The code will close a keep-alive connection after 25 requests. It can be adjusted thru config. Not sure if 25 is a good default value.

Seems good. We can tune this in the future. My approach would be looking at what other tools have done and seeing if there's some discussion around pros/cons. I don't think this process should be blocking though.

Nginx uses 1000 https://nginx.org/en/docs/http/ngx_http_core_module.html#keepalive_requests. The docs mention memory being a tradeoff. I'm not sure what the memory overhead of a keepalive connection is in puma. I would also think the bigger memory concern is the total number of retained requests and closing after N requests is only a proxy config for reducing the retained requests number.

As you may recall, there have been some previous iterations of this regarding the select-loop delay. I settled on this as the minimal amount that seems to work with various app response times. I tested from 3 mS to 600 mS.

I'm good with your best judgement here. IMHO the old values were turned for the old behavior. I think you've done a lot with benchmarks and friends. My preference is to get this into the real world and get feedback.

Since I've only tested with my local systems, some real world testing would be helpful.

Agreed

Most of the testing I've done uses hey. When running ab or wrk locally, I get intermittent exceeding long responses at the high end (95% is okay). But, I see the same with several Puma versions, along with this PR. Maybe something odd with my system. not sure. I also use Ruby head for almost all my testing.

Good to note. Since you're seeing it in older versions, I wouldn't consider this a regression so it's non blocking. Might be worth opening an issue to track, ideally isolate to some specific conditions.

If anyone is interested in trying this, I believe the following change to the Gemfile will work:

gem "puma", github: "puma/puma", ref: "pull/3678/head"

This is included in 7.0.0.pre1, which is available as a gem.

Is it possible that this fix results in much lower puma resource consumption/higher performance even if you aren't getting traffic using keep-alive?

I have upgraded my app to puma 7.0.0.pre1. it still is running on heroku, but still running on "router 1.0" (Server: Cowboy), which means there should not be persistent connections/keep-alive involved if i understand it right.

Nonetheless, it looks to me like for the same volume of traffic, we are using many fewer max/peak puma workers. We were usually using 4-5 before, with fairly regular higher spikes to 7-15. Now usually just 1-2 puma workers for same traffic, with seldom any spikes and they are seldom higher than 5.

i don't have quite enough data to be sure of this -- this is just using heroku "metrics" dashboard, assuming it is working properly (including for monitoring num puma workers), and without having captured actual data from before we deployed puma 7.0.0.pre1 (but I know we used to have more than 1-2 threads in use).

But thought I'd put this out there in case anyone else is seeing anything. Is it even plausible that "long tail" improvements could apply to more than just keep-alive? Whatever is going on, it is certainly very welcome that my puma thread usage has gone down so much.

(OR has puma 7.0.0 broken heroku's 'puma pool usage' monitor, would be another possibility!)

Is it possible that this fix results in much lower puma resource consumption/higher performance even if you aren't getting traffic using keep-alive?

FWIW I noticed that my memory went down when I deployed this, but I made the mistake of changing several things at a time so it's not conclusive.

For feedback or general discussion please post at #3697. Only people subscribed to this thread will get comments on it, and I like to focus comments on the act of approving or closing the PR. Now that it's merged if we can move comments somewhere else, it would be better.

this seems to replace efficient condition

Could you open the an issue or PR for discussion. I think that we always want to wait a tiny bit which triggers the gitlab patch behavior (to better balance out worker scheduling by the OS). We could replace with a condition variable + always waiting a set amount or some other kind of delay. I think perhaps there's also a bit of tuning the accept rate going on here based on load (so workers with more current capacity will call accept slightly faster) but it's all tangled.

Either way, please move conversation elsewhere, and it would be good to talk about "when someone wants to modify or optimize this code, what should they be looking at?" There's also a bunch of benchmarks checked in now, perhaps one of them shows some differences.