Skip to content
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

net/http: performance degradation when sending HTTP request with bytes.Reader #44759

Open
nan01ab opened this issue Mar 3, 2021 · 4 comments
Open

Comments

@nan01ab
Copy link

@nan01ab nan01ab commented Mar 3, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
go env
...
GOARCH="amd64"
GOOS="linux"
...

What did you do?

Sending a HTTP PUT request with bytes.Reader

        url := "http://IP:Port/xxxx/yyy"
	data := make([]byte, 1<<20)
	// write something into data
	r, err := http.NewRequest(http.MethodPut, url, bytes.NewReader(data))
	if err != nil {
		return err
	}
        res, err := http.DefaultClient.Do(r)
	if err != nil {
		return err
	}
       // do something more

There is a significant performance degradation due to some problems with the net/http.

Simple analysis:
The http.DefaultClient.Do call will eventually go to io.Copy.

func Copy(dst Writer, src Reader) (written int64, err error) {
	return copyBuffer(dst, src, nil)
}

In this case, dst is a *bufio.Writer, and src is an ioutil.nopCloser. And, in the copyBuffer, bufio.Writer.ReadFrom will be called. In bufio.Writer.ReadFrom, becasue some headers have been written to b.buf and not been Flush, the for loop will be executed. In this loop, it will only copy 4KiB to b.buf each time, resulting in useless memory copies and a large number of system calls, which in turn leads to performance degradation.

func (b *Writer) ReadFrom(r io.Reader) (n int64, err error) {
	if b.err != nil {
		return 0, b.err
	}
	if b.Buffered() == 0 { //  b.Buffered() > 0, becasue some headers have been written here and not been `Flush`
		if w, ok := b.wr.(io.ReaderFrom); ok {
			n, err = w.ReadFrom(r)
			b.err = err
			return n, err
		}
	}
	var m int
	for {
		if b.Available() == 0 {
			if err1 := b.Flush(); err1 != nil {
				return n, err1
			}
		}
		nr := 0
		for nr < maxConsecutiveEmptyReads {
			m, err = r.Read(b.buf[b.n:])
			if m != 0 || err != nil {
				break
			}
			nr++
		}
		// ...
	}
      // ...
}

Suggested fix

use

// src/net/http/transfer.go
func (t *transferWriter) unwrapBody() io.Reader {
	if reflect.TypeOf(t.Body) == nopCloserType {
		return reflect.ValueOf(t.Body).Field(0).Interface().(io.Reader)
	}
	if r, ok := t.Body.(*readTrackingBody); ok {
		r.didRead = true
		if reflect.TypeOf(r.ReadCloser) == nopCloserType {
			return reflect.ValueOf(r.ReadCloser).Field(0).Interface().(io.Reader)
		}
		return r.ReadCloser
	}
	return t.Body
}

instead of

func (t *transferWriter) unwrapBody() io.Reader {
	if reflect.TypeOf(t.Body) == nopCloserType {
		return reflect.ValueOf(t.Body).Field(0).Interface().(io.Reader)
	}
	if r, ok := t.Body.(*readTrackingBody); ok {
		r.didRead = true
		return r.ReadCloser
	}
	return t.Body
}

.

@nan01ab nan01ab changed the title net/http: The performance issue of sending http request with bytes.Reader net/http: A performance issue of sending http request with bytes.Reader Mar 4, 2021
@neild
Copy link
Contributor

@neild neild commented Mar 5, 2021

I don't believe the suggested fix is sufficient. The problem isn't that the io.Reader is wrapping the wrong type, it's that the bufio.Writer has had unflushed headers written to it by the time the io.Copy happens.

I'm not certain what the correct behavior is here.

I wonder if bufio.Writer should fall back to ReadFrom (when available) after filling and flushing a partially-full buffer.

@networkimprov
Copy link

@networkimprov networkimprov commented Mar 5, 2021

@neild
Copy link
Contributor

@neild neild commented Mar 5, 2021

Filed #44815 to propose the bufio.Writer change.

@dmitshur dmitshur added this to the Backlog milestone Mar 5, 2021
@dmitshur dmitshur changed the title net/http: A performance issue of sending http request with bytes.Reader net/http: performance degradation when sending HTTP request with bytes.Reader Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants