net/http: unfurl persistConnWriter's underlying writer #30390
Conversation
googlebot
added
the
cla: yes
|
This PR (HEAD: a2bdd12) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/163737 to see it. Tip: You can toggle comments from me using the |
|
Message from Chris Marchesi: Patch Set 1: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
|
Message from Emmanuel Odeke: Patch Set 1: (15 comments) Thank you for this work Chris! I've added some suggestions, PTAL. Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
|
Message from Emmanuel Odeke: Patch Set 1: Let's also add some benchmark results for FileAndServer to this CL, if they make sense before the follow-up CL. Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
vancluever
changed the title
|
This PR (HEAD: a444ccf) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/163737 to see it. Tip: You can toggle comments from me using the |
|
Message from Emmanuel Odeke: Patch Set 3: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
|
Message from Chris Marchesi: Patch Set 1: (15 comments) Hey Emmanuel, all done (will follow up on the latest patchset too) I also have benchmarks and it may be worth posting them. I'll reply on that comment. Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
|
Message from Chris Marchesi: Patch Set 3:
Here are benchmarks for both Linux and Darwin again but just using this CL. Looks like a lot of the gains are just from the implementation alone. #30377 has benchmarks for both CLs that were updated last night after the changes to the actual benchmark code itself. linux/amd64: benchmark old ns/op new ns/op delta benchmark old MB/s new MB/s speedup benchmark old allocs new allocs delta benchmark old bytes new bytes delta darwin/amd64: benchmark old ns/op new ns/op delta benchmark old MB/s new MB/s speedup benchmark old allocs new allocs delta benchmark old bytes new bytes delta Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
|
Message from Chris Marchesi: Patch Set 3: (1 comment) Added the speedups for both Linux and Darwin. Let me know if that works! Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
|
Message from Chris Marchesi: Patch Set 3:
Just waiting for gopherbot. If it didn't take from the GH PR in a few minutes I'll update here. Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
|
Message from Emmanuel Odeke: Patch Set 4: Run-TryBot+1 Code-Review+2 Awesome, LGTM! Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
|
Message from Gobot Gobot: Patch Set 4: TryBots beginning. Status page: https://farmer.golang.org/try?commit=1393de5a Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
|
Message from Gobot Gobot: Patch Set 4: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
|
Message from Emmanuel Odeke: Patch Set 4: Hey Chris, would you mind rebasing from master as this currently registers as a merge conflict? Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
Make persistConnWriter implement io.ReaderFrom, via an io.Copy on the underlying net.Conn. This in turn enables it to use OS level optimizations such as sendfile. This has been observed giving performance gains even in the absence of ReaderFrom, more than likely due to the difference in io's default buffer (32 KB) versus bufio's (4 KB). Updates #30377.
|
This PR (HEAD: a6f67cd) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/163737 to see it. Tip: You can toggle comments from me using the |
|
Message from Chris Marchesi: Patch Set 5:
Hey Emmanuel, just did it. Oddly enough I wasn't asked to resolve any conflicts, so hopefully this clears it up! Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
|
Message from Emmanuel Odeke: Patch Set 5: Run-TryBot+1
Yes, that rebase cleared it up and after this is submitted, please do rebase the Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
|
Message from Gobot Gobot: Patch Set 5: TryBots beginning. Status page: https://farmer.golang.org/try?commit=2cc31de4 Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
|
Message from Gobot Gobot: Patch Set 5: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/163737. |
Make persistConnWriter implement io.ReaderFrom, via an io.Copy on the underlying net.Conn. This in turn enables it to use OS level optimizations such as sendfile. This has been observed giving performance gains even in the absence of ReaderFrom, more than likely due to the difference in io's default buffer (32 KB) versus bufio's (4 KB). Speedups on linux/amd64: benchmark old MB/s new MB/s speedup BenchmarkFileAndServer_16MB/NoTLS-4 662.96 2703.74 4.08x BenchmarkFileAndServer_16MB/TLS-4 552.76 1420.72 2.57x Speedups on darwin/amd64: benchmark old MB/s new MB/s speedup BenchmarkFileAndServer_16MB/NoTLS-8 357.58 1972.86 5.52x BenchmarkFileAndServer_16MB/TLS-8 346.20 1067.41 3.08x Updates #30377. Change-Id: Ic88d4ac254f665223536fcba4d551fc32ae105b6 GitHub-Last-Rev: a6f67cd GitHub-Pull-Request: #30390 Reviewed-on: https://go-review.googlesource.com/c/go/+/163737 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/163737 has been merged. |
Make persistConnWriter implement io.ReaderFrom, via an io.Copy on the
underlying net.Conn. This in turn enables it to use OS level
optimizations such as sendfile.
This has been observed giving performance gains even in the absence
of ReaderFrom, more than likely due to the difference in io's default
buffer (32 KB) versus bufio's (4 KB).
Speedups on linux/amd64:
benchmark old MB/s new MB/s speedup
BenchmarkFileAndServer_16MB/NoTLS-4 662.96 2703.74 4.08x
BenchmarkFileAndServer_16MB/TLS-4 552.76 1420.72 2.57x
Speedups on darwin/amd64:
benchmark old MB/s new MB/s speedup
BenchmarkFileAndServer_16MB/NoTLS-8 357.58 1972.86 5.52x
BenchmarkFileAndServer_16MB/TLS-8 346.20 1067.41 3.08x
Updates #30377.