Response refactor, increase requests-per-second, body type handling #2896
Conversation
MSP-Greg
added
perf
waiting-for-review
|
Below is data for ssl connections. Again, significant speed increases in most of the body types/sizes. |
MSP-Greg
changed the title
|
All of the above was done with Windows WSL2/Ubuntu 22.04. But, that's not native, essentially running in a VM. So, I ran the benchmarks on GitHub Actions, which are 2 core VM's running on larger systems. They list several processors, I looked up two, one had 12 cores, another 32. I ran the test a few times to account for 'noisy neighbors'. Data below. Again, almost all response body type/size combinations run faster than current master... wrk latency data, grouped by response body size
|
lib/puma/request.rb
Outdated
| module Request | ||
|
|
||
| LEN_BUFFER = 1_024 * 256 | ||
| STRM_LEN_MAX = 1_024 * 1_024 * 4 |
There was a problem hiding this comment.
Short for stream. Some of the code here calls the IOBuffer object stream, and some of the current code calls it lines.
Would it be better to call it io_buffer?
Also, much of the code uses io for the client socket. Should it be called socket?
And, thanks again, when I'm writing code, I often use short names. Need to stop doing that.
There was a problem hiding this comment.
See the last commit, renamed as mentioned above. Also, I often needed to remind myself what res_info was. Changed it to resp_info. I see res, and I don't immediately think of response...
lib/puma/request.rb
Outdated
| LEN_BUFFER = 1_024 * 256 | ||
| STRM_LEN_MAX = 1_024 * 1_024 * 4 | ||
|
|
||
| SKT_WRITE_ERR_MSG = "Socket timeout writing data" |
There was a problem hiding this comment.
I would like if we used "socket" in the code over "skt"
There was a problem hiding this comment.
Good idea. See above comment.
|
Just some comments from the phone, haven't looked through the actual changes. Not sure when I'll be able to do that. |
MSP-Greg
force-pushed
the
00-response-refactor
branch
from
3dd99b0 to
9f5b979
Compare
MSP-Greg
force-pushed
the
00-response-refactor
branch
from
522cfd7 to
6c03e83
Compare
| class IOBuffer < String | ||
| def append(*args) | ||
| args.each { |a| concat(a) } | ||
| class IOBuffer < StringIO |
There was a problem hiding this comment.
Would there be any benefit in using Ruby’s IO::Buffer if it’s available?
There was a problem hiding this comment.
I looked at the API, and it's a bit more 'low level' than StringIO. I switched to StringIO because it's more 'byte' oriented (or encoding agnostic), and it works reasonably well with both String & IO objects.
I haven't benched anything, but I normally use Ruby master locally, so I suppose...
|
@MSP-Greg I think is waiting on some variable renames but otherwise looks good to go? |
|
@nateberkopec working on this re PR #2892. Very busy with a (paid) project fixing Ruby code I wrote 10+ years ago. I was pretty new to Ruby then, mostly did C# and js back then... |
|
Question: Should an empty response body return a |
|
That's a good question, HTTP allows it and some clients might want to know it (e.g for verification), but I'm not sure how common is having it for empty body responses. |
MSP-Greg
force-pushed
the
00-response-refactor
branch
from
6c03e83 to
d156ca5
Compare
|
@guilleiguaran I updated this, adding checks for if the body is a File io without a Content-Length. Thanks for all your help here and in PR #2892... Also, please see eb5d7a35ce8b, that's the code similar to your PR. I start with From what I can tell,
Now it's being set for a body that's either |
| length = 0 | ||
| if array_body = app_body.to_ary | ||
| body = array_body.map { |part| length += part.bytesize; part } | ||
| elsif app_body.is_a?(::File) && app_body.respond_to?(:size) |
There was a problem hiding this comment.
Perhaps we should check if app_body responds to to_path instead of is_a?(::File) to conform with the Rack SPEC?
The SPEC mentions 'File-like' and to_path but doesn't mention the File class specifically.
There was a problem hiding this comment.
So I'm looking at ActionDispatch::Response::FileBody, and seeing that it is not a File, but responds to to_path and each is patched to return 16k chunks. Since it's not a File, we can't get the size from it, so it has to be chunked?
There was a problem hiding this comment.
I added another commit to handle to_path. Reason for the File object check is that Puma can use copy_stream for File objects, which would typically be faster than calling any kind of each iteration.
MSP-Greg
force-pushed
the
00-response-refactor
branch
from
9d67a5b to
a77f356
Compare
lib/puma/request.rb
Outdated
| end | ||
| end | ||
| body.close unless body.closed? | ||
| elsif body.respond_to?(:to_ary) && body.length == 1 |
There was a problem hiding this comment.
I'm getting an exception here when the response coming from Rails is already chunked (Rails app sets Transfer-Encoding: chunked header so Puma doesn't try to calculate the Content-Length), in this case body reach this point being a RackBody instance that responds to to_ary but don't implements length:
2022-09-05 01:25:12 -0700 Read: #<NoMethodError: undefined method `length' for #<ActionDispatch::Response::RackBody:0x000000014ed57948>>
There was a problem hiding this comment.
Thanks for checking/testing.
Fixed - changed body.respond_to?(:to_ary) to `body.is_a?(::Array)
Maybe the PR needs more tests with RackBody. I'll see about adding some...
There was a problem hiding this comment.
See additional commit test_rack_server.rb - add tests for Rack::Chunked middleware
MSP-Greg
force-pushed
the
00-response-refactor
branch
from
894bf4c to
726ce6d
Compare
|
Rebased for Rack 3, added commit |
|
Thank you for the results, sorry they take so long. For the most part, your data tracks with mine, in that many of the speed increases are significant. I'm wondering if something is wrong with the code, as the Ruby 2.7 string data is much different that Ruby 3.1. I've used Ruby master for most of my local testing. Do you recall if latency summary section output before the main summary shown above listed the string body sizes as similar to the other sized bodies? Example from my output: JFYI, the body size number isn't terribly accurate due to the limited digits in wrk's 'bytes read' output. EDIT: I'm going to test on Ruby 2.7 locally. |
MSP-Greg
force-pushed
the
00-response-refactor
branch
from
fcf2575 to
04de30a
Compare
Co-authored-by: Guillermo Iguaran <[email protected]>
Co-authored-by: Guillermo Iguaran <[email protected]>
MSP-Greg
force-pushed
the
00-response-refactor
branch
from
04de30a to
c32f8ff
Compare
|
Late to the party, but I tried this on Ruby 3.2.0 Preview 2. Following are the results. My laptop informationmaster - 0e832e7View Full ResultThis PR - c32f8ffView Full Result |
|
@JuanitoFatas Thanks for the data. |
MSP-Greg
changed the title
|
I updated the notes in the first post, and mentioned two possible issues, correct client (socket) error handling, and possible issues if Rack::Sendfile middleware is used. Otherwise, I think it's time to own it and merge. |
7 participants
Description
This PR refactors response generation, speeding up operations with various body types and sizes. It also adds code to better handle the various body types returned by Rails and/or Rack. Much of this has been worked on by @guilleiguaran, so thanks for his work on it (see #2892). Essentially, body types fall into five categories:
IO.copy_stream.to_path- Puma will attempt to open the file, and if successful, will send as above.call.This PR has been around for a while, and I'm sure I've run millions of requests thru it. Two items:
to_pathon a response body. This PR has been updated to do so when the body responds to it, but isn't a File object. It opens the file (based on the name into_path), and treats it as a File/IO object. This may cause issues with the Rack::Sendfile middleware.Below summarizes performance using the code in benchmarks/local, all using:
benchmarks/local/response_time_wrk.sh -w2 -t5:5 -s tcp6 -Y
wrk -t8 -c16 -d10s
Server cluster mode -w2 -t5:5, bind: tcp6
ruby 3.2.0dev (2022-09-12T13:13:32Z master 2aa8edaec7) +YJIT [x86_64-linux]
Both include 'test/rackup/ci_*.ru files - cache response bodies' (#2952)
Windows WSL2/Ubuntu 22.04
There are significant speed increases for almost all body types/sizes, with the exception of 'single string' and 'io/file' bodies, which show smaller increases.
The commits:
io_buffer.rb - change to using StringIO - allows IOBuffer to be used both as a string and as an IO. Previously it was subclassed from
String.request.rb - refactor - Split
fast_writeintofast_write_responseandfast_write_str. Refactor. Compute array bodies'Content-Lengthonly when it's not provided by app. See Compute Content-Length header correctly when response body is an instance of Rack::BodyProxy #2892 by @guilleiguaran. Only an Array responds to#to_ary, so it is used for the body type check.client.rb - small refactor
All three of the below are small refactors
test_persistent.rb - chunked tests must be enum, not array
test_rack_server.rb - Add Rack::BodyProxy tests - from PR Compute Content-Length header correctly when response body is an instance of Rack::BodyProxy #2892.
test_puma_server.rb - always start one thread
request.rb - rename variables - Rename
linestoio_buffer, which is what it is. Renameiotosocket.Most of the remaining commits involve code to handle various body types, along with tests.
Please feel free to test these changes.
Closes #2892, closes #2703, closes #2457
Your checklist for this pull request
[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.