Code review - Issue 7188044: code review 7188044: net: Use accept4() on Linux.https://codereview.appspot.com/2013-01-30T23:32:23+00:00rietveld
Message from unknown
2013-01-22T09:48:18+00:00songofacandyurn:md5:7604ddbb9a57fdd56b121d93343ac1ea
Message from unknown
2013-01-22T09:48:28+00:00songofacandyurn:md5:bad66669bfab6b5b94644db3d8f5a336
Message from unknown
2013-01-22T09:59:14+00:00songofacandyurn:md5:782b2c7468eca9f1278534a095a7d226
Message from songofacandy@gmail.com
2013-01-22T09:59:22+00:00songofacandyurn:md5:a1fc35d098a3b219cd7592705a2c193c
Hello golang-dev@googlegroups.com,
I'd like you to review this change to
https://code.google.com/p/go
Message from dave@cheney.net
2013-01-22T10:16:01+00:00dfcurn:md5:c9d98d02f2ed036590b88d7c4d890758
On 2013/01/22 09:59:22, songofacandy wrote:
> Hello mailto:golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
Hello,
Thank you for this. Reading the man page for accept, there is no indication that accept4 is non blocking, and the majority of the improvement appears from reducing the contention on the fork lock.
Is it possible to redefine syscall.Accept in terms of accept4 and avoid adding a new public symbol ?
Message from songofacandy@gmail.com
2013-01-22T10:38:23+00:00songofacandyurn:md5:5ee57109f78066ae92bd4a5469d2a494
How can I emulate accept4 on bsd?
Changing the signature of Accept() like following is OK?
func Accept(fd, flags int, blocking bool) (nfd int, sa Sockaddr, err error)
{
if !blocking && flags&SOCK_CLOEXEC {
ForkLock.RLock()
}
....
nfd = accept(fd, &rsa, &len)
....
if flags&SOCK_CLOEXEC {
CloseOnExec(nfd)
if !blocking {
ForkLock.RUnlock()
}
}
....
}
On Tue, Jan 22, 2013 at 7:16 PM, <dave@cheney.net> wrote:
> On 2013/01/22 09:59:22, songofacandy wrote:
>
>> Hello mailto:golang-dev@**googlegroups.com <golang-dev@googlegroups.com>,
>>
>
> I'd like you to review this change to
>> https://code.google.com/p/go
>>
>
> Hello,
>
> Thank you for this. Reading the man page for accept, there is no
> indication that accept4 is non blocking, and the majority of the
> improvement appears from reducing the contention on the fork lock.
>
> Is it possible to redefine syscall.Accept in terms of accept4 and avoid
> adding a new public symbol ?
>
> https://codereview.appspot.**com/7188044/<https://codereview.appspot.com/7188044/>
>
--
INADA Naoki <songofacandy@gmail.com>
Message from dave@cheney.net
2013-01-22T10:44:08+00:00dfcurn:md5:13801479d88829097c828e655a113eec
We cannot change the signature of a public symbol already published in Go
1.0. It would break the api check in the build stage, and more importantly,
break the Go 1.0 contract.
On 22 Jan 2013 21:38, "INADA Naoki" <songofacandy@gmail.com> wrote:
> How can I emulate accept4 on bsd?
> Changing the signature of Accept() like following is OK?
>
> func Accept(fd, flags int, blocking bool) (nfd int, sa Sockaddr, err
> error) {
> if !blocking && flags&SOCK_CLOEXEC {
> ForkLock.RLock()
> }
> ....
> nfd = accept(fd, &rsa, &len)
> ....
> if flags&SOCK_CLOEXEC {
> CloseOnExec(nfd)
> if !blocking {
> ForkLock.RUnlock()
> }
> }
> ....
> }
>
>
>
> On Tue, Jan 22, 2013 at 7:16 PM, <dave@cheney.net> wrote:
>
>> On 2013/01/22 09:59:22, songofacandy wrote:
>>
>>> Hello mailto:golang-dev@**googlegroups.com <golang-dev@googlegroups.com>
>>> ,
>>>
>>
>> I'd like you to review this change to
>>> https://code.google.com/p/go
>>>
>>
>> Hello,
>>
>> Thank you for this. Reading the man page for accept, there is no
>> indication that accept4 is non blocking, and the majority of the
>> improvement appears from reducing the contention on the fork lock.
>>
>> Is it possible to redefine syscall.Accept in terms of accept4 and avoid
>> adding a new public symbol ?
>>
>> https://codereview.appspot.**com/7188044/<https://codereview.appspot.com/7188044/>
>>
>
>
>
> --
> INADA Naoki <songofacandy@gmail.com>
>
Message from songofacandy@gmail.com
2013-01-22T11:04:44+00:00songofacandyurn:md5:a7341d5cb4ae30f79dae196975e767d8
On 2013/01/22 10:44:08, dfc wrote:
> We cannot change the signature of a public symbol already published in Go
> 1.0. It would break the api check in the build stage, and more importantly,
> break the Go 1.0 contract.
Then, can I change behavior of the Accept() to set CLOEXEC?
Are there any reason for don't set CLOEXEC to accepted socket?
Or should I add syscall.Accept4()?
Message from unknown
2013-01-22T17:32:40+00:00songofacandyurn:md5:b1d92d640902097234a945517a3ea079
Message from songofacandy@gmail.com
2013-01-22T17:32:47+00:00songofacandyurn:md5:3fd17933197342a2a0e85ea01c94a08d
Hello golang-dev@googlegroups.com, dave@cheney.net (cc: golang-dev@googlegroups.com),
Please take another look.
Message from unknown
2013-01-22T17:34:36+00:00songofacandyurn:md5:cf119bd0ddc888fa95b08e9bd07d51c7
Message from songofacandy@gmail.com
2013-01-22T17:34:42+00:00songofacandyurn:md5:705b84840780f7eeca91a2cf93d2a1c8
Hello golang-dev@googlegroups.com, dave@cheney.net (cc: golang-dev@googlegroups.com),
Please take another look.
Message from bradfitz@golang.org
2013-01-22T19:47:00+00:00bradfitzurn:md5:c07fa7e76478e3f7d3c7e06437200f94
It seems that the syscall.AcceptNB naming is a misleading use of "NB",
since it's rarely NB.
If we don't want to modify syscall.Accept to set CLOEXEC, we might want to
name it something like:
func AcceptCloexec(fd int) (nfd int, sa Sockaddr, err error) { ... }
... which is like Accept, but guarantees CLOEXEC is set (via whatever
means), using accept4 on Linux if available.
On Tue, Jan 22, 2013 at 1:59 AM, <songofacandy@gmail.com> wrote:
> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net: Use accept4() on Linux.
>
> Add syscall.AcceptNB() and net.fd.Accept() use it.
> AcceptNB() recieves nonblocking fd and returns new fd which is set
> nonblock and cloexec.
> On Linux, syscall.AcceptNB() is implemented by accept4() syscall.
>
> Please review this at https://codereview.appspot.**com/7188044/<https://codereview.appspot.com/7188044/>
>
> Affected files:
> M src/pkg/net/fd_unix.go
> M src/pkg/syscall/syscall_bsd.go
> M src/pkg/syscall/syscall_linux.**go
> M src/pkg/syscall/syscall_linux_**386.go
> M src/pkg/syscall/syscall_linux_**amd64.go
> M src/pkg/syscall/zsyscall_**linux_amd64.go
>
>
> Index: src/pkg/net/fd_unix.go
> ==============================**==============================**=======
> --- a/src/pkg/net/fd_unix.go
> +++ b/src/pkg/net/fd_unix.go
> @@ -615,16 +615,11 @@
> }
> defer fd.decref()
>
> - // See ../syscall/exec_unix.go for description of ForkLock.
> - // It is okay to hold the lock across syscall.Accept
> - // because we have put fd.sysfd into non-blocking mode.
> var s int
> var rsa syscall.Sockaddr
> for {
> - syscall.ForkLock.RLock()
> - s, rsa, err = syscall.Accept(fd.sysfd)
> + s, rsa, err = syscall.AcceptNB(fd.sysfd)
> if err != nil {
> - syscall.ForkLock.RUnlock()
> if err == syscall.EAGAIN {
> if err = fd.pollServer.WaitRead(fd); err
> == nil {
> continue
> @@ -638,8 +633,6 @@
> }
> break
> }
> - syscall.CloseOnExec(s)
> - syscall.ForkLock.RUnlock()
>
> if netfd, err = newFD(s, fd.family, fd.sotype, fd.net); err !=
> nil {
> closesocket(s)
> Index: src/pkg/syscall/syscall_bsd.go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/syscall_bsd.**go
> +++ b/src/pkg/syscall/syscall_bsd.**go
> @@ -320,6 +320,24 @@
> return
> }
>
> +func AcceptNB(fd int) (nfd int, sa Sockaddr, err error) {
> + // See exec_unix.go for description of ForkLock.
> + // It is okay to hold the lock across syscall.Accept
> + // because we have put fd.sysfd into non-blocking mode.
> + ForkLock.RLock()
> + nfd, sa, err = Accept(fd)
> + if err != nil {
> + ForkLock.RUnlock()
> + return
> + }
> + CloseOnExec(nfd)
> + ForkLock.RUnlock()
> + err = SetNonblock(nfd, true)
> + if err != nil {
> + Close(nfd)
> + }
> +}
> +
> func Getsockname(fd int) (sa Sockaddr, err error) {
> var rsa RawSockaddrAny
> var len _Socklen = SizeofSockaddrAny
> Index: src/pkg/syscall/syscall_linux.**go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/syscall_**linux.go
> +++ b/src/pkg/syscall/syscall_**linux.go
> @@ -427,6 +427,21 @@
> return
> }
>
> +func AcceptNB(fd int) (nfd int, sa Sockaddr, err error) {
> + var rsa RawSockaddrAny
> + var len _Socklen = SizeofSockaddrAny
> + nfd, err = accept4(fd, &rsa, &len, SOCK_NONBLOCK|SOCK_CLOEXEC)
> + if err != nil {
> + return
> + }
> + sa, err = anyToSockaddr(&rsa)
> + if err != nil {
> + Close(nfd)
> + nfd = 0
> + }
> + return
> +}
> +
> func Getsockname(fd int) (sa Sockaddr, err error) {
> var rsa RawSockaddrAny
> var len _Socklen = SizeofSockaddrAny
> Index: src/pkg/syscall/syscall_linux_**386.go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/syscall_**linux_386.go
> +++ b/src/pkg/syscall/syscall_**linux_386.go
> @@ -164,6 +164,7 @@
> _GETSOCKOPT = 15
> _SENDMSG = 16
> _RECVMSG = 17
> + _ACCEPT4 = 18
> )
>
> func socketcall(call int, a0, a1, a2, a3, a4, a5 uintptr) (n int, err
> Errno)
> @@ -177,6 +178,14 @@
> return
> }
>
> +func accept4(s int, rsa *RawSockaddrAny, addrlen *_Socklen, flags int)
> (fd int, err error) {
> + fd, e := socketcall(_ACCEPT4, uintptr(s),
> uintptr(unsafe.Pointer(rsa)), uintptr(unsafe.Pointer(**addrlen)),
> uintptr(flags), 0, 0)
> + if e != 0 {
> + err = e
> + }
> + return
> +}
> +
> func getsockname(s int, rsa *RawSockaddrAny, addrlen *_Socklen) (err
> error) {
> _, e := rawsocketcall(_GETSOCKNAME, uintptr(s),
> uintptr(unsafe.Pointer(rsa)), uintptr(unsafe.Pointer(**addrlen)), 0, 0, 0)
> if e != 0 {
> Index: src/pkg/syscall/syscall_linux_**amd64.go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/syscall_**linux_amd64.go
> +++ b/src/pkg/syscall/syscall_**linux_amd64.go
> @@ -39,6 +39,7 @@
> //sys SyncFileRange(fd int, off int64, n int64, flags int) (err error)
> //sys Truncate(path string, length int64) (err error)
> //sys accept(s int, rsa *RawSockaddrAny, addrlen *_Socklen) (fd int, err
> error)
> +//sysnb accept4(s int, rsa *RawSockaddrAny, addrlen *_Socklen,
> flags int) (fd int, err error)
> //sys bind(s int, addr uintptr, addrlen _Socklen) (err error)
> //sys connect(s int, addr uintptr, addrlen _Socklen) (err error)
> //sysnb getgroups(n int, list *_Gid_t) (nn int, err error)
> Index: src/pkg/syscall/zsyscall_**linux_amd64.go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/zsyscall_**linux_amd64.go
> +++ b/src/pkg/syscall/zsyscall_**linux_amd64.go
> @@ -1721,6 +1721,17 @@
>
> // THIS FILE IS GENERATED BY THE COMMAND AT THE TOP; DO NOT EDIT
>
> +func accept4(s int, rsa *RawSockaddrAny, addrlen *_Socklen, flags int)
> (fd int, err error) {
> + r0, _, e1 := RawSyscall6(SYS_ACCEPT4, uintptr(s),
> uintptr(unsafe.Pointer(rsa)), uintptr(unsafe.Pointer(**addrlen)),
> uintptr(flags), 0, 0)
> + fd = int(r0)
> + if e1 != 0 {
> + err = e1
> + }
> + return
> +}
> +
> +// THIS FILE IS GENERATED BY THE COMMAND AT THE TOP; DO NOT EDIT
> +
> func bind(s int, addr uintptr, addrlen _Socklen) (err error) {
> _, _, e1 := Syscall(SYS_BIND, uintptr(s), uintptr(addr),
> uintptr(addrlen))
> if e1 != 0 {
>
>
>
Message from songofacandy@gmail.com
2013-01-22T23:46:36+00:00songofacandyurn:md5:5005b21a239b268085fe389fbb3c390e
On 2013/01/22 19:47:00, bradfitz wrote:
> It seems that the syscall.AcceptNB naming is a misleading use of "NB",
> since it's rarely NB.
When accept4 is not available, AcceptNB() can't use ForkLock without knowing
the fd is blocking or not. This is why I named it "NB".
>
> If we don't want to modify syscall.Accept to set CLOEXEC, we might want to
> name it something like:
>
> func AcceptCloexec(fd int) (nfd int, sa Sockaddr, err error) { ... }
>
> ... which is like Accept, but guarantees CLOEXEC is set (via whatever
> means), using accept4 on Linux if available.
Now I change the name of new function as Accept4().
It returns ENOSYS when accept4 is not available and caller should fallback to
syscall.Accept() with ForkLock.
Message from dave@cheney.net
2013-01-22T23:50:00+00:00dfcurn:md5:6d79dcb42367472520634503835d7b77
> Now I change the name of new function as Accept4().
> It returns ENOSYS when accept4 is not available and caller should
> fallback to
> syscall.Accept() with ForkLock.
I think we can make this simpler by moving the decision to use accept
or accept4 into syscall.Accept. I will try to work up a CL today.
Message from bradfitz@golang.org
2013-01-22T23:54:27+00:00bradfitzurn:md5:7049aadb4f812e756532489b560e09e9
On Tue, Jan 22, 2013 at 3:49 PM, Dave Cheney <dave@cheney.net> wrote:
> > Now I change the name of new function as Accept4().
> > It returns ENOSYS when accept4 is not available and caller should
> > fallback to
> > syscall.Accept() with ForkLock.
>
> I think we can make this simpler by moving the decision to use accept
> or accept4 into syscall.Accept. I will try to work up a CL today.
I don't think we should change the behavior of go1's syscall.Accept.
In Go 1, syscall.Accept did not set the fd's CLOEXEC and it did not take
locks.
If we're going to be doing new things, I think we need a new function. I
still don't think /NB$/ is the right naming, especially if it might grab
locks.
Let's just add syscall.Accept4 for now (which can return ENOSYS) and do
+build stuff in pkg/net until we can decide whether we want to make an
Accept helper (cloexec'ing + syscall picking) in pkg syscall, and what it
should be named.
Message from songofacandy@gmail.com
2013-01-22T23:55:55+00:00songofacandyurn:md5:e7dab070e83d5cef3ac2da47c1c9d3c2
On 2013/01/22 23:46:36, songofacandy wrote:
> On 2013/01/22 19:47:00, bradfitz wrote:
> > It seems that the syscall.AcceptNB naming is a misleading use of "NB",
> > since it's rarely NB.
>
> When accept4 is not available, AcceptNB() can't use ForkLock without knowing
> the fd is blocking or not. This is why I named it "NB".
>
Accept() can know the fd is blocking or not with fcntl().
But it introduces additional overhead.
Caller should tell that ForkLock is needed or not.
Message from dave@cheney.net
2013-01-22T23:57:21+00:00dfcurn:md5:4b537ff6cbbde2f0dc166a99dc4e99f7
+1 for Accept4() on linux platforms only.
On Wed, Jan 23, 2013 at 10:54 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
>
> On Tue, Jan 22, 2013 at 3:49 PM, Dave Cheney <dave@cheney.net> wrote:
>>
>> > Now I change the name of new function as Accept4().
>> > It returns ENOSYS when accept4 is not available and caller should
>> > fallback to
>> > syscall.Accept() with ForkLock.
>>
>> I think we can make this simpler by moving the decision to use accept
>> or accept4 into syscall.Accept. I will try to work up a CL today.
>
>
> I don't think we should change the behavior of go1's syscall.Accept.
>
> In Go 1, syscall.Accept did not set the fd's CLOEXEC and it did not take
> locks.
>
> If we're going to be doing new things, I think we need a new function. I
> still don't think /NB$/ is the right naming, especially if it might grab
> locks.
>
> Let's just add syscall.Accept4 for now (which can return ENOSYS) and do
> +build stuff in pkg/net until we can decide whether we want to make an
> Accept helper (cloexec'ing + syscall picking) in pkg syscall, and what it
> should be named.
>
Message from unknown
2013-01-23T01:30:51+00:00songofacandyurn:md5:1b3be6705f1992a7188a36c6e57f54f7
Message from songofacandy@gmail.com
2013-01-23T01:30:58+00:00songofacandyurn:md5:9d7bfd46fd75ffa355f158bbf526ccdb
Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org (cc: golang-dev@googlegroups.com),
Please take another look.
Message from unknown
2013-01-23T01:31:19+00:00songofacandyurn:md5:9ded118aa4eeaff1415834beca3b7ca7
Message from songofacandy@gmail.com
2013-01-23T01:31:24+00:00songofacandyurn:md5:323f3acb2e39018dd802958a8085907b
Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org (cc: golang-dev@googlegroups.com),
Please take another look.
Message from bradfitz@golang.org
2013-01-23T01:34:46+00:00bradfitzurn:md5:fe7fbd62c2cd17999974c7519105f8ad
https://codereview.appspot.com/7188044/diff/5009/src/pkg/net/fd_bsd.go
File src/pkg/net/fd_bsd.go (right):
https://codereview.appspot.com/7188044/diff/5009/src/pkg/net/fd_bsd.go#newcode120
src/pkg/net/fd_bsd.go:120: func (fd *netFD) accept(toAddr func(syscall.Sockaddr) Addr) (netfd *netFD, err error) {
this is a lot of subtle code to copy/paste between two files entirely. I'd rather move this to a new file (accept_bsd.go and add +build to include darwin) than have it in twice.
and where is linux?
Message from rsc@golang.org
2013-01-23T02:52:17+00:00rscurn:md5:7c08301521a56bd6d1cad5d04431ff80
Do we know that this makes anything faster?
Message from unknown
2013-01-23T02:58:55+00:00songofacandyurn:md5:e92ceb21b8c7644ac012966d4a442261
Message from songofacandy@gmail.com
2013-01-23T02:59:02+00:00songofacandyurn:md5:e1019c65a9d4bfe41b03754661691c14
Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),
Please take another look.
Message from unknown
2013-01-23T02:59:17+00:00songofacandyurn:md5:d1883c1207e66d522b6d4c2211fe574b
Message from songofacandy@gmail.com
2013-01-23T02:59:24+00:00songofacandyurn:md5:14ccefcd58ab465debd7374aadee9e03
Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),
Please take another look.
Message from unknown
2013-01-23T03:01:15+00:00songofacandyurn:md5:f87d2c35b776608ff48208b93ed8fa14
Message from songofacandy@gmail.com
2013-01-23T03:01:21+00:00songofacandyurn:md5:9675041e3d9d5905957915b8b807bb77
Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),
Please take another look.
Message from bradfitz@golang.org
2013-01-23T03:17:51+00:00bradfitzurn:md5:5f253d957434d3569fd2b58b0dcb792d
Per earlier thread, it seemed to reduce fork lock contention and qps from
siege increased.
On Jan 22, 2013 6:52 PM, "Russ Cox" <rsc@golang.org> wrote:
> Do we know that this makes anything faster?
>
>
Message from mikioh.mikioh@gmail.com
2013-01-23T03:18:08+00:00mikiourn:md5:f1076b38ef098f7fdeeaeea9ea5e1f50
On Wed, Jan 23, 2013 at 11:52 AM, Russ Cox <rsc@golang.org> wrote:
> Do we know that this makes anything faster?
This patch just reduces # of syscalls at inbound TCP reception on Linux,
so not for anything network ops.
The comment #34 on issue 3412 shows HTTP transaction numbers.
I guess Dave or Brad will suggest to run TCP benchmarks once we
have clean patches.
Message from rsc@golang.org
2013-01-23T03:28:22+00:00rscurn:md5:f639ef54e2187161a5aeba1ee6c36a08
I'm pretty surprised about ForkLock contention. It's an RWMutex that is
almost never W-locked. The lock/unlock paths are each a single atomic add.
I'm surprised that shows up in a profile.
Russ
Message from bradfitz@golang.org
2013-01-23T03:41:48+00:00bradfitzurn:md5:99cae6efd705a75b0fb4065b173b25c5
The siege benchmarks also included the ReadNB stuff but we thought nobody
could make that look better in benchmarks. Hence attributing it to
ForkLock. I haven't actually looked at block profiles or done any
benchmarks.
On Jan 22, 2013 7:28 PM, "Russ Cox" <rsc@golang.org> wrote:
> I'm pretty surprised about ForkLock contention. It's an RWMutex that is
> almost never W-locked. The lock/unlock paths are each a single atomic add.
> I'm surprised that shows up in a profile.
>
> Russ
>
>
Message from unknown
2013-01-23T06:07:36+00:00songofacandyurn:md5:a05b0513303801ae17940a3d558823c5
Message from songofacandy@gmail.com
2013-01-23T06:07:44+00:00songofacandyurn:md5:f029e714830399feb701fdbbd0f73904
Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org, rsc@golang.org, mikioh.mikioh@gmail.com (cc: golang-dev@googlegroups.com),
Please take another look.
Message from dave@cheney.net
2013-01-23T06:09:40+00:00dfcurn:md5:7fba6a9fc76a46ef6b4209ef5c739b9d
You do not need to use hg mail every time you want to upload a change
to the code review server.
Use hg upload NNNN to upload to codereview, then hg mail when you are
happy with the final revision.
On Wed, Jan 23, 2013 at 5:07 PM, <songofacandy@gmail.com> wrote:
> Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org,
> rsc@golang.org, mikioh.mikioh@gmail.com (cc:
>
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> https://codereview.appspot.com/7188044/
Message from mikioh.mikioh@gmail.com
2013-01-23T06:10:40+00:00mikiourn:md5:7e3e9081f71b7fc900cfff7534652718
could you please split this CL into two, syscall and net.
seems like we need accept4 for linux at first.
Message from songofacandy@gmail.com
2013-01-23T06:11:47+00:00songofacandyurn:md5:930b875b38842f4b76d82a573be2ae22
I have run micro benchmark and find that performance improvement comes from syscall.SetNonblock.
benchmark code: https://gist.github.com/4602406
GOMAXPROCS=8
with accept4:
$ time ./client
real 0m5.571s
user 0m0.124s
sys 0m2.700s
w/o accept4:
$ time ./client
real 0m5.731s
user 0m0.116s
sys 0m2.692s
On 2013/01/23 03:28:22, rsc wrote:
> I'm pretty surprised about ForkLock contention. It's an RWMutex that is
> almost never W-locked. The lock/unlock paths are each a single atomic add.
> I'm surprised that shows up in a profile.
>
> Russ
Message from dave@cheney.net
2013-01-23T06:15:24+00:00dfcurn:md5:e5f141cbcfc4fe63b6c4dedc2215d31f
I'm concerned at how large a change this is. If this CL works then lets use it as a checkpoint to do benchmark comparisons and then consider ways to make the change smaller if it is an improvement.
https://codereview.appspot.com/7188044/diff/17001/src/pkg/net/accept_linux.go
File src/pkg/net/accept_linux.go (right):
https://codereview.appspot.com/7188044/diff/17001/src/pkg/net/accept_linux.go#newcode11
src/pkg/net/accept_linux.go:11: var use_accept4 = true
Idiomatic Go code uses camel case.
Also, this is not thread safe.
https://codereview.appspot.com/7188044/diff/17001/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):
https://codereview.appspot.com/7188044/diff/17001/src/pkg/net/sock_posix.go#newcode53
src/pkg/net/sock_posix.go:53: err = syscall.SetNonblock(s, true)
Collapse this onto one line
Message from dave@cheney.net
2013-01-23T06:16:44+00:00dfcurn:md5:c386b7fc3a5a6667b87caabbfd7f52a8
Hmm, this isn't a big improvement, what about rerunning the siege
benchmarks that you originally posted. Please rerun them for
GOMAXSPROCS={1,2,4,8}
> GOMAXPROCS=8
> with accept4:
> $ time ./client
>
> real 0m5.571s
> user 0m0.124s
> sys 0m2.700s
>
> w/o accept4:
>
> $ time ./client
>
> real 0m5.731s
> user 0m0.116s
> sys 0m2.692s
>
>
> On 2013/01/23 03:28:22, rsc wrote:
>>
>> I'm pretty surprised about ForkLock contention. It's an RWMutex that
>
> is
>>
>> almost never W-locked. The lock/unlock paths are each a single atomic
>
> add.
>>
>> I'm surprised that shows up in a profile.
>
>
>> Russ
>
>
>
> https://codereview.appspot.com/7188044/
Message from dave@cheney.net
2013-01-23T06:20:43+00:00dfcurn:md5:a8cc04216aa918c76bb2b1323db3ee82
Here is a possibly simpler way to benchmark this change -- hack syscall.Accept to call accept4 with a hardcoded set of flags. That should be a pretty small change.
Message from unknown
2013-01-23T06:26:44+00:00songofacandyurn:md5:a209730e449e1356e0a732c969a20e3d
Message from songofacandy@gmail.com
2013-01-23T06:26:50+00:00songofacandyurn:md5:b53c1ba22b73e821ce4814c72d07acfb
Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org, rsc@golang.org, mikioh.mikioh@gmail.com (cc: golang-dev@googlegroups.com),
Please take another look.
Message from dave@cheney.net
2013-01-23T06:29:23+00:00dfcurn:md5:08ba7e0c62b03ff24a5da3507111b2e3
Please, lets stop furious hacking, and come up with some benchmark numbers that support this change.
https://codereview.appspot.com/7188044/diff/23001/src/pkg/net/accept_linux.go
File src/pkg/net/accept_linux.go (right):
https://codereview.appspot.com/7188044/diff/23001/src/pkg/net/accept_linux.go#newcode21
src/pkg/net/accept_linux.go:21: accept4 := useAccept4
sorry, this hasn't fixed it. You could use an atomic boolean, or possibly an init function to determine if accept4 is supported before the program runs.
Message from songofacandy@gmail.com
2013-01-23T06:30:57+00:00songofacandyurn:md5:4197d5c3a2bb6bf272b132f1e1bd481b
On 2013/01/23 06:20:43, dfc wrote:
> Here is a possibly simpler way to benchmark this change -- hack syscall.Accept
> to call accept4 with a hardcoded set of flags. That should be a pretty small
> change.
And remove calling syscall.SetCloexec and syscall.SetNonblock.
Message from songofacandy@gmail.com
2013-01-23T06:53:20+00:00songofacandyurn:md5:129e78ea33ac2b380d693d83da25d20d
On 2013/01/23 06:16:44, dfc wrote:
> Hmm, this isn't a big improvement, what about rerunning the siege
> benchmarks that you originally posted. Please rerun them for
> GOMAXSPROCS={1,2,4,8}
No significant gain for GOMAXPROCS>=2 (~1%)
https://gist.github.com/4602686
>
> > GOMAXPROCS=8
> > with accept4:
> > $ time ./client
> >
> > real 0m5.571s
> > user 0m0.124s
> > sys 0m2.700s
> >
> > w/o accept4:
> >
> > $ time ./client
> >
> > real 0m5.731s
> > user 0m0.116s
> > sys 0m2.692s
> >
> >
> > On 2013/01/23 03:28:22, rsc wrote:
> >>
> >> I'm pretty surprised about ForkLock contention. It's an RWMutex that
> >
> > is
> >>
> >> almost never W-locked. The lock/unlock paths are each a single atomic
> >
> > add.
> >>
> >> I'm surprised that shows up in a profile.
> >
> >
> >> Russ
> >
> >
> >
> > https://codereview.appspot.com/7188044/
Message from rsc@golang.org
2013-01-30T16:57:56+00:00rscurn:md5:313718df96d49ad2865545538ebddc90
Removing reviewers; I believe Ian's CL supercedes this.
Message from songofacandy@gmail.com
2013-01-30T23:24:42+00:00songofacandyurn:md5:2229771eb789782b34389b12f61c5988
On 2013/01/30 16:57:56, rsc wrote:
> Removing reviewers; I believe Ian's CL supercedes this.
I hope https://codereview.appspot.com/7241050/ is merged.
Message from dave@cheney.net
2013-01-30T23:32:23+00:00dfcurn:md5:1cc2e1a29e0931c6386bf572ffcf3575
Iant's previous CL demonstrated a 10% speedup in GOMAXPROCS=1 and 0%
speedup in GOMAXPROCS=4 on one test system.
I think we should only continue to tinker with this set of changes if
they can be shown to demonstrate a speedup.
Are you able to benchmark Iant's change ?
Cheers
Dave
On Thu, Jan 31, 2013 at 10:24 AM, <songofacandy@gmail.com> wrote:
> On 2013/01/30 16:57:56, rsc wrote:
>>
>> Removing reviewers; I believe Ian's CL supercedes this.
>
>
> I hope https://codereview.appspot.com/7241050/ is merged.
>
>
> https://codereview.appspot.com/7188044/
>
> --
>
> ---You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>