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

Jump to bottom

io: Copy() unnecessarily allocates memory when src is a file and is empty #53658

Closed
daulet opened this issue Jul 2, 2022 · 3 comments
Closed

io: Copy() unnecessarily allocates memory when src is a file and is empty #53658

daulet opened this issue Jul 2, 2022 · 3 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@daulet
Copy link
Contributor

daulet commented Jul 2, 2022

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

$ go version
go version go1.18 linux/amd64

Does this issue reproduce with the latest release?

Ye.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/dake/.cache/go-build"
GOENV="/home/dake/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/dake/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/dake/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/dake/dev/goroot"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/dake/dev/goroot/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/dake/dev/copybug/go.mod"
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 -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build544396302=/tmp/go-build -gno-record-gcc-switches"

What did you do?

This affects io.Copy() when src is a file, and dst is a *net.TCPConn.

Here is a minimal benchmark: https://go.dev/play/p/zgZwpjUatSq
Benchmark with go test -bench=. -benchmem

Summary is that a gorotoutine writes to a file, another goroutine reads from the file and writes it to a TCPConn with io.Copy(). io.Copy tries to be efficient and will try to use sendfile(2), however it doesn't always succeed and ends up allocating a buffer anyway. In my deubgging it turns out the current logic believes sendfile(2) failed if we written 0 bytes. Using benchmark above one can see that we allocate 5KB/op, where op is writing a single byte.

This also affects io.CopyBuffer(), when we fail to use sendfile(2) (in case of empty src) it falls back to io.Copy(writerOnly{w}, r) which ignores passed in buffer and allocates its own.

What did you expect to see?

CopyBuffer always uses passed in buffer, Copy does not allocate extra buffer when src is empty.

What did you see instead?

Running the benchmark above shows 5GB was allocated to transfer 0.5MB worth of data, due to allocating a buffer (see size := 32 * 1024 below) when it wasn't necessary (sendfile(2) tried to copy, but there was nothing to copy).

$ go test -bench=. -benchmem -run=^\$ -memprofile memprofile.out 
goos: linux
goarch: amd64
pkg: copybug
cpu: AMD Ryzen 9 5900HS with Radeon Graphics        
BenchmarkCopy-16          549627              2187 ns/op            5391 B/op          1 allocs/op
PASS
ok      copybug 1.510s
$ go tool pprof memprofile.out
File: copybug.test
Type: alloc_space
Time: Jul 1, 2022 at 5:01pm (PDT)
Entering interactive mode (type "help" for commands, "o" for options)
(pprof) top
Showing nodes accounting for 5042.76MB, 99.65% of 5060.27MB total
Dropped 25 nodes (cum <= 25.30MB)
      flat  flat%   sum%        cum   cum%
 5039.76MB 99.59% 99.59%  5056.26MB 99.92%  io.copyBuffer
       3MB 0.059% 99.65%  5042.76MB 99.65%  net.genericReadFrom
         0     0% 99.65%  5056.26MB 99.92%  copybug.BenchmarkCopy.func2
         0     0% 99.65%  5056.26MB 99.92%  io.Copy (inline)
         0     0% 99.65%  5056.26MB 99.92%  net.(*TCPConn).ReadFrom
         0     0% 99.65%  5056.26MB 99.92%  net.(*TCPConn).readFrom
(pprof) list copyBuffer
Total: 4.94GB
ROUTINE ======================== io.copyBuffer in /home/dake/dev/goroot/src/io/io.go
    4.92GB     9.86GB (flat, cum) 199.52% of Total
         .          .    407:   if wt, ok := src.(WriterTo); ok {
         .          .    408:           return wt.WriteTo(dst)
         .          .    409:   }
         .          .    410:   // Similarly, if the writer has a ReadFrom method, use it to do the copy.
         .          .    411:   if rt, ok := dst.(ReaderFrom); ok {
         .     4.94GB    412:           return rt.ReadFrom(src)
         .          .    413:   }
         .          .    414:   if buf == nil {
         .          .    415:           size := 32 * 1024
         .          .    416:           if l, ok := src.(*LimitedReader); ok && int64(size) > l.N {
         .          .    417:                   if l.N < 1 {
         .          .    418:                           size = 1
         .          .    419:                   } else {
         .          .    420:                           size = int(l.N)
         .          .    421:                   }
         .          .    422:           }
    4.92GB     4.92GB    423:           buf = make([]byte, size)
         .          .    424:   }
         .          .    425:   for {
         .          .    426:           nr, er := src.Read(buf)
         .          .    427:           if nr > 0 {
         .          .    428:                   nw, ew := dst.Write(buf[0:nr])
(pprof)
@daulet daulet mentioned this issue Jul 2, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/415834 mentions this issue: io: correctly process result of sendfile(2) when src returns 0 bytes

@heschi heschi added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 6, 2022
@heschi heschi added this to the Backlog milestone Jul 6, 2022
@heschi
Copy link
Contributor

heschi commented Jul 6, 2022

cc @griesemer

@gopherbot
Copy link

Change https://go.dev/cl/536455 mentions this issue: net,internal/poll: mark it as handled even if sendfile(2) succeeded with 0 bytes sent

gopherbot pushed a commit that referenced this issue Oct 23, 2023
@panjf2000 @gopherbot
net,internal/poll: mark it as handled even if sendfile(2) succeeded w…
c65f74d 
…ith 0 bytes sent

CL 415834 fixed #53658 and somehow it only fixed it on Linux,
sendfile can also succeed with 0 bytes sent on other platforms
according to their manuals, this CL will finish the work that
CL 415834 left out on other platforms.

goos: darwin
goarch: arm64
pkg: net
                     │     old     │                new                 │
                     │   sec/op    │   sec/op     vs base               │
SendfileZeroBytes-10   7.563µ ± 5%   7.184µ ± 6%  -5.01% (p=0.009 n=10)

                     │     old     │                new                 │
                     │    B/op     │    B/op     vs base                │
SendfileZeroBytes-10   3562.5 ± 7%   590.0 ± 2%  -83.44% (p=0.000 n=10)

                     │    old    │             new              │
                     │ allocs/op │ allocs/op   vs base          │
SendfileZeroBytes-10   0.00 ± 0%   11.00 ± 0%  ? (p=0.000 n=10)

[1] https://man.freebsd.org/cgi/man.cgi?sendfile(2)
[2] https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/sendfile.2.html
[3] https://man.dragonflybsd.org/?command=sendfile&section=2
[4] https://docs.oracle.com/cd/E88353_01/html/E37843/sendfile-3c.html

Change-Id: I55832487595ee8e0f44f367cf2a3a1d827ba590d
Reviewed-on: https://go-review.googlesource.com/c/go/+/536455
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

io: correctly process result of sendfile(2) when src returns 0 bytes daulet/go
3 participants
@daulet @gopherbot @heschi