From 4a77dd1b80140274bf3ed20ad7465ff3cc06febf Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Sat, 23 Feb 2019 20:09:56 -0800 Subject: [PATCH] net/http: let Transport request body writes use sendfile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/net/http/transfer.go | 53 ++++++---- src/net/http/transfer_test.go | 188 ++++++++++++++++++++++++++++++++++ 2 files changed, 222 insertions(+), 19 deletions(-) diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go index e8a93e9137..7d73dc4fc0 100644 --- a/src/net/http/transfer.go +++ b/src/net/http/transfer.go @@ -53,19 +53,6 @@ func (br *byteReader) Read(p []byte) (n int, err error) { return 1, io.EOF } -// transferBodyReader is an io.Reader that reads from tw.Body -// and records any non-EOF error in tw.bodyReadError. -// It is exactly 1 pointer wide to avoid allocations into interfaces. -type transferBodyReader struct{ tw *transferWriter } - -func (br transferBodyReader) Read(p []byte) (n int, err error) { - n, err = br.tw.Body.Read(p) - if err != nil && err != io.EOF { - br.tw.bodyReadError = err - } - return -} - // transferWriter inspects the fields of a user-supplied Request or Response, // sanitizes them without changing the user object and provides methods for // writing the respective header, body and trailer in wire format. @@ -347,15 +334,18 @@ func (t *transferWriter) writeBody(w io.Writer) error { var err error var ncopy int64 - // Write body + // Write body. We "unwrap" the body first if it was wrapped in a + // nopCloser. This is to ensure that we can take advantage of + // OS-level optimizations in the event that the body is an + // *os.File. if t.Body != nil { - var body = transferBodyReader{t} + var body = t.unwrapBody() if chunked(t.TransferEncoding) { if bw, ok := w.(*bufio.Writer); ok && !t.IsResponse { w = &internal.FlushAfterChunkWriter{Writer: bw} } cw := internal.NewChunkedWriter(w) - _, err = io.Copy(cw, body) + _, err = t.doBodyCopy(cw, body) if err == nil { err = cw.Close() } @@ -364,14 +354,14 @@ func (t *transferWriter) writeBody(w io.Writer) error { if t.Method == "CONNECT" { dst = bufioFlushWriter{dst} } - ncopy, err = io.Copy(dst, body) + ncopy, err = t.doBodyCopy(dst, body) } else { - ncopy, err = io.Copy(w, io.LimitReader(body, t.ContentLength)) + ncopy, err = t.doBodyCopy(w, io.LimitReader(body, t.ContentLength)) if err != nil { return err } var nextra int64 - nextra, err = io.Copy(ioutil.Discard, body) + nextra, err = t.doBodyCopy(ioutil.Discard, body) ncopy += nextra } if err != nil { @@ -402,6 +392,31 @@ func (t *transferWriter) writeBody(w io.Writer) error { return err } +// doBodyCopy wraps a copy operation, with any resulting error also +// being saved in bodyReadError. +// +// This function is only intended for use in writeBody. +func (t *transferWriter) doBodyCopy(dst io.Writer, src io.Reader) (n int64, err error) { + n, err = io.Copy(dst, src) + if err != nil && err != io.EOF { + t.bodyReadError = err + } + return +} + +// unwrapBodyReader unwraps the body's inner reader if it's a +// nopCloser. This is to ensure that body writes sourced from local +// files (*os.File types) are properly optimized. +// +// This function is only intended for use in writeBody. +func (t *transferWriter) unwrapBody() io.Reader { + if reflect.TypeOf(t.Body) == nopCloserType { + return reflect.ValueOf(t.Body).Field(0).Interface().(io.Reader) + } + + return t.Body +} + type transferReader struct { // Input Header Header diff --git a/src/net/http/transfer_test.go b/src/net/http/transfer_test.go index 993ea4ef18..aa465d0600 100644 --- a/src/net/http/transfer_test.go +++ b/src/net/http/transfer_test.go @@ -7,8 +7,12 @@ package http import ( "bufio" "bytes" + "crypto/rand" + "fmt" "io" "io/ioutil" + "os" + "reflect" "strings" "testing" ) @@ -90,3 +94,187 @@ func TestDetectInMemoryReaders(t *testing.T) { } } } + +type mockTransferWriter struct { + CalledReader io.Reader + WriteCalled bool +} + +var _ io.ReaderFrom = (*mockTransferWriter)(nil) + +func (w *mockTransferWriter) ReadFrom(r io.Reader) (int64, error) { + w.CalledReader = r + return io.Copy(ioutil.Discard, r) +} + +func (w *mockTransferWriter) Write(p []byte) (int, error) { + w.WriteCalled = true + return ioutil.Discard.Write(p) +} + +func TestTransferWriterWriteBodyReaderTypes(t *testing.T) { + fileType := reflect.TypeOf(&os.File{}) + bufferType := reflect.TypeOf(&bytes.Buffer{}) + + nBytes := int64(1 << 10) + newFileFunc := func() (r io.Reader, done func(), err error) { + f, err := ioutil.TempFile("", "net-http-newfilefunc") + if err != nil { + return nil, nil, err + } + + // Write some bytes to the file to enable reading. + if _, err := io.CopyN(f, rand.Reader, nBytes); err != nil { + return nil, nil, fmt.Errorf("failed to write data to file: %v", err) + } + if _, err := f.Seek(0, 0); err != nil { + return nil, nil, fmt.Errorf("failed to seek to front: %v", err) + } + + done = func() { + f.Close() + os.Remove(f.Name()) + } + + return f, done, nil + } + + newBufferFunc := func() (io.Reader, func(), error) { + return bytes.NewBuffer(make([]byte, nBytes)), func() {}, nil + } + + cases := []struct { + name string + bodyFunc func() (io.Reader, func(), error) + method string + contentLength int64 + transferEncoding []string + limitedReader bool + expectedReader reflect.Type + expectedWrite bool + }{ + { + name: "file, non-chunked, size set", + bodyFunc: newFileFunc, + method: "PUT", + contentLength: nBytes, + limitedReader: true, + expectedReader: fileType, + }, + { + name: "file, non-chunked, size set, nopCloser wrapped", + method: "PUT", + bodyFunc: func() (io.Reader, func(), error) { + r, cleanup, err := newFileFunc() + return ioutil.NopCloser(r), cleanup, err + }, + contentLength: nBytes, + limitedReader: true, + expectedReader: fileType, + }, + { + name: "file, non-chunked, negative size", + method: "PUT", + bodyFunc: newFileFunc, + contentLength: -1, + expectedReader: fileType, + }, + { + name: "file, non-chunked, CONNECT, negative size", + method: "CONNECT", + bodyFunc: newFileFunc, + contentLength: -1, + expectedReader: fileType, + }, + { + name: "file, chunked", + method: "PUT", + bodyFunc: newFileFunc, + transferEncoding: []string{"chunked"}, + expectedWrite: true, + }, + { + name: "buffer, non-chunked, size set", + bodyFunc: newBufferFunc, + method: "PUT", + contentLength: nBytes, + limitedReader: true, + expectedReader: bufferType, + }, + { + name: "buffer, non-chunked, size set, nopCloser wrapped", + method: "PUT", + bodyFunc: func() (io.Reader, func(), error) { + r, cleanup, err := newBufferFunc() + return ioutil.NopCloser(r), cleanup, err + }, + contentLength: nBytes, + limitedReader: true, + expectedReader: bufferType, + }, + { + name: "buffer, non-chunked, negative size", + method: "PUT", + bodyFunc: newBufferFunc, + contentLength: -1, + expectedWrite: true, + }, + { + name: "buffer, non-chunked, CONNECT, negative size", + method: "CONNECT", + bodyFunc: newBufferFunc, + contentLength: -1, + expectedWrite: true, + }, + { + name: "buffer, chunked", + method: "PUT", + bodyFunc: newBufferFunc, + transferEncoding: []string{"chunked"}, + expectedWrite: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + body, cleanup, err := tc.bodyFunc() + if err != nil { + t.Fatal(err) + } + defer cleanup() + + mw := &mockTransferWriter{} + tw := &transferWriter{ + Body: body, + ContentLength: tc.contentLength, + TransferEncoding: tc.transferEncoding, + } + + if err := tw.writeBody(mw); err != nil { + t.Fatal(err) + } + + if tc.expectedReader != nil { + if mw.CalledReader == nil { + t.Fatal("did not call ReadFrom") + } + + var actualReader reflect.Type + lr, ok := mw.CalledReader.(*io.LimitedReader) + if ok && tc.limitedReader { + actualReader = reflect.TypeOf(lr.R) + } else { + actualReader = reflect.TypeOf(mw.CalledReader) + } + + if tc.expectedReader != actualReader { + t.Fatalf("got reader %T want %T", actualReader, tc.expectedReader) + } + } + + if tc.expectedWrite && !mw.WriteCalled { + t.Fatal("did not invoke Write") + } + }) + } +}