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

bufio: Reader should fill the read destination slice if possible #22482

Closed
paulstuart opened this issue Oct 29, 2017 · 3 comments
Closed

bufio: Reader should fill the read destination slice if possible #22482

paulstuart opened this issue Oct 29, 2017 · 3 comments

Comments

@paulstuart
Copy link

Please answer these questions before submitting your issue. Thanks!

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

go version go1.9.2 darwin/amd64

Does this issue reproduce with the latest release?

yes

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/pstuart/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/qt/_9gpg7kx2zn0180w_tftkqgh0000gn/T/go-build893477021=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

Used a bufio reader to read small chunks from a large io stream

Example here: https://play.golang.org/p/ufs826X0l7

What did you expect to see?

I expected the read to fill my buffer completely

What did you see instead?

I only saw what was left in the buffer

This is fixed by adding the following at line 222 of bufio.go:

    // refill buffer if needed
    if len(p) > b.w-b.r {
            // Slide existing data to beginning.
            if b.r > 0 {
                    copy(b.buf, b.buf[b.r:b.w])
                    b.w -= b.r
                    b.r = 0
            }

            n, err := b.rd.Read(b.buf[b.w:])
            if n < 0 {
                    panic(errNegativeRead)
            }
            b.w += n
            if err != nil {
                    b.err = err
            }
    }
@as
Copy link
Contributor

as commented Oct 29, 2017

Just to clarify, The example reads (1) 4096 + (2) 2048+ (3) 4096 bytes from a buffer of size 8192. The last read is unaligned because your slice is 2048 bytes larger than what's left in the 8192 byte bucket. You propose that these last 2048 bytes are collected with an extra read call to the underlying reader.

If this is correct, it seems like a bad trade off to gain the convenience of a full slice at the expense of latency. The underlying reader could be expensive to read from.

If you want to fill the slice, why not use something like io.ReadAtLeast ?

Have you run benchmarks on your change to see how it affects performance in a non-trivial example?

@as
Copy link
Contributor

as commented Oct 29, 2017

In addition, consider the example when the length of p is 8192+1, should it really try to read that one last byte from the underlying data source just to return a full slice?

@gbbr gbbr changed the title bufio.Reader should fill the read destination slice if possible bufio: Reader should fill the read destination slice if possible Oct 30, 2017
@paulstuart
Copy link
Author

d'oh! ReadAtLeast addresses the issue and I completely overlooked that. Sorry for the trouble.

@golang golang locked and limited conversation to collaborators Oct 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants