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

Issue 6496054: code review 6496054: net: spread fd over several pollservers. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 3 months ago by Sebastien Paolacci
Modified:
10 years, 2 months ago
Reviewers:
CC:
rsc, dvyukov, dfc, mikio, bradfitz, remyoudompheng, golang-dev
Visibility:
Public.

Description

net: spread fd over several pollservers. Lighten contention without preventing further improvements on pollservers. Connections are spread over Min(GOMAXPROCS, NumCPU, 8) pollserver instances. Median of 10 runs, 4 cores @ 3.4GHz, amd/linux-3.2: BenchmarkTCPOneShot 171917 ns/op 175194 ns/op 1.91% BenchmarkTCPOneShot-2 101413 ns/op 109462 ns/op 7.94% BenchmarkTCPOneShot-4 91796 ns/op 35712 ns/op -61.10% BenchmarkTCPOneShot-6 90938 ns/op 30607 ns/op -66.34% BenchmarkTCPOneShot-8 90374 ns/op 29150 ns/op -67.75% BenchmarkTCPOneShot-16 101089 ns/op 111526 ns/op 10.32% BenchmarkTCPOneShotTimeout 174986 ns/op 178606 ns/op 2.07% BenchmarkTCPOneShotTimeout-2 101585 ns/op 110678 ns/op 8.95% BenchmarkTCPOneShotTimeout-4 91547 ns/op 35931 ns/op -60.75% BenchmarkTCPOneShotTimeout-6 91496 ns/op 31019 ns/op -66.10% BenchmarkTCPOneShotTimeout-8 90670 ns/op 29531 ns/op -67.43% BenchmarkTCPOneShotTimeout-16 101013 ns/op 106026 ns/op 4.96% BenchmarkTCPPersistent 51731 ns/op 53324 ns/op 3.08% BenchmarkTCPPersistent-2 32888 ns/op 30678 ns/op -6.72% BenchmarkTCPPersistent-4 25751 ns/op 15595 ns/op -39.44% BenchmarkTCPPersistent-6 26737 ns/op 9805 ns/op -63.33% BenchmarkTCPPersistent-8 26850 ns/op 9730 ns/op -63.76% BenchmarkTCPPersistent-16 104449 ns/op 102838 ns/op -1.54% BenchmarkTCPPersistentTimeout 51806 ns/op 53281 ns/op 2.85% BenchmarkTCPPersistentTimeout-2 32956 ns/op 30895 ns/op -6.25% BenchmarkTCPPersistentTimeout-4 25994 ns/op 18111 ns/op -30.33% BenchmarkTCPPersistentTimeout-6 26679 ns/op 9846 ns/op -63.09% BenchmarkTCPPersistentTimeout-8 26810 ns/op 9727 ns/op -63.72% BenchmarkTCPPersistentTimeout-16 101652 ns/op 104410 ns/op 2.71%

Patch Set 1 #

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

Total comments: 1

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

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

Total comments: 2

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

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

Total comments: 5

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+49 lines, -20 lines) Patch
M src/pkg/net/fd_unix.go View 1 2 3 4 5 6 14 chunks +47 lines, -18 lines 0 comments Download
M src/pkg/net/sendfile_freebsd.go View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/net/sendfile_linux.go View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 37
Sebastien Paolacci
PTAL. As already observed by Dmitry, numbers really get crazy (and stable) for any cpu ...
10 years, 3 months ago (2012-08-29 06:53:20 UTC) #1
rsc
This looks nice, thanks. Is the speedup correlated with number of cores? Should we use ...
10 years, 2 months ago (2012-09-11 01:22:15 UTC) #2
dfc
I'm -1 on this change as it stands. Spreading the fds over a number of ...
10 years, 2 months ago (2012-09-11 01:32:27 UTC) #3
rsc
It would probably be okay to create poll servers on demand up to the max?
10 years, 2 months ago (2012-09-11 02:58:02 UTC) #4
dfc
SGTM On Tue, Sep 11, 2012 at 12:58 PM, Russ Cox <rsc@golang.org> wrote: > It ...
10 years, 2 months ago (2012-09-11 02:59:22 UTC) #5
dvyukov
On 2012/08/29 06:53:20, Sebastien Paolacci wrote: > PTAL. > > As already observed by Dmitry, ...
10 years, 2 months ago (2012-09-11 03:25:12 UTC) #6
dvyukov
On 2012/09/11 01:22:15, rsc wrote: > This looks nice, thanks. Is the speedup correlated with ...
10 years, 2 months ago (2012-09-11 03:29:55 UTC) #7
Sebastien Paolacci
On 2012/09/11 01:22:15, rsc wrote: > This looks nice, thanks. Is the speedup correlated with ...
10 years, 2 months ago (2012-09-11 19:23:05 UTC) #8
Sebastien Paolacci
On 2012/09/11 02:58:02, rsc wrote: > It would probably be okay to create poll servers ...
10 years, 2 months ago (2012-09-11 19:30:26 UTC) #9
Sebastien Paolacci
On 2012/09/11 03:25:12, dvyukov wrote: > On 2012/08/29 06:53:20, Sebastien Paolacci wrote: > > PTAL. ...
10 years, 2 months ago (2012-09-11 19:50:49 UTC) #10
Sebastien Paolacci
On 2012/09/11 03:29:55, dvyukov wrote: > It should be limited by min(GOMAXPROCS, NumCPU), because it ...
10 years, 2 months ago (2012-09-11 20:05:38 UTC) #11
Sebastien Paolacci
Hello rsc@golang.org, dvyukov@google.com, dave@cheney.net (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
10 years, 2 months ago (2012-09-11 20:10:19 UTC) #12
dvyukov
On 2012/09/11 19:30:26, Sebastien Paolacci wrote: > On 2012/09/11 02:58:02, rsc wrote: > > It ...
10 years, 2 months ago (2012-09-16 05:00:11 UTC) #13
dvyukov
On 2012/09/11 19:50:49, Sebastien Paolacci wrote: > On 2012/09/11 03:25:12, dvyukov wrote: > > On ...
10 years, 2 months ago (2012-09-16 05:05:02 UTC) #14
dvyukov
On 2012/09/11 20:05:38, Sebastien Paolacci wrote: > On 2012/09/11 03:29:55, dvyukov wrote: > > It ...
10 years, 2 months ago (2012-09-16 05:09:07 UTC) #15
Sebastien Paolacci
On 2012/09/16 05:09:07, dvyukov wrote: > 25% performance penalty does not look particularly cool. Indeed ...
10 years, 2 months ago (2012-09-19 20:39:38 UTC) #16
Sebastien Paolacci
Hello rsc@golang.org, dvyukov@google.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 2 months ago (2012-09-19 20:40:22 UTC) #17
Sebastien Paolacci
CL's description updated, here's last patch set result: Median of 10 runs, 4 cores @ ...
10 years, 2 months ago (2012-09-19 20:45:44 UTC) #18
dvyukov
LGTM
10 years, 2 months ago (2012-09-19 20:54:47 UTC) #19
mikio
https://codereview.appspot.com/6496054/diff/13002/src/pkg/net/sendfile_linux.go File src/pkg/net/sendfile_linux.go (right): https://codereview.appspot.com/6496054/diff/13002/src/pkg/net/sendfile_linux.go#newcode62 src/pkg/net/sendfile_linux.go:62: if err1 = c.pollServer().WaitWrite(c); err1 == nil { sendfile_freebsd.go ...
10 years, 2 months ago (2012-09-19 21:57:11 UTC) #20
Sebastien Paolacci
https://codereview.appspot.com/6496054/diff/13002/src/pkg/net/sendfile_linux.go File src/pkg/net/sendfile_linux.go (right): https://codereview.appspot.com/6496054/diff/13002/src/pkg/net/sendfile_linux.go#newcode62 src/pkg/net/sendfile_linux.go:62: if err1 = c.pollServer().WaitWrite(c); err1 == nil { On ...
10 years, 2 months ago (2012-09-19 22:13:51 UTC) #21
mikio
LGTM
10 years, 2 months ago (2012-09-19 22:19:52 UTC) #22
bradfitz
So if GOMAXPROCS changes at runtime, the pollServer() accessor can lose its *pollServer? Why not ...
10 years, 2 months ago (2012-09-19 22:33:57 UTC) #23
Sebastien Paolacci
Hello Brad, > So if GOMAXPROCS changes at runtime, the pollServer() accessor can > lose ...
10 years, 2 months ago (2012-09-20 08:28:12 UTC) #24
Sebastien Paolacci
I eventually manage to make `TestTCPListenClose' fail by injecting a dummy `rand.Int() % pollN' selection ...
10 years, 2 months ago (2012-09-20 11:03:18 UTC) #25
dfc
Thank you for your proposal, but I remain uncomfortable with it. In return for additional ...
10 years, 2 months ago (2012-09-20 11:12:49 UTC) #26
Sebastien Paolacci
Hello Dave, I'm not sure I really got your point. It indeed adds few lines ...
10 years, 2 months ago (2012-09-20 14:16:15 UTC) #27
bradfitz
https://codereview.appspot.com/6496054/diff/23001/src/pkg/net/fd_unix.go File src/pkg/net/fd_unix.go (right): https://codereview.appspot.com/6496054/diff/23001/src/pkg/net/fd_unix.go#newcode308 src/pkg/net/fd_unix.go:308: startServersOnce[k]() this line may crash if GOMAXPROCS has increased ...
10 years, 2 months ago (2012-09-20 18:43:18 UTC) #28
dvyukov
https://codereview.appspot.com/6496054/diff/23001/src/pkg/net/fd_unix.go File src/pkg/net/fd_unix.go (right): https://codereview.appspot.com/6496054/diff/23001/src/pkg/net/fd_unix.go#newcode308 src/pkg/net/fd_unix.go:308: startServersOnce[k]() On 2012/09/20 18:43:18, bradfitz wrote: > this line ...
10 years, 2 months ago (2012-09-20 19:15:11 UTC) #29
bradfitz
https://codereview.appspot.com/6496054/diff/23001/src/pkg/net/fd_unix.go File src/pkg/net/fd_unix.go (right): https://codereview.appspot.com/6496054/diff/23001/src/pkg/net/fd_unix.go#newcode308 src/pkg/net/fd_unix.go:308: startServersOnce[k]() On 2012/09/20 19:15:11, dvyukov wrote: > On 2012/09/20 ...
10 years, 2 months ago (2012-09-20 19:18:06 UTC) #30
dvyukov
https://codereview.appspot.com/6496054/diff/23001/src/pkg/net/fd_unix.go File src/pkg/net/fd_unix.go (right): https://codereview.appspot.com/6496054/diff/23001/src/pkg/net/fd_unix.go#newcode303 src/pkg/net/fd_unix.go:303: pollN := runtime.GOMAXPROCS(0) Please move this out of newFD(). ...
10 years, 2 months ago (2012-09-20 19:22:20 UTC) #31
Sebastien Paolacci
On 2012/09/20 19:22:20, dvyukov wrote: https://codereview.appspot.com/6496054/diff/23001/src/pkg/net/fd_unix.go#newcode303 > src/pkg/net/fd_unix.go:303: pollN := runtime.GOMAXPROCS(0) > Please move this ...
10 years, 2 months ago (2012-09-21 11:13:27 UTC) #32
rsc
> I would consider that anything beyond NumCPU is useless, that anything > smaller is ...
10 years, 2 months ago (2012-09-21 14:36:13 UTC) #33
Sebastien Paolacci
On 2012/09/21 14:36:13, rsc wrote: > > I would consider that anything beyond NumCPU is ...
10 years, 2 months ago (2012-09-21 16:43:17 UTC) #34
dvyukov
LGTM
10 years, 2 months ago (2012-09-21 20:16:35 UTC) #35
remyoudompheng
Is this going to be submitted?
10 years, 2 months ago (2012-09-26 19:27:42 UTC) #36
rsc
10 years, 2 months ago (2012-09-26 19:33:04 UTC) #37
*** Submitted as http://code.google.com/p/go/source/detail?r=19cf9361902a ***

net: spread fd over several pollservers.

Lighten contention without preventing further improvements on pollservers.
Connections are spread over Min(GOMAXPROCS, NumCPU, 8) pollserver instances.

Median of 10 runs, 4 cores @ 3.4GHz, amd/linux-3.2:

BenchmarkTCPOneShot                171917 ns/op   175194 ns/op      1.91%
BenchmarkTCPOneShot-2              101413 ns/op   109462 ns/op      7.94%
BenchmarkTCPOneShot-4               91796 ns/op    35712 ns/op    -61.10%
BenchmarkTCPOneShot-6               90938 ns/op    30607 ns/op    -66.34%
BenchmarkTCPOneShot-8               90374 ns/op    29150 ns/op    -67.75%
BenchmarkTCPOneShot-16             101089 ns/op   111526 ns/op     10.32%

BenchmarkTCPOneShotTimeout         174986 ns/op   178606 ns/op      2.07%
BenchmarkTCPOneShotTimeout-2       101585 ns/op   110678 ns/op      8.95%
BenchmarkTCPOneShotTimeout-4        91547 ns/op    35931 ns/op    -60.75%
BenchmarkTCPOneShotTimeout-6        91496 ns/op    31019 ns/op    -66.10%
BenchmarkTCPOneShotTimeout-8        90670 ns/op    29531 ns/op    -67.43%
BenchmarkTCPOneShotTimeout-16      101013 ns/op   106026 ns/op      4.96%

BenchmarkTCPPersistent              51731 ns/op    53324 ns/op      3.08%
BenchmarkTCPPersistent-2            32888 ns/op    30678 ns/op     -6.72%
BenchmarkTCPPersistent-4            25751 ns/op    15595 ns/op    -39.44%
BenchmarkTCPPersistent-6            26737 ns/op     9805 ns/op    -63.33%
BenchmarkTCPPersistent-8            26850 ns/op     9730 ns/op    -63.76%
BenchmarkTCPPersistent-16          104449 ns/op   102838 ns/op     -1.54%

BenchmarkTCPPersistentTimeout       51806 ns/op    53281 ns/op      2.85%
BenchmarkTCPPersistentTimeout-2     32956 ns/op    30895 ns/op     -6.25%
BenchmarkTCPPersistentTimeout-4     25994 ns/op    18111 ns/op    -30.33%
BenchmarkTCPPersistentTimeout-6     26679 ns/op     9846 ns/op    -63.09%
BenchmarkTCPPersistentTimeout-8     26810 ns/op     9727 ns/op    -63.72%
BenchmarkTCPPersistentTimeout-16   101652 ns/op   104410 ns/op      2.71%

R=rsc, dvyukov, dave, mikioh.mikioh, bradfitz, remyoudompheng
CC=golang-dev
http://codereview.appspot.com/6496054

Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.

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