-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Response refactor, increase requests-per-second, body type handling #2896
Conversation
Below is data for ssl connections. Again, significant speed increases in most of the body types/sizes.
|
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
@@ -12,6 +12,13 @@ module Puma | |||
# | |||
module Request | |||
|
|||
LEN_BUFFER = 1_024 * 256 | |||
STRM_LEN_MAX = 1_024 * 1_024 * 4 |
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.
What is "strm"?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your suggestions
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like if we used "socket" in the code over "skt"
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.
Good idea. See above comment.
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.
@dentarg - fixed
Just some comments from the phone, haven't looked through the actual changes. Not sure when I'll be able to do that. |
3dd99b0
to
9f5b979
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be any benefit in using Ruby’s IO::Buffer
if it’s available?
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 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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Rack 3.0.0 released…
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See additional commit test_rack_server.rb - add tests for Rack::Chunked middleware
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. |
fcf2575
to
04de30a
Compare
Co-authored-by: Guillermo Iguaran <guilleiguaran@gmail.com>
Co-authored-by: Guillermo Iguaran <guilleiguaran@gmail.com>
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 information
master - 0e832e7
View Full Result
This PR - c32f8ff
View Full Result
|
@JuanitoFatas Thanks for the data. |
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. |
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_path
on 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_write
intofast_write_response
andfast_write_str
. Refactor. Compute array bodies'Content-Length
only 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
lines
toio_buffer
, which is what it is. Renameio
tosocket
.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.