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.

Sign up for GitHub

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

Jump to bottom

net/http: reusing http.Request is slower than allocating a new one #28310

Closed
thesyncim opened this issue Oct 22, 2018 · 2 comments
Closed

net/http: reusing http.Request is slower than allocating a new one #28310

thesyncim opened this issue Oct 22, 2018 · 2 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@thesyncim
Copy link

thesyncim commented Oct 22, 2018 •
edited

Please answer these questions before submitting your issue

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.
https://play.golang.org/p/okT3ECKTCiu

What did you expect to see?

the same or better performance in BenchmarkReuseRequest

What did you see instead?

lower performance and higher allocations
goos: darwin
goarch: amd64
BenchmarkDoNotReuseRequest-8 20000 79941 ns/op 4660 B/op 70 allocs/op
BenchmarkReuseRequest-8 5000 349885 ns/op 18562 B/op 157 allocs/op
PASS

System details

go version go1.11.1 darwin/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/stream/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/stream/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11.1/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11.1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
GOROOT/bin/go version: go version go1.11.1 darwin/amd64
GOROOT/bin/go tool compile -V: compile version go1.11.1
uname -v: Darwin Kernel Version 17.7.0: Thu Jun 21 22:53:14 PDT 2018; root:xnu-4570.71.2~1/RELEASE_X86_64
ProductName:	Mac OS X
ProductVersion:	10.13.6
BuildVersion:	17G65
lldb --version: lldb-1000.0.37
  Swift-4.2
@thesyncim thesyncim changed the title net/http reusing http.Request is slower than allocating a new one net/http: reusing http.Request is slower than allocating a new one Oct 22, 2018
@agnivade
Copy link
Contributor

/cc @bradfitz

@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 22, 2018
@agnivade agnivade added this to the Unplanned milestone Oct 22, 2018
@odeke-em
Copy link
Member

odeke-em commented Oct 14, 2019 •
edited

Thank you for filing this issue @thesyncim!

Your current benchmarks unfortunately don't do an apples-to-apples comparison because in the case of BenchmarkReuseRequest you create the body once and reuse that. For every other request except the first one, the body will be non-nil but at EOF which takes a whole different route during RoundTrip-ping than if it is were created afresh. This diff should bring them to parity

78c78
< 	req, err := http.NewRequest(http.MethodPost, addr, strings.NewReader("hello"))
---
> 	req, err := http.NewRequest(http.MethodPost, addr, nil)
83a84
>                 req.Body = ioutil.NopCloser(strings.NewReader("hello"))

making your BenchmarkReuseRequest loop now

for i := 0; i < b.N; i++ {
                req.URL = u
                req.Body = ioutil.NopCloser(strings.NewReader("hello"))
                resp, err := client.Do(req)

and when we examine the comparisons, you'll see them almost exactly the same (except of course for the extra 3 allocations per loop in BenchmarkReuseRequest

$ go test -run=^$ -bench=. -count=7
goos: darwin
goarch: amd64
pkg: github.com/odeke-em/bugs/golang/28310
BenchmarkDoNotReuseRequest-8   	   20721	     57681 ns/op	    4694 B/op	      65 allocs/op
BenchmarkDoNotReuseRequest-8   	   20332	     60927 ns/op	    4695 B/op	      65 allocs/op
BenchmarkDoNotReuseRequest-8   	   19497	     59840 ns/op	    4695 B/op	      65 allocs/op
BenchmarkDoNotReuseRequest-8   	   19916	     59264 ns/op	    4692 B/op	      65 allocs/op
BenchmarkDoNotReuseRequest-8   	   20248	     61995 ns/op	    4695 B/op	      65 allocs/op
BenchmarkDoNotReuseRequest-8   	   19990	     63561 ns/op	    4696 B/op	      65 allocs/op
BenchmarkDoNotReuseRequest-8   	   19410	     62359 ns/op	    4696 B/op	      65 allocs/op
BenchmarkReuseRequest-8   	   19927	     61764 ns/op	    4316 B/op	      68 allocs/op
BenchmarkReuseRequest-8   	   19706	     66927 ns/op	    4319 B/op	      68 allocs/op
BenchmarkReuseRequest-8   	   18552	     62377 ns/op	    4326 B/op	      68 allocs/op
BenchmarkReuseRequest-8   	   20034	     60995 ns/op	    4319 B/op	      68 allocs/op
BenchmarkReuseRequest-8   	   18922	     62632 ns/op	    4316 B/op	      68 allocs/op
BenchmarkReuseRequest-8   	   18802	     61532 ns/op	    4315 B/op	      68 allocs/op
BenchmarkReuseRequest-8   	   18019	     70261 ns/op	    4325 B/op	      68 allocs/op

and then with benchstat

$ benchstat before.txt after.txt
name            old time/op    new time/op    delta
ReuseRequest-8    60.8µs ± 5%    63.8µs ±10%    ~     (p=0.097 n=7+7)

name            old alloc/op   new alloc/op   delta
ReuseRequest-8    4.69kB ± 0%    4.32kB ± 0%  -7.99%  (p=0.001 n=7+7)

name            old allocs/op  new allocs/op  delta
ReuseRequest-8      65.0 ± 0%      68.0 ± 0%  +4.62%  (p=0.001 n=7+7)

which is the right parity and for a facet is even better with an ~8% reduction in allocations per ops.

However, reusing requests is to be taken with caution, please refer to docs on how to do so properly.

I'll close this issue as I've shown with the proper comparison. Please feel free however, to open if something else is up.

@golang golang locked and limited conversation to collaborators Oct 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@thesyncim @agnivade @odeke-em @gopherbot