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: consider adding configurable buffer sizes to Transport #22618

Open
nirs opened this Issue Nov 7, 2017 · 8 comments

Comments

Projects
None yet
7 participants
@nirs

nirs commented Nov 7, 2017

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

go version devel +6e3a2b3 Tue Nov 7 15:04:53 2017 +0200 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/nsoffer/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/nsoffer/go"
GORACE=""
GOROOT="/home/nsoffer/src/go"
GOTMPDIR=""
GOTOOLDIR="/home/nsoffer/src/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="gcc++"
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"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build316051983=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Test uploading files over https, note low throughput compared with similar Python program.
For example programs and benchmarks, see
https://github.com/nirs/http-bench/tree/go-bufsize

What did you expect to see?

Being able to control buffer size used by http.Client, similar to the way you can control the buffer size in io.CopyBuffer.

What did you see instead?

Hardcoded value hidden deep in the code and my free time being wasted locating it :).

For example, here is output from strace:

[pid 32264] read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4096
[pid 32264] write(4, "\27\3\3\20\30\0\0\0\0\0\3l\"w\201\360W\307F=\215Zj&\6hj\253\343\20EN"..., 4125) = 4125

Use case

An example use case is uploading vm images to storage, for example, ovirt-imageio.

Initial proposal

Add Transport.WriteBufSize and Transport.ReadBufSize:
https://go-review.googlesource.com/#/c/go/+/76410/

Example usage:

t := &http.Transport{WriteBufSize: 128*1024}
client := &http.Client{Transport: t}

I tested the maximum benefit of this change by uploading data from
/dev/zero to a server discarding the data. Here is an example upload
using the default buffer size:

$ time ./upload 10 https://localhost:8000/
Uploaded 10.00g in 25.13 seconds (407.49m/s)

real	0m25.135s
user	0m5.167s
sys	0m11.643s

With this change, using 128k buffer size:

$ time ./upload 10 https://localhost:8000/
Uploaded 10.00g in 7.93 seconds (1291.51m/s)

real	0m7.935s
user	0m4.517s
sys	0m2.603s

Tested on Lenovo T450s
Intel(R) Core(TM) i7-5600U CPU @ 2.60GHz

In real world usage the difference will be smaller, depending on the
local and remote storage and the network.

Similar enhancement was added lately to Python, see:
python/cpython@ad455cd

@bradfitz bradfitz changed the title from Provide configurable buffer size in http.Client to net/http: consider adding configurable buffer sizes to Transport Nov 7, 2017

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Nov 7, 2017

@gopherbot

This comment has been minimized.

gopherbot commented Nov 7, 2017

Change https://golang.org/cl/76410 mentions this issue: net/http: configurable transport buffer size

@nirs

This comment has been minimized.

nirs commented May 22, 2018

Any news about this?

@bradfitz bradfitz modified the milestones: Go1.11, Unplanned May 23, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 23, 2018

Sorry, I just found this back.

I left some comments on the change. It's a little late in Go 1.11, but you did mail it well before the deadline. Adding new API always makes me uncomfortable, though, especially performance knobs which I wish could be more automatic.

/cc @bcmills @tombergan

@bradfitz bradfitz modified the milestones: Unplanned, Go1.12 May 23, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 23, 2018

If not Go 1.11, I'll at least mark this for Go 1.2.

@CAFxX

This comment has been minimized.

Contributor

CAFxX commented Sep 30, 2018

especially performance knobs which I wish could be more automatic.

FWIW, I would also prefer the default to be automatic tuning. Pardon the handwaving but maybe something like:

  • if the size of the current write/send is known, and it is significantly (as in "orders of magnitude") bigger than the current sndbuf, raise the sndbuf (up to a safe limit)
  • if the size of the current user-provided read/receive buffer is known, and it is bigger than the current rcvbuf, raise the rcvbuf (up to a safe limit)

Ideally such heuristics should be then refined with information about the actual throughput of the connection (there's no point increasing the buffer if the actual throughput is very low).

@JohnRusk

This comment has been minimized.

JohnRusk commented Oct 4, 2018

This is a great idea. Is it confirmed for inclusion in 1.12?

One minor point: IIRC, when using HTTPS there's effectively a 16k buffer that kicks in as part of the TLS processing: maxPlaintext = 16384 in crypto/tls/common.go. I believe means that setting Transport's proposed WriteBufSize may have reduced effectiveness for sizes over 16k. I.e. increasing from 4k to 16 will give the full benefit of increasing the buffer size... but increasing above 16k may have less benefit, since things are getting chopped into 16k chunks by the TLS code anyway. Is the 16k limit unavoidable in TLS , or should it somehow also be made to increase when Transport is using larger buffers?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Oct 4, 2018

This is a great idea.

What is? There's nothing concrete proposed here yet.

Is it confirmed for inclusion in 1.12?

Nothing is confirmed. But I wanted to understand the problem and options more at least for 1.12. Not sure if there will be time to do something about it.

@JohnRusk

This comment has been minimized.

JohnRusk commented Oct 4, 2018

What is? There's nothing concrete proposed here yet.

Back at the start of this thread, isn't there a concrete proposal here: https://go-review.googlesource.com/#/c/go/+/76410/

Having said that, a simpler option could be to just change the default size up to 16K from the current 4k. Based on my own testing, I would expect even that to make a significant difference in CPU usage and/or throughput for high-throughput cases (i.e. in the multi-gigabit range).

@rsc rsc modified the milestones: Go1.12, Go1.13 Nov 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment