net: move deadline logic into pollServer
Update issue 4434.
The proposal attempts to reduce the number of places where fd,{r,w}deadline is checked and updated in preparation for issue 4434. In doing so the deadline logic is simplified by letting the pollster return errTimeout from netFD.Wait{Read,Write} as part of the wakeup logic.
The behaviour of setting n = 0 has been restored to match rev 2a55e349097f, which was the previous change to fd_unix.go before CL 6851096.
Hello jsing@google.com, bradfitz@golang.org, mikioh.mikioh@gmail.com, rsc@golang.org (cc: fullung@gmail.com, golang-dev@googlegroups.com), I'd like you to review this change ...
11 years, 5 months ago
(2012-11-27 09:18:24 UTC)
#1
I would like to do this as two separate CLs. I'd also like to get ...
11 years, 5 months ago
(2012-11-27 09:52:29 UTC)
#3
I would like to do this as two separate CLs. I'd also like to get some feedback
from jsing, as this CL replaces his, and hopefully addresses his concerns.
Although the goal is to make no change, I would like to see it passing on the
builders before replacing the rdeadline,wdeadline fields with atomic accessors.
That latter change will be pretty straight forward now there are less places
where we set and check the deadlines.
https://codereview.appspot.com/6850110/diff/6003/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):
https://codereview.appspot.com/6850110/diff/6003/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:40: // owned by client
On 2012/11/27 09:42:07, bradfitz wrote:
> in one of these rounds I'd like to see this whole section cleaned up and
> documented. e.g. ownership, locking rules, what does rdeadline <0, 0, >0 mean
> (and that they're unix nanos), etc.
Will do in a followup CL
It passes on openbsd, yay. https://codereview.appspot.com/6850110/diff/6003/src/pkg/net/fd_unix.go File src/pkg/net/fd_unix.go (right): https://codereview.appspot.com/6850110/diff/6003/src/pkg/net/fd_unix.go#newcode435 src/pkg/net/fd_unix.go:435: if n == 0 ...
11 years, 5 months ago
(2012-11-27 12:34:36 UTC)
#4
Thank you for your comments. I am encouraged to hear OpenBSD is passing. PTAL. https://codereview.appspot.com/6850110/diff/6003/src/pkg/net/fd_unix.go ...
11 years, 5 months ago
(2012-11-27 12:48:58 UTC)
#5
LGTM. This is the less intrusive change, although more expensive in terms of consecutive calls ...
11 years, 5 months ago
(2012-11-27 16:01:55 UTC)
#7
LGTM.
This is the less intrusive change, although more expensive in terms of
consecutive calls to read/write after the deadline has passed - I'm not
convinced this matters though. I still think there may be benefit in factoring
out the "check deadline" code, which is currently sprinkled in multiple places.
FTR this makes the tests pass again on a machine similar to the OpenBSD/amd64
builder and fixes the failure of the TestReadDeadlineConsecutive test provided
in https://codereview.appspot.com/6842101/.
LGTM On Wed, Nov 28, 2012 at 12:26 AM, <rsc@golang.org> wrote: > https://codereview.appspot.com/6850110/diff/11002/src/pkg/net/fd_unix.go#newcode435 > src/pkg/net/fd_unix.go:435: ...
11 years, 4 months ago
(2012-11-27 21:23:18 UTC)
#8
LGTM
On Wed, Nov 28, 2012 at 12:26 AM, <rsc@golang.org> wrote:
>
https://codereview.appspot.com/6850110/diff/11002/src/pkg/net/fd_unix.go#newc...
> src/pkg/net/fd_unix.go:435: if n == 0 && err == nil && fd.sotype ==
> syscall.SOCK_STREAM {
> Please change this back to != SOCK_DGRAM.
> This is mishandling SOCK_SEQPACKET.
Ah, I forgot it. I'm fine with except SOCK_DGRAM and SOCK_RAW.
On Tue, Nov 27, 2012 at 9:48 PM, <dave@cheney.net> wrote:
> Done, please note the TODO on line 493.
n, oobn = 0, 0 at L494? I guess it works for ReadMsgUnix on
stream-type UnixConn.
(not tested yet, need more test cases...)
PTAL. I'm leaving this code to stress test for a few hours, then will submit ...
11 years, 4 months ago
(2012-11-27 21:35:16 UTC)
#9
PTAL. I'm leaving this code to stress test for a few hours, then will submit if
nothing bad falls out.
@jsing - i believe that in the case where Read is called after the deadline has
passed the cost is about the same as the original code in rev 2a55e349097f.
Previously a call to netFD.Read would have executed syscall.Read
unconditionally, then gone into WaitRead, waited for the lock, gone into AddFD,
and called time.Now before detecting the deadline had passed. Now the check of
the deadline is done at the top of the loop. If the deadline passes after the
check at the top, but before the non blocking syscall.Read, then the original
logic applies.
On my machine time.Now() is ~ 26ns, but may be higher on the BSDs that don't
have a fast syscall mechanism.
@ mikioh - yes, I intend to write test cases for the lesser known Read{Msg,From}
variations to resolve that TODO.
https://codereview.appspot.com/6850110/diff/11002/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):
https://codereview.appspot.com/6850110/diff/11002/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:435: if n == 0 && err == nil && fd.sotype ==
syscall.SOCK_STREAM {
On 2012/11/27 15:26:58, rsc wrote:
> Please change this back to != SOCK_DGRAM.
> This is mishandling SOCK_SEQPACKET.
Done.
https://codereview.appspot.com/6850110/diff/11002/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:469: if n == 0 && err == nil && fd.sotype ==
syscall.SOCK_STREAM {
On 2012/11/27 15:26:58, rsc wrote:
> Same.
Done.
https://codereview.appspot.com/6850110/diff/11002/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:503: if n == 0 && err == nil && fd.sotype ==
syscall.SOCK_STREAM {
On 2012/11/27 15:26:58, rsc wrote:
> Same.
Done.
On Wed, Nov 28, 2012 at 8:48 AM, <dave@cheney.net> wrote: > Hopefully this is the ...
11 years, 4 months ago
(2012-11-28 00:06:55 UTC)
#12
On Wed, Nov 28, 2012 at 8:48 AM, <dave@cheney.net> wrote:
> Hopefully this is the final tweak. As the logic of that check was
> getting large, I have moved it into a helper. PTAL.
Thanks.
*** Submitted as http://code.google.com/p/go/source/detail?r=1289e67dec66 *** net: move deadline logic into pollServer Update issue 4434. The ...
11 years, 4 months ago
(2012-11-28 00:29:48 UTC)
#14
*** Submitted as http://code.google.com/p/go/source/detail?r=1289e67dec66 ***
net: move deadline logic into pollServer
Update issue 4434.
The proposal attempts to reduce the number of places where fd,{r,w}deadline is
checked and updated in preparation for issue 4434. In doing so the deadline
logic is simplified by letting the pollster return errTimeout from
netFD.Wait{Read,Write} as part of the wakeup logic.
The behaviour of setting n = 0 has been restored to match rev 2a55e349097f,
which was the previous change to fd_unix.go before CL 6851096.
R=jsing, bradfitz, mikioh.mikioh, rsc
CC=fullung, golang-dev
http://codereview.appspot.com/6850110
Issue 6850110: code review 6850110: net: move deadline logic into pollServer
(Closed)
Created 11 years, 5 months ago by dave_cheney.net
Modified 11 years, 4 months ago
Reviewers:
Base URL:
Comments: 20