]> Cypherpunks repositories - gostls13.git/commitdiff
net/http: let Transport request body writes use sendfile
authorChris Marchesi <chrism@vancluevertech.com>
Thu, 7 Mar 2019 20:01:21 +0000 (20:01 +0000)
committerEmmanuel Odeke <emm.odeke@gmail.com>
Thu, 7 Mar 2019 20:11:21 +0000 (20:11 +0000)
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: 4a77dd1b80140274bf3ed20ad7465ff3cc06febf
GitHub-Pull-Request: golang/go#30378
Reviewed-on: https://go-review.googlesource.com/c/go/+/163599
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
src/net/http/transfer.go
src/net/http/transfer_test.go

index e8a93e9137eb8a97d3840d018296462a3dee6d54..7d73dc4fc043419c9ddb1849df813178a82c4505 100644 (file)
@@ -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
index 993ea4ef18c449a5332736dc09ccec82e93948a8..aa465d060086cd0fe888a4338280f7066f2b7c04 100644 (file)
@@ -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")
+                       }
+               })
+       }
+}