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

Closed
nan01ab opened this issue Mar 3, 2021 · 4 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@nan01ab
Copy link

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
@networkimprov
Copy link

cc @fraenkel @neild @bradfitz

@neild
Copy link
Contributor

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

cc @tombergan @empijei

@neild
Copy link
Contributor

neild commented Mar 5, 2021

Filed #44815 to propose the bufio.Writer change.

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance labels Mar 5, 2021
@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
@nan01ab nan01ab closed this as completed Jul 16, 2022
@golang golang locked and limited conversation to collaborators Jul 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests

5 participants