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

Issue 6813046: code review 6813046: net: use WriteNB on non-blocking sockets (Closed)

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

Description

net: use WriteNB on non-blocking sockets Update issue 3412. Requires: https://codereview.appspot.com/7126043/

Patch Set 1 #

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

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

Total comments: 1

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

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

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

Total comments: 1

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

Total comments: 1

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

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

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

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

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -1 line) Patch
M src/pkg/net/fd_unix.go View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 44
dfc
Hello mikioh.mikioh@gmail.com, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
6 years, 7 months ago (2012-10-28 16:44:53 UTC) #1
dfc
Cross compilation tests pass on all OSs, but I don't have access to darwin for ...
6 years, 7 months ago (2012-10-28 16:46:28 UTC) #2
bradfitz
LGTM but you might want to wait for others. I don't see how this could ...
6 years, 7 months ago (2012-10-28 16:54:41 UTC) #3
minux1
for such a small change, i think you can just manually generate zsyscall_GOOS_GOARCH.go files (just ...
6 years, 7 months ago (2012-10-28 16:56:46 UTC) #4
dfc
> for such a small change, i think you can just manually generate > zsyscall_GOOS_GOARCH.go ...
6 years, 7 months ago (2012-10-28 17:05:28 UTC) #5
r
NOT LGTM the semantic change is disturbing. networks are used for things other than http ...
6 years, 7 months ago (2012-10-28 17:14:19 UTC) #6
minux1
On 2012/10/28 17:05:28, dfc wrote: > Are you suggesting adding > > // sys writeNB(...) ...
6 years, 7 months ago (2012-10-28 17:19:53 UTC) #7
bradfitz
Could you elaborate? He's not changing the semantics or implementation of syscall.Write. He's just adding ...
6 years, 7 months ago (2012-10-28 17:20:17 UTC) #8
r
I misunderstood where the magic was happening. I withdraw my objection. As usual, networking is ...
6 years, 7 months ago (2012-10-28 17:27:48 UTC) #9
dfc
Hello mikioh.mikioh@gmail.com, remyoudompheng@gmail.com, bradfitz@golang.org, minux.ma@gmail.com, r@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 7 months ago (2012-10-28 17:46:37 UTC) #10
ioe
Just verified it by looking at linux kernel code. Writing to non-blocking file descriptors might ...
6 years, 7 months ago (2012-10-28 18:11:10 UTC) #11
remyoudompheng
I still don't see the corresponding ReadNB
6 years, 7 months ago (2012-10-28 19:42:05 UTC) #12
dfc
I would be happy to add this in a follow-up CL. In the profiles that ...
6 years, 7 months ago (2012-10-28 22:09:50 UTC) #13
bradfitz
I can't find the numbers in that thread. Or rather, there were a bunch of ...
6 years, 7 months ago (2012-10-29 13:32:31 UTC) #14
iant
LGTM https://codereview.appspot.com/6813046/diff/8020/src/pkg/syscall/syscall_unix.go File src/pkg/syscall/syscall_unix.go (right): https://codereview.appspot.com/6813046/diff/8020/src/pkg/syscall/syscall_unix.go#newcode146 src/pkg/syscall/syscall_unix.go:146: // WriteNB should only be called when it ...
6 years, 7 months ago (2012-10-29 16:11:44 UTC) #15
dfc
> WriteNB does a write system call to a descriptor that has been set to ...
6 years, 7 months ago (2012-10-29 16:13:57 UTC) #16
bradfitz
On Mon, Oct 29, 2012 at 5:13 PM, Dave Cheney <dave@cheney.net> wrote: > > WriteNB ...
6 years, 7 months ago (2012-10-29 16:15:03 UTC) #17
mikio
https://codereview.appspot.com/6813046/diff/11003/src/pkg/syscall/syscall_unix.go File src/pkg/syscall/syscall_unix.go (right): https://codereview.appspot.com/6813046/diff/11003/src/pkg/syscall/syscall_unix.go#newcode150 src/pkg/syscall/syscall_unix.go:150: func WriteNB(fd int, p []byte) (n int, err error) ...
6 years, 7 months ago (2012-10-30 06:36:38 UTC) #18
dfc
WriteNB takes its name from the sysnb annotation that mksyscall.plconsumes. In this case I think ...
6 years, 7 months ago (2012-10-30 06:50:58 UTC) #19
mikio
On Tue, Oct 30, 2012 at 3:50 PM, Dave Cheney <dave@cheney.net> wrote: > WriteNB takes ...
6 years, 7 months ago (2012-10-30 06:56:30 UTC) #20
mikio
LGTM it passes on freebsd/amd64.
6 years, 7 months ago (2012-10-30 06:58:52 UTC) #21
dfc
Excellent, thanks for testing. On 30 Oct 2012 07:58, <mikioh.mikioh@gmail.com> wrote: > LGTM > > ...
6 years, 7 months ago (2012-10-30 07:05:13 UTC) #22
dfc
Thank you to those who reviewed this CL. I was traveling at the time so ...
6 years, 7 months ago (2012-11-18 04:23:43 UTC) #23
dfc
After (re)reading the siege documentation i've come up with some interesting results. Test hardware, Lenovo ...
6 years, 7 months ago (2012-11-21 05:31:12 UTC) #24
bradfitz
Any change with larger writes? Those responses look quite small. Personally I'm not concerned either ...
6 years, 7 months ago (2012-11-21 06:18:02 UTC) #25
remyoudompheng
On 2012/11/21 05:31:12, dfc wrote: > After (re)reading the siege documentation i've come up with ...
6 years, 7 months ago (2012-11-21 07:43:30 UTC) #26
dfc
I'm sorry I did not measure that. I was fighting at with running out of ...
6 years, 7 months ago (2012-11-21 07:56:56 UTC) #27
remyoudompheng
On 2012/11/21 07:56:56, dfc wrote: > I'm sorry I did not measure that. I was ...
6 years, 7 months ago (2012-11-21 21:53:03 UTC) #28
dfc
PTAL. This CL should apply cleanly now. I will try to find some time to ...
6 years, 5 months ago (2013-01-15 06:12:36 UTC) #29
bradfitz
If you're nervous about checking this in, I would at least be fine with you ...
6 years, 5 months ago (2013-01-15 06:29:45 UTC) #30
dfc
Thanks Brad. I like the idea of splitting this into two diffs, I'll do that. ...
6 years, 5 months ago (2013-01-15 06:34:27 UTC) #31
remyoudompheng
I'd like we have dvyukov's opinion about the viability of this change.
6 years, 5 months ago (2013-01-15 19:30:50 UTC) #32
dvyukov
Do we actually want to add public functions for that? It's irreversible change and we ...
6 years, 5 months ago (2013-01-15 20:15:52 UTC) #33
dvyukov
On 2013/01/15 20:15:52, dvyukov wrote: > Do we actually want to add public functions for ...
6 years, 5 months ago (2013-01-15 20:18:16 UTC) #34
dfc
On 2013/01/15 20:15:52, dvyukov wrote: > Do we actually want to add public functions for ...
6 years, 5 months ago (2013-01-15 20:18:54 UTC) #35
dvyukov
I am surprised that it provides only 10% speedup. Try net benchmarks with GOMAXPROCS=1,2,4,8,16...
6 years, 5 months ago (2013-01-15 20:19:18 UTC) #36
bradfitz
On Tue, Jan 15, 2013 at 12:15 PM, <dvyukov@google.com> wrote: > Do we actually want ...
6 years, 5 months ago (2013-01-15 20:43:53 UTC) #37
Sebastien Paolacci
Hello Dmitriy / All, Few weeks ago I write and play with an edge triggered ...
6 years, 5 months ago (2013-01-15 22:55:50 UTC) #38
dfc
Hello mikioh.mikioh@gmail.com, remyoudompheng@gmail.com, bradfitz@golang.org, minux.ma@gmail.com, r@golang.org, nightlyone@googlemail.com, iant@golang.org, dvyukov@google.com, sebastien.paolacci@gmail.com (cc: golang-dev@googlegroups.com), Please take another ...
6 years, 5 months ago (2013-01-16 00:26:08 UTC) #39
rsc
NOT LGTM I still believe that scheduler changes should make it possible to eliminate this ...
6 years, 4 months ago (2013-01-30 16:33:48 UTC) #40
dvyukov
On 2013/01/30 16:33:48, rsc wrote: > NOT LGTM > > I still believe that scheduler ...
6 years, 4 months ago (2013-01-30 16:42:50 UTC) #41
rsc
Let's wait and see how the scheduler work turns out, please. Russ
6 years, 4 months ago (2013-01-30 17:25:58 UTC) #42
dfc
*** Abandoned ***
6 years, 4 months ago (2013-01-30 23:48:55 UTC) #43
dfc
6 years, 4 months ago (2013-01-30 23:50:05 UTC) #44
Message was sent while issue was closed.
On 2013/01/30 16:33:48, rsc wrote:
> NOT LGTM
> 
> I still believe that scheduler changes should make it possible to eliminate
this
> distinction between blocking and non-blocking system calls. Let's not expose
> that wart in any more public API.

sgtm. I think we've already implemented part of this with the accept4 CL. These
WriteNB changes have not shown to make a constant improvement.
Sign in to reply to this message.

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