Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(767)

Issue 7188044: code review 7188044: net: Use accept4() on Linux. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 5 months ago by songofacandy
Modified:
6 years, 4 months ago
Reviewers:
dfc
CC:
golang-dev
Visibility:
Public.

Description

net: Use accept4() on Linux. Add syscall.Accept4() and use it in net.fd.Accept(). Fallback to Accept() when Accept4() returns ENOSYS.

Patch Set 1 #

Patch Set 2 : diff -r 2da48f86d386 https://code.google.com/p/go #

Patch Set 3 : diff -r 2da48f86d386 https://code.google.com/p/go #

Patch Set 4 : diff -r 2da48f86d386 https://code.google.com/p/go #

Patch Set 5 : diff -r 2da48f86d386 https://code.google.com/p/go #

Patch Set 6 : diff -r 2da48f86d386 https://code.google.com/p/go #

Patch Set 7 : diff -r 2da48f86d386 https://code.google.com/p/go #

Total comments: 1

Patch Set 8 : diff -r 2da48f86d386 https://code.google.com/p/go #

Patch Set 9 : diff -r 2da48f86d386 https://code.google.com/p/go #

Patch Set 10 : diff -r 2da48f86d386 https://code.google.com/p/go #

Patch Set 11 : diff -r 2da48f86d386 https://code.google.com/p/go #

Total comments: 2

Patch Set 12 : diff -r 2da48f86d386 https://code.google.com/p/go #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+164 lines, -48 lines) Patch
A src/pkg/net/accept_bsd.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +53 lines, -0 lines 0 comments Download
A src/pkg/net/accept_linux.go View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +69 lines, -0 lines 1 comment Download
M src/pkg/net/fd_unix.go View 1 2 3 4 5 6 7 8 9 10 3 chunks +2 lines, -46 lines 0 comments Download
M src/pkg/net/file_unix.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -1 line 0 comments Download
M src/pkg/net/sock_posix.go View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -1 line 0 comments Download
M src/pkg/syscall/syscall_linux.go View 1 2 3 1 chunk +15 lines, -0 lines 0 comments Download
M src/pkg/syscall/syscall_linux_386.go View 1 2 chunks +9 lines, -0 lines 0 comments Download
M src/pkg/syscall/syscall_linux_amd64.go View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M src/pkg/syscall/zsyscall_linux_amd64.go View 1 2 3 1 chunk +11 lines, -0 lines 0 comments Download

Messages

Total messages: 38
songofacandy
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
6 years, 5 months ago (2013-01-22 09:59:22 UTC) #1
dfc
On 2013/01/22 09:59:22, songofacandy wrote: > Hello mailto:golang-dev@googlegroups.com, > > I'd like you to review ...
6 years, 5 months ago (2013-01-22 10:16:01 UTC) #2
songofacandy
How can I emulate accept4 on bsd? Changing the signature of Accept() like following is ...
6 years, 5 months ago (2013-01-22 10:38:23 UTC) #3
dfc
We cannot change the signature of a public symbol already published in Go 1.0. It ...
6 years, 5 months ago (2013-01-22 10:44:08 UTC) #4
songofacandy
On 2013/01/22 10:44:08, dfc wrote: > We cannot change the signature of a public symbol ...
6 years, 5 months ago (2013-01-22 11:04:44 UTC) #5
songofacandy
Hello golang-dev@googlegroups.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 4 months ago (2013-01-22 17:32:47 UTC) #6
songofacandy
Hello golang-dev@googlegroups.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 4 months ago (2013-01-22 17:34:42 UTC) #7
bradfitz
It seems that the syscall.AcceptNB naming is a misleading use of "NB", since it's rarely ...
6 years, 4 months ago (2013-01-22 19:47:00 UTC) #8
songofacandy
On 2013/01/22 19:47:00, bradfitz wrote: > It seems that the syscall.AcceptNB naming is a misleading ...
6 years, 4 months ago (2013-01-22 23:46:36 UTC) #9
dfc
> Now I change the name of new function as Accept4(). > It returns ENOSYS ...
6 years, 4 months ago (2013-01-22 23:50:00 UTC) #10
bradfitz
On Tue, Jan 22, 2013 at 3:49 PM, Dave Cheney <dave@cheney.net> wrote: > > Now ...
6 years, 4 months ago (2013-01-22 23:54:27 UTC) #11
songofacandy
On 2013/01/22 23:46:36, songofacandy wrote: > On 2013/01/22 19:47:00, bradfitz wrote: > > It seems ...
6 years, 4 months ago (2013-01-22 23:55:55 UTC) #12
dfc
+1 for Accept4() on linux platforms only. On Wed, Jan 23, 2013 at 10:54 AM, ...
6 years, 4 months ago (2013-01-22 23:57:21 UTC) #13
songofacandy
Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 4 months ago (2013-01-23 01:30:58 UTC) #14
songofacandy
Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 4 months ago (2013-01-23 01:31:24 UTC) #15
bradfitz
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 ...
6 years, 4 months ago (2013-01-23 01:34:46 UTC) #16
rsc
Do we know that this makes anything faster?
6 years, 4 months ago (2013-01-23 02:52:17 UTC) #17
songofacandy
Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 4 months ago (2013-01-23 02:59:02 UTC) #18
songofacandy
Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 4 months ago (2013-01-23 02:59:24 UTC) #19
songofacandy
Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 4 months ago (2013-01-23 03:01:21 UTC) #20
bradfitz
Per earlier thread, it seemed to reduce fork lock contention and qps from siege increased. ...
6 years, 4 months ago (2013-01-23 03:17:51 UTC) #21
mikio
On Wed, Jan 23, 2013 at 11:52 AM, Russ Cox <rsc@golang.org> wrote: > Do we ...
6 years, 4 months ago (2013-01-23 03:18:08 UTC) #22
rsc
I'm pretty surprised about ForkLock contention. It's an RWMutex that is almost never W-locked. The ...
6 years, 4 months ago (2013-01-23 03:28:22 UTC) #23
bradfitz
The siege benchmarks also included the ReadNB stuff but we thought nobody could make that ...
6 years, 4 months ago (2013-01-23 03:41:48 UTC) #24
songofacandy
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.
6 years, 4 months ago (2013-01-23 06:07:44 UTC) #25
dfc
You do not need to use hg mail every time you want to upload a ...
6 years, 4 months ago (2013-01-23 06:09:40 UTC) #26
mikio
could you please split this CL into two, syscall and net. seems like we need ...
6 years, 4 months ago (2013-01-23 06:10:40 UTC) #27
songofacandy
I have run micro benchmark and find that performance improvement comes from syscall.SetNonblock. benchmark code: ...
6 years, 4 months ago (2013-01-23 06:11:47 UTC) #28
dfc
I'm concerned at how large a change this is. If this CL works then lets ...
6 years, 4 months ago (2013-01-23 06:15:24 UTC) #29
dfc
Hmm, this isn't a big improvement, what about rerunning the siege benchmarks that you originally ...
6 years, 4 months ago (2013-01-23 06:16:44 UTC) #30
dfc
Here is a possibly simpler way to benchmark this change -- hack syscall.Accept to call ...
6 years, 4 months ago (2013-01-23 06:20:43 UTC) #31
songofacandy
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.
6 years, 4 months ago (2013-01-23 06:26:50 UTC) #32
dfc
Please, lets stop furious hacking, and come up with some benchmark numbers that support this ...
6 years, 4 months ago (2013-01-23 06:29:23 UTC) #33
songofacandy
On 2013/01/23 06:20:43, dfc wrote: > Here is a possibly simpler way to benchmark this ...
6 years, 4 months ago (2013-01-23 06:30:57 UTC) #34
songofacandy
On 2013/01/23 06:16:44, dfc wrote: > Hmm, this isn't a big improvement, what about rerunning ...
6 years, 4 months ago (2013-01-23 06:53:20 UTC) #35
rsc
Removing reviewers; I believe Ian's CL supercedes this.
6 years, 4 months ago (2013-01-30 16:57:56 UTC) #36
songofacandy
On 2013/01/30 16:57:56, rsc wrote: > Removing reviewers; I believe Ian's CL supercedes this. I ...
6 years, 4 months ago (2013-01-30 23:24:42 UTC) #37
dfc
6 years, 4 months ago (2013-01-30 23:32:23 UTC) #38
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.
>
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b