Skip to content

net/http: transferWriter.doBodyCopy() calls io.Copy() resulting in many buffer allocations #57202

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
Closed
mmatczuk opened this issue Dec 9, 2022 · 4 comments
Closed

net/http: transferWriter.doBodyCopy() calls io.Copy() resulting in many buffer allocations #57202

mmatczuk opened this issue Dec 9, 2022 · 4 comments
Assignees
@neild
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Backlog

Comments

@mmatczuk
Copy link

mmatczuk commented Dec 9, 2022 •
edited
Loading

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

$ go version go1.19.4 darwin/arm64

Does this issue reproduce with the latest release?

Yes

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

go env Output
GO111MODULE="on"
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/michal/Library/Caches/go-build"
GOENV="/Users/michal/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/michal/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/michal/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/michal/sdk/go1.19.4"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/michal/sdk/go1.19.4/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.19.4"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/7q/nyynjpwj5p19npby48ykjpx00000gn/T/go-build3980036370=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I have HTTP proxy server proxying plain HTTP 1.1 requests based on google/martian. When I profile the memory allocations I see that transferWriter.doBodyCopy() calls io.Copy() resulting in many buffer allocations. The profile graph is attached below.

Screenshot 2022-12-09 at 13 40 11

I think doBodyCopy() could use copyBufPool we have in response.ReadFrom() to avoid the allocations.
@mmatczuk mmatczuk changed the title net/http: transferWriter.doBodyCopy calls io.Copy reslting net/http: transferWriter.doBodyCopy() calls io.Copy() resulting in many buffer allocations Dec 9, 2022
@seankhliao seankhliao added Performance NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 9, 2022
mmatczuk added a commit to mmatczuk/go that referenced this issue Dec 9, 2022
@mmatczuk
net/http: use copyBufPool in transferWriter.doBodyCopy()
ba09cda 
This patch is a followup to 6fd82d8.
It applies copyBufPool optimization to transferWriter.doBodyCopy().
The function is very frequently used - everytime Request or Response is written to a wire.
Without this patch for every Request and Response processed, if there is a body, we need to allocate and GC a 32k buffer.
This is quickly causing GC pressure.

Fixes golang#57202
@mmatczuk mmatczuk mentioned this issue Dec 9, 2022
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/456435 mentions this issue: net/http: use copyBufPool in transferWriter.doBodyCopy()

@neild
Copy link
Contributor

neild commented Dec 9, 2022

I wonder if io.Copy should use a pool to allocate buffers.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/456555 mentions this issue: io: allocate copy buffers from a pool

@neild neild self-assigned this Dec 9, 2022
@seankhliao seankhliao added this to the Backlog milestone Jan 20, 2023
gopherbot pushed a commit that referenced this issue Jan 31, 2023
@neild @gopherbot
io: allocate copy buffers from a pool
7d7fd6d 
CopyBuffer allocates a 32k buffer when no buffer is available.
Allocate these buffers from a sync.Pool.

This removes an optimization where the copy buffer size was
reduced when the source is a io.LimitedReader (including the
case of CopyN) with a limit less than the default buffer size.
This change could cause a program which only uses io.Copy
with sources with a small limit to allocate unnecessarily
large buffers. Programs which care about the transient
buffer allocation can avoid this by providing their own buffer.

name           old time/op    new time/op    delta
CopyNSmall-10     165ns ± 7%     117ns ± 7%  -29.19%  (p=0.001 n=7+7)
CopyNLarge-10    7.33µs ±34%    4.07µs ± 2%  -44.52%  (p=0.001 n=7+7)

name           old alloc/op   new alloc/op   delta
CopyNSmall-10    2.20kB ±12%    1.20kB ± 4%  -45.24%  (p=0.000 n=8+7)
CopyNLarge-10     148kB ± 9%      81kB ± 4%  -45.26%  (p=0.000 n=8+7)

name           old allocs/op  new allocs/op  delta
CopyNSmall-10      2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.000 n=8+8)
CopyNLarge-10      2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.000 n=8+8)

For #57202

Change-Id: I2292226da9ba1dc09a2543f5d74fe5da06080d49
Reviewed-on: https://go-review.googlesource.com/c/go/+/456555
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Damien Neil <[email protected]>
Reviewed-by: Thomas Austad <[email protected]>
Auto-Submit: Damien Neil <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/467095 mentions this issue: Revert "io: allocate copy buffers from a pool"

gopherbot pushed a commit that referenced this issue Feb 10, 2023
@neild
Revert "io: allocate copy buffers from a pool"
b02d5d3 
This reverts CL 456555.

Reason for revert: This seems too likely to exercise race conditions
in code where a Write call continues to access its buffer after
returning. The HTTP/2 ResponseWriter is one such example.

Reverting this change while we think about this some more.

For #57202

Change-Id: Ic86823f81d7da410ea6b3f17fb5b3f9a979e3340
Reviewed-on: https://go-review.googlesource.com/c/go/+/467095
Reviewed-by: Bryan Mills <[email protected]>
Run-TryBot: Damien Neil <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 12, 2023
@neild @johanbrandhorst
io: allocate copy buffers from a pool
74bbad6 
CopyBuffer allocates a 32k buffer when no buffer is available.
Allocate these buffers from a sync.Pool.

This removes an optimization where the copy buffer size was
reduced when the source is a io.LimitedReader (including the
case of CopyN) with a limit less than the default buffer size.
This change could cause a program which only uses io.Copy
with sources with a small limit to allocate unnecessarily
large buffers. Programs which care about the transient
buffer allocation can avoid this by providing their own buffer.

name           old time/op    new time/op    delta
CopyNSmall-10     165ns ± 7%     117ns ± 7%  -29.19%  (p=0.001 n=7+7)
CopyNLarge-10    7.33µs ±34%    4.07µs ± 2%  -44.52%  (p=0.001 n=7+7)

name           old alloc/op   new alloc/op   delta
CopyNSmall-10    2.20kB ±12%    1.20kB ± 4%  -45.24%  (p=0.000 n=8+7)
CopyNLarge-10     148kB ± 9%      81kB ± 4%  -45.26%  (p=0.000 n=8+7)

name           old allocs/op  new allocs/op  delta
CopyNSmall-10      2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.000 n=8+8)
CopyNLarge-10      2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.000 n=8+8)

For golang#57202

Change-Id: I2292226da9ba1dc09a2543f5d74fe5da06080d49
Reviewed-on: https://go-review.googlesource.com/c/go/+/456555
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Damien Neil <[email protected]>
Reviewed-by: Thomas Austad <[email protected]>
Auto-Submit: Damien Neil <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 12, 2023
@neild @johanbrandhorst
Revert "io: allocate copy buffers from a pool"
49f488d 
This reverts CL 456555.

Reason for revert: This seems too likely to exercise race conditions
in code where a Write call continues to access its buffer after
returning. The HTTP/2 ResponseWriter is one such example.

Reverting this change while we think about this some more.

For golang#57202

Change-Id: Ic86823f81d7da410ea6b3f17fb5b3f9a979e3340
Reviewed-on: https://go-review.googlesource.com/c/go/+/467095
Reviewed-by: Bryan Mills <[email protected]>
Run-TryBot: Damien Neil <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
mmatczuk added a commit to mmatczuk/go that referenced this issue Nov 2, 2023
@mmatczuk
net/http: use copyBufPool in transferWriter.doBodyCopy()
d02a78b 
This patch is a followup to 6fd82d8.
It applies copyBufPool optimization to transferWriter.doBodyCopy().
The function is very frequently used - every time Request or Response is written.

Without this patch for every Request and Response processed, if there is a body, we need to allocate and GC a 32k buffer.
This is quickly causing GC pressure.

Fixes golang#57202
@mmatczuk mmatczuk mentioned this issue Nov 3, 2023
Closed
mmatczuk added a commit to mmatczuk/go that referenced this issue Nov 8, 2023
@mmatczuk
net/http: use copyBufPool in transferWriter.doBodyCopy()
c167574 
This is a followup to CL 14177. It applies copyBufPool optimization to
transferWriter.doBodyCopy(). The function is used every time Request or
Response is written.

Without this patch for every Request and Response processed, if there is
a body, we need to allocate and GC a 32k buffer. This is quickly causing
GC pressure.

Fixes golang#57202
mmatczuk added a commit to mmatczuk/go that referenced this issue Nov 9, 2023
@mmatczuk
net/http: use copyBufPool in transferWriter.doBodyCopy()
908573c 
This is a followup to CL 14177. It applies copyBufPool optimization to
transferWriter.doBodyCopy(). The function is used every time Request or
Response is written.

Without this patch for every Request and Response processed, if there is
a body, we need to allocate and GC a 32k buffer. This is quickly causing
GC pressure.

Fixes golang#57202
@golang golang locked and limited conversation to collaborators Nov 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@neild neild

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
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

net/http: use copyBufPool in transferWriter.doBodyCopy() mmatczuk/go
4 participants
@neild @mmatczuk @gopherbot @seankhliao