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

net: performance issue with TCPConn.SetReadBuffer (~25ms network delay) #25701

Closed
damnever opened this issue Jun 2, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@damnever
Copy link

commented Jun 2, 2018

I'm using go1.9+ on CentOS Linux release 7.3.1611 (Core) 3.10.0-514.el7.x86_64 #1 SMP Tue Nov 22 16:42:41 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux, built it with env GOOS=linux GOARCH=amd64.

In our production environment(multi-datacenter, delay ~25ms), compare to Python, using TCPConn.SetReadBuffer is much slower, here shows the result(with some scripts and configs, note the docker environment can not reproduce it). Also, changing the system global config net.ipv4.tcp_rmem works fine, it may be different from program config, but I think it's not in this case. So I think there may be something wrong with Golang.

Let me know If you require any further information.

@ianlancetaylor ianlancetaylor changed the title Performance issue with TCPConn.SetReadBuffer (~25ms network delay) net: performance issue with TCPConn.SetReadBuffer (~25ms network delay) Jun 2, 2018

@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Jun 2, 2018

@dspezia

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2018

On Linux (and probably some other OS), you can only set the size of the buffers before the socket is connected/accepted. In the Python code, the buffer size is correctly set between the creation of the socket and the connect operation. In the Go code, Dial creates the socket and establishes the connection, so the SetReadBuffer call has no effect.

IMO, this is one flaw of the net API in Go: the API does not allow to properly configure some socket options before the socket is actually used.

A workaround is to directly execute the corresponding syscalls, but this is cumbersome.
More explanations were given by Mikio Hara in this thread.

@dspezia

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2018

Note that CL 72810 will probably offer a solution to this problem.

@damnever

This comment has been minimized.

Copy link
Author

commented Jun 2, 2018

Thanks, setsockopt before connect solve the problem. The Control(Dialer.Control & ListenConfig.Control) callback seems nice, but I kind of dislike the nested callback thing.. also we can't actually read/write something from/to syscall.RawConn before dialing/binding, right?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

Issue #9661 has been fixed for 1.11, so there will be a way to do this going forward.

It is correct that reading or writing to a syscall.RawConn before dialing or binding will not work. The calls will simply return an error.

We could see a good wayt to implement this correctly and safely without a nested callback. The problem is that the file must be locked during the operation, but permitting user code to lock files itself seems like a recipe for bugs.

Since it seems that people have identified the problem, and there doesn't seem to be anything to change in Go, I'm going to close this issue. Please comment if you disagree.

@damnever

This comment has been minimized.

Copy link
Author

commented Jun 5, 2018

I think this would be nice:

type Dialer struct {
    Control func(network, address string, sfd int) error
}

func (d *Dialer) Dial(network, address string) (Conn, error) {
    ...
    if d.Control != nil {
        rawConn.Control(func(s uintptr) { d.Control(network, address, int(s)) })
    }
    ...
}
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

@damnever Windows does not use int for the file descriptor. In general the platform independent part of the os package avoids using any specific type for the system's representation of a file.

@damnever

This comment has been minimized.

Copy link
Author

commented Jun 6, 2018

@ianlancetaylor I am not familiar with Windows. The API style is what I like better.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2018

@damnever OK, understood, but we can't use that approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.