net/http: let Transport request body writes use sendfile #30378
Conversation
googlebot
added
the
cla: yes
|
This PR (HEAD: 9499223) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/163599 to see it. Tip: You can toggle comments from me using the |
|
Message from Gobot Gobot: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Dave Cheney: Patch Set 1: (1 comment) This logic is subtle and likely to be broken in the future. Can you please add a test. Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Chris Marchesi: Patch Set 1:
Sounds good on both fronts (the tests and separating the ReaderFrom implementation into a different change). I was having issues trying to figure out how to test this before due to all the layers involved (ie: the root cause cascading all the way down to the underlying connection), but got a test together now that uses a custom TCPConn to detect calls to ReadFrom and check the passed in reader. The test is kind of coupled to what will be both of the changes once the persistConnWriter change is spun off, so I'll make sure to break that out during the update too. Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Brad Fitzpatrick: Patch Set 1: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
bradfitz
changed the title
|
Message from Chris Marchesi: Patch Set 2: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
This PR (HEAD: 1531155) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/163599 to see it. Tip: You can toggle comments from me using the |
|
This PR (HEAD: 6751de1) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/163599 to see it. Tip: You can toggle comments from me using the |
|
Message from Chris Marchesi: Patch Set 4: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
This PR (HEAD: 335abf9) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/163599 to see it. Tip: You can toggle comments from me using the |
|
This PR (HEAD: 2a6e85a) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/163599 to see it. Tip: You can toggle comments from me using the |
|
Message from Chris Marchesi: Patch Set 6: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Chris Marchesi: Patch Set 7: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Chris Marchesi: Patch Set 7: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Chris Marchesi: Patch Set 8: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Chris Marchesi: Patch Set 9: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Chris Marchesi: Patch Set 9: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
This PR (HEAD: 097006b) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/163599 to see it. Tip: You can toggle comments from me using the |
|
This PR (HEAD: fc6d68a) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/163599 to see it. Tip: You can toggle comments from me using the |
|
Message from Emmanuel Odeke: Patch Set 13: (11 comments) Nice work Chris! I've added some suggestions PTAL. Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
This PR (HEAD: 9a541bb) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/163599 to see it. Tip: You can toggle comments from me using the |
|
Message from Chris Marchesi: Patch Set 13: (10 comments) Hey Emmanuel! Made the edits to the tests. Replied to the commit comments too - I think there's some confusion as to what's going on here in that it's the reader that we're targeting here and not the writer. Let me know! Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Emmanuel Odeke: Patch Set 15: Run-TryBot+1 Code-Review+2 Thank you Chris, LGTM! Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Gobot Gobot: Patch Set 15: TryBots beginning. Status page: https://farmer.golang.org/try?commit=7b10b182 Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Gobot Gobot: Patch Set 15: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Emmanuel Odeke: Patch Set 15: I'll wait until CL 163737 is updated and merged and then we can tack this one on. Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Emmanuel Odeke: Patch Set 15: Please get the latest from master to pull in CL 163737, then rebase. For now we can submit the tests as they are but later on(after submitting this CL) it would be great to refactor the test and reuse components such as newFileFunc, newBytes* etc. Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Emmanuel Odeke: Patch Set 15:
Oh and finally, please post up in this commit message benchmark summaries from benchstat in their full form. That makes it easy for anyone reading through commits to quantify and qualify the benefits without having to go digging through CL conversations :) Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
This PR (HEAD: cd35c99) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/163599 to see it. Tip: You can toggle comments from me using the |
|
Message from Chris Marchesi: Patch Set 15:
Rebased and pushed now. I did actually take a stab at refactoring this this morning and then realized that these tests are actually based in the main http package whereas the transport tests are based in the external http_test package, so re-factoring them into helpers is a little less trivial. Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Emmanuel Odeke: Patch Set 16: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Emmanuel Odeke: Patch Set 16:
Yeah I realized that this might be tricky, which was why I had suggested to perhaps look at it after submission of this CL (at a later time). Just the benchmarks after -count=10 left and we'll be gucci to go. Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
This PR (HEAD: 059d5b6) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/163599 to see it. Tip: You can toggle comments from me using the |
|
This PR (HEAD: 46a2402) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/163599 to see it. Tip: You can toggle comments from me using the |
|
Message from Chris Marchesi: Patch Set 18:
Just posted the benchmarks. I included deltas to both https://golang.org/cl/163737 and https://golang.org/cl/163862 so that we have stats for both the incremental improvement that this CL makes between the ReaderFrom implementation, and the total improvement that both CLs make. Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Emmanuel Odeke: Patch Set 19: Thanks for the benchmarks Chris! You seemed to have pasted many runs of the benchmarks instead of the summarized/aggregated(but compiled multiple runs). What I meant by count=10 is the following: Before all these changes, please run: After incorporating these changes: Finally to get the results and that summary is what you'll paste here. Just that update and then we'll be good to go. Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Chris Marchesi: Patch Set 19:
Hey Emmanuel, just wanted to confirm you wanted a summary of the benchmarks from https://golang.org/cl/163862 onwards (so stats that summarize the total benefit of both this and https://golang.org/cl/163737)? Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Emmanuel Odeke: Patch Set 19:
Yes, please. Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Emmanuel Odeke: Patch Set 19:
Actually my apologies, I got confused seeing alloc/op vs allocs/op and thought that you had duplicated results. Your summary can suffice as is, but would nice with all the updates. Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
net.TCPConn has the ability to send data out using system calls such as sendfile when the source data comes from an *os.File. However, the way that I/O has been laid out in the transport means that the File is actually wrapped behind two outer io.Readers, and as such the TCP stack cannot properly type-assert the reader, ensuring that it falls back to genericReadFrom. This commit does the following: * Removes transferBodyReader and moves its functionality to a new doBodyCopy helper. This is not an io.Reader implementation, but no functionality is lost this way, and it allows us to unwrap one layer from the body. * The second layer of the body is unwrapped if the original reader was wrapped with ioutil.NopCloser, which is what NewRequest wraps the body in if it's not a ReadCloser on its own. The unwrap operation passes through the existing body if there's no nopCloser. Note that this depends on change https://golang.org/cl/163737 to properly function, as the lack of ReaderFrom implementation otherwise means that this functionality is essentially walled off. Benchmarks between this commit and https://golang.org/cl/163862, incorporating https://golang.org/cl/163737: linux/amd64: name old time/op new time/op delta FileAndServer_1KB/NoTLS-4 53.2µs ± 0% 53.3µs ± 0% ~ (p=0.075 n=10+9) FileAndServer_1KB/TLS-4 61.2µs ± 0% 60.7µs ± 0% -0.77% (p=0.000 n=10+9) FileAndServer_16MB/NoTLS-4 25.3ms ± 5% 3.8ms ± 6% -84.95% (p=0.000 n=10+10) FileAndServer_16MB/TLS-4 33.2ms ± 2% 13.4ms ± 2% -59.57% (p=0.000 n=10+10) FileAndServer_64MB/NoTLS-4 106ms ± 4% 16ms ± 2% -84.45% (p=0.000 n=10+10) FileAndServer_64MB/TLS-4 129ms ± 1% 54ms ± 3% -58.32% (p=0.000 n=8+10) name old speed new speed delta FileAndServer_1KB/NoTLS-4 19.2MB/s ± 0% 19.2MB/s ± 0% ~ (p=0.095 n=10+9) FileAndServer_1KB/TLS-4 16.7MB/s ± 0% 16.9MB/s ± 0% +0.78% (p=0.000 n=10+9) FileAndServer_16MB/NoTLS-4 664MB/s ± 5% 4415MB/s ± 6% +565.27% (p=0.000 n=10+10) FileAndServer_16MB/TLS-4 505MB/s ± 2% 1250MB/s ± 2% +147.32% (p=0.000 n=10+10) FileAndServer_64MB/NoTLS-4 636MB/s ± 4% 4090MB/s ± 2% +542.81% (p=0.000 n=10+10) FileAndServer_64MB/TLS-4 522MB/s ± 1% 1251MB/s ± 3% +139.95% (p=0.000 n=8+10) darwin/amd64: name old time/op new time/op delta FileAndServer_1KB/NoTLS-8 93.0µs ± 5% 96.6µs ±11% ~ (p=0.190 n=10+10) FileAndServer_1KB/TLS-8 105µs ± 7% 100µs ± 5% -5.14% (p=0.002 n=10+9) FileAndServer_16MB/NoTLS-8 87.5ms ±19% 10.0ms ± 6% -88.57% (p=0.000 n=10+10) FileAndServer_16MB/TLS-8 52.7ms ±11% 17.4ms ± 5% -66.92% (p=0.000 n=10+10) FileAndServer_64MB/NoTLS-8 363ms ±54% 39ms ± 7% -89.24% (p=0.000 n=10+10) FileAndServer_64MB/TLS-8 209ms ±13% 73ms ± 5% -65.37% (p=0.000 n=9+10) name old speed new speed delta FileAndServer_1KB/NoTLS-8 11.0MB/s ± 5% 10.6MB/s ±10% ~ (p=0.184 n=10+10) FileAndServer_1KB/TLS-8 9.75MB/s ± 7% 10.27MB/s ± 5% +5.26% (p=0.003 n=10+9) FileAndServer_16MB/NoTLS-8 194MB/s ±16% 1680MB/s ± 6% +767.83% (p=0.000 n=10+10) FileAndServer_16MB/TLS-8 319MB/s ±10% 963MB/s ± 4% +201.36% (p=0.000 n=10+10) FileAndServer_64MB/NoTLS-8 180MB/s ±31% 1719MB/s ± 7% +853.61% (p=0.000 n=9+10) FileAndServer_64MB/TLS-8 321MB/s ±12% 926MB/s ± 5% +188.24% (p=0.000 n=9+10) Updates #30377.
|
This PR (HEAD: 4a77dd1) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/163599 to see it. Tip: You can toggle comments from me using the |
|
Message from Emmanuel Odeke: Patch Set 20: Run-TryBot+1 Thank you Chris, LGTM! Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Gobot Gobot: Patch Set 20: TryBots beginning. Status page: https://farmer.golang.org/try?commit=49724c9b Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Chris Marchesi: Patch Set 20:
Ack! I just updated without the memory benchmarks. :P Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Chris Marchesi: Patch Set 20:
Alright, great. :) I'm sure there's plenty of history here if anyone wants to see the memory benchmarks and the other very specific deltas. Thanks again for all your help on this Emmanuel! Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
|
Message from Gobot Gobot: Patch Set 20: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/163599. |
net.TCPConn has the ability to send data out using system calls such as sendfile when the source data comes from an *os.File. However, the way that I/O has been laid out in the transport means that the File is actually wrapped behind two outer io.Readers, and as such the TCP stack cannot properly type-assert the reader, ensuring that it falls back to genericReadFrom. This commit does the following: * Removes transferBodyReader and moves its functionality to a new doBodyCopy helper. This is not an io.Reader implementation, but no functionality is lost this way, and it allows us to unwrap one layer from the body. * The second layer of the body is unwrapped if the original reader was wrapped with ioutil.NopCloser, which is what NewRequest wraps the body in if it's not a ReadCloser on its own. The unwrap operation passes through the existing body if there's no nopCloser. Note that this depends on change https://golang.org/cl/163737 to properly function, as the lack of ReaderFrom implementation otherwise means that this functionality is essentially walled off. Benchmarks between this commit and https://golang.org/cl/163862, incorporating https://golang.org/cl/163737: linux/amd64: name old time/op new time/op delta FileAndServer_1KB/NoTLS-4 53.2µs ± 0% 53.3µs ± 0% ~ (p=0.075 n=10+9) FileAndServer_1KB/TLS-4 61.2µs ± 0% 60.7µs ± 0% -0.77% (p=0.000 n=10+9) FileAndServer_16MB/NoTLS-4 25.3ms ± 5% 3.8ms ± 6% -84.95% (p=0.000 n=10+10) FileAndServer_16MB/TLS-4 33.2ms ± 2% 13.4ms ± 2% -59.57% (p=0.000 n=10+10) FileAndServer_64MB/NoTLS-4 106ms ± 4% 16ms ± 2% -84.45% (p=0.000 n=10+10) FileAndServer_64MB/TLS-4 129ms ± 1% 54ms ± 3% -58.32% (p=0.000 n=8+10) name old speed new speed delta FileAndServer_1KB/NoTLS-4 19.2MB/s ± 0% 19.2MB/s ± 0% ~ (p=0.095 n=10+9) FileAndServer_1KB/TLS-4 16.7MB/s ± 0% 16.9MB/s ± 0% +0.78% (p=0.000 n=10+9) FileAndServer_16MB/NoTLS-4 664MB/s ± 5% 4415MB/s ± 6% +565.27% (p=0.000 n=10+10) FileAndServer_16MB/TLS-4 505MB/s ± 2% 1250MB/s ± 2% +147.32% (p=0.000 n=10+10) FileAndServer_64MB/NoTLS-4 636MB/s ± 4% 4090MB/s ± 2% +542.81% (p=0.000 n=10+10) FileAndServer_64MB/TLS-4 522MB/s ± 1% 1251MB/s ± 3% +139.95% (p=0.000 n=8+10) darwin/amd64: name old time/op new time/op delta FileAndServer_1KB/NoTLS-8 93.0µs ± 5% 96.6µs ±11% ~ (p=0.190 n=10+10) FileAndServer_1KB/TLS-8 105µs ± 7% 100µs ± 5% -5.14% (p=0.002 n=10+9) FileAndServer_16MB/NoTLS-8 87.5ms ±19% 10.0ms ± 6% -88.57% (p=0.000 n=10+10) FileAndServer_16MB/TLS-8 52.7ms ±11% 17.4ms ± 5% -66.92% (p=0.000 n=10+10) FileAndServer_64MB/NoTLS-8 363ms ±54% 39ms ± 7% -89.24% (p=0.000 n=10+10) FileAndServer_64MB/TLS-8 209ms ±13% 73ms ± 5% -65.37% (p=0.000 n=9+10) name old speed new speed delta FileAndServer_1KB/NoTLS-8 11.0MB/s ± 5% 10.6MB/s ±10% ~ (p=0.184 n=10+10) FileAndServer_1KB/TLS-8 9.75MB/s ± 7% 10.27MB/s ± 5% +5.26% (p=0.003 n=10+9) FileAndServer_16MB/NoTLS-8 194MB/s ±16% 1680MB/s ± 6% +767.83% (p=0.000 n=10+10) FileAndServer_16MB/TLS-8 319MB/s ±10% 963MB/s ± 4% +201.36% (p=0.000 n=10+10) FileAndServer_64MB/NoTLS-8 180MB/s ±31% 1719MB/s ± 7% +853.61% (p=0.000 n=9+10) FileAndServer_64MB/TLS-8 321MB/s ±12% 926MB/s ± 5% +188.24% (p=0.000 n=9+10) Updates #30377. Change-Id: I631a73cea75371dfbb418c9cd487c4aa35e73fcd GitHub-Last-Rev: 4a77dd1 GitHub-Pull-Request: #30378 Reviewed-on: https://go-review.googlesource.com/c/go/+/163599 Run-TryBot: Emmanuel Odeke <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Emmanuel Odeke <[email protected]>
|
This PR is being closed because golang.org/cl/163599 has been merged. |
net.TCPConn has the ability to send data out using system calls such as
sendfile when the source data comes from an *os.File. However, the way
that I/O has been laid out in the transport means that the File is
actually wrapped behind two outer io.Readers, and as such the TCP stack
cannot properly type-assert the reader, ensuring that it falls back to
genericReadFrom.
This commit does the following:
Removes transferBodyReader and moves its functionality to a new
doBodyCopy helper. This is not an io.Reader implementation, but no
functionality is lost this way, and it allows us to unwrap one layer
from the body.
The second layer of the body is unwrapped if the original reader
was wrapped with ioutil.NopCloser, which is what NewRequest wraps the
body in if it's not a ReadCloser on its own. The unwrap operation
passes through the existing body if there's no nopCloser.
Note that this depends on change https://golang.org/cl/163737 to
properly function, as the lack of ReaderFrom implementation otherwise
means that this functionality is essentially walled off.
Benchmarks between this commit and https://golang.org/cl/163862,
incorporating https://golang.org/cl/163737:
linux/amd64:
name old time/op new time/op delta
FileAndServer_1KB/NoTLS-4 53.2µs ± 0% 53.3µs ± 0% ~ (p=0.075 n=10+9)
FileAndServer_1KB/TLS-4 61.2µs ± 0% 60.7µs ± 0% -0.77% (p=0.000 n=10+9)
FileAndServer_16MB/NoTLS-4 25.3ms ± 5% 3.8ms ± 6% -84.95% (p=0.000 n=10+10)
FileAndServer_16MB/TLS-4 33.2ms ± 2% 13.4ms ± 2% -59.57% (p=0.000 n=10+10)
FileAndServer_64MB/NoTLS-4 106ms ± 4% 16ms ± 2% -84.45% (p=0.000 n=10+10)
FileAndServer_64MB/TLS-4 129ms ± 1% 54ms ± 3% -58.32% (p=0.000 n=8+10)
name old speed new speed delta
FileAndServer_1KB/NoTLS-4 19.2MB/s ± 0% 19.2MB/s ± 0% ~ (p=0.095 n=10+9)
FileAndServer_1KB/TLS-4 16.7MB/s ± 0% 16.9MB/s ± 0% +0.78% (p=0.000 n=10+9)
FileAndServer_16MB/NoTLS-4 664MB/s ± 5% 4415MB/s ± 6% +565.27% (p=0.000 n=10+10)
FileAndServer_16MB/TLS-4 505MB/s ± 2% 1250MB/s ± 2% +147.32% (p=0.000 n=10+10)
FileAndServer_64MB/NoTLS-4 636MB/s ± 4% 4090MB/s ± 2% +542.81% (p=0.000 n=10+10)
FileAndServer_64MB/TLS-4 522MB/s ± 1% 1251MB/s ± 3% +139.95% (p=0.000 n=8+10)
darwin/amd64:
name old time/op new time/op delta
FileAndServer_1KB/NoTLS-8 93.0µs ± 5% 96.6µs ±11% ~ (p=0.190 n=10+10)
FileAndServer_1KB/TLS-8 105µs ± 7% 100µs ± 5% -5.14% (p=0.002 n=10+9)
FileAndServer_16MB/NoTLS-8 87.5ms ±19% 10.0ms ± 6% -88.57% (p=0.000 n=10+10)
FileAndServer_16MB/TLS-8 52.7ms ±11% 17.4ms ± 5% -66.92% (p=0.000 n=10+10)
FileAndServer_64MB/NoTLS-8 363ms ±54% 39ms ± 7% -89.24% (p=0.000 n=10+10)
FileAndServer_64MB/TLS-8 209ms ±13% 73ms ± 5% -65.37% (p=0.000 n=9+10)
name old speed new speed delta
FileAndServer_1KB/NoTLS-8 11.0MB/s ± 5% 10.6MB/s ±10% ~ (p=0.184 n=10+10)
FileAndServer_1KB/TLS-8 9.75MB/s ± 7% 10.27MB/s ± 5% +5.26% (p=0.003 n=10+9)
FileAndServer_16MB/NoTLS-8 194MB/s ±16% 1680MB/s ± 6% +767.83% (p=0.000 n=10+10)
FileAndServer_16MB/TLS-8 319MB/s ±10% 963MB/s ± 4% +201.36% (p=0.000 n=10+10)
FileAndServer_64MB/NoTLS-8 180MB/s ±31% 1719MB/s ± 7% +853.61% (p=0.000 n=9+10)
FileAndServer_64MB/TLS-8 321MB/s ±12% 926MB/s ± 5% +188.24% (p=0.000 n=9+10)
Updates #30377.