code review 6866050: net: fix data races on deadline vars

net: fix data races on deadline vars

Fixes issue 4434.

This proposal replaces the previous CL 6855110. Due to issue 599, 64-bit atomic operations should probably be avoided, so use a sync.Mutex instead.

Benchmark comparisons against 025b9d070a85 on linux/386:

CL 6855110:

benchmark                        old ns/op    new ns/op    delta
BenchmarkTCPOneShot                 710024       727409   +2.45%
BenchmarkTCPOneShotTimeout          758178       768620   +1.38%
BenchmarkTCPPersistent              223464       228058   +2.06%
BenchmarkTCPPersistentTimeout       234494       242600   +3.46%

This proposal:

benchmark                        old ns/op    new ns/op    delta
BenchmarkTCPOneShot                 710024       718492   +1.19%
BenchmarkTCPOneShotTimeout          758178       748783   -1.24%
BenchmarkTCPPersistent              223464       227628   +1.86%
BenchmarkTCPPersistentTimeout       234494       238321   +1.63%

[dfc, 2012-12-04 11:46:26]

Hello rsc@golang.org, dvyukov@google.com, mikioh.mikioh@gmail.com,
alex.brainman@gmail.com (cc: golang-dev@googlegroups.com,
remyoudompheng@gmail.com),

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2012-12-04 14:41:34]

LGTM

https://codereview.appspot.com/6866050/diff/7007/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

https://codereview.appspot.com/6866050/diff/7007/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:299: // TODO(dfc,brainman) when we get a windows race
builder, revisit this.
or err on the other side: make this code the same as POSIX for now, and see if
it could be made faster later.

either way.

[dvyukov, 2012-12-04 14:48:39]

https://codereview.appspot.com/6866050/diff/7007/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

https://codereview.appspot.com/6866050/diff/7007/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:299: // TODO(dfc,brainman) when we get a windows race
builder, revisit this.
On 2012/12/04 14:41:34, bradfitz wrote:
> or err on the other side: make this code the same as POSIX for now, and see if
> it could be made faster later.
> 
> either way.

Note that it's not about our race builders. It's about user programs. It's very
unpleasant when all your bots are read because of a bug in std lib that can be
fixed only in several months.

[dfc, 2012-12-05 02:00:04]

https://codereview.appspot.com/6866050/diff/7007/src/pkg/net/fd_windows.go#ne...
> src/pkg/net/fd_windows.go:299: // TODO(dfc,brainman) when we get a windows
race
> builder, revisit this.
> or err on the other side: make this code the same as POSIX for now, and see if
> it could be made faster later.

@brainman, if you have no objections, I can move the deadline structure into
net.go (or somewhere else shared by all posix OSs) and share it between
fd_unix.go and fd_windows.go

[dfc, 2012-12-05 02:27:32]

Hello rsc@golang.org, dvyukov@google.com, mikioh.mikioh@gmail.com,
alex.brainman@gmail.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com,
remyoudompheng@gmail.com),

Please take another look.

[dfc, 2012-12-05 02:28:14]

On 2012/12/05 02:27:32, dfc wrote:
> Hello mailto:rsc@golang.org, mailto:dvyukov@google.com,
mailto:mikioh.mikioh@gmail.com,
> mailto:alex.brainman@gmail.com, mailto:bradfitz@golang.org (cc:
mailto:golang-dev@googlegroups.com,
> mailto:remyoudompheng@gmail.com),
> 
> Please take another look.

@mikio / brainman - could you please test on freebsd / windows respectively.

[brainman, 2012-12-05 03:56:16]

LGTM

[dfc, 2012-12-05 03:57:10]

On 2012/12/05 03:56:16, brainman wrote:
> LGTM

Thanks Alex. I'll submit this evening unless there are objections.

[mikio, 2012-12-05 04:27:04]

LGTM

[dfc, 2012-12-05 04:59:18]

*** Submitted as https://code.google.com/p/go/source/detail?r=6e0e4077f488 ***

net: fix data races on deadline vars

Fixes issue 4434.

This proposal replaces the previous CL 6855110. Due to issue 599, 64-bit atomic
operations should probably be avoided, so use a sync.Mutex instead.

Benchmark comparisons against 025b9d070a85 on linux/386:

CL 6855110:

benchmark                        old ns/op    new ns/op    delta
BenchmarkTCPOneShot                 710024       727409   +2.45%
BenchmarkTCPOneShotTimeout          758178       768620   +1.38%
BenchmarkTCPPersistent              223464       228058   +2.06%
BenchmarkTCPPersistentTimeout       234494       242600   +3.46%

This proposal:

benchmark                        old ns/op    new ns/op    delta
BenchmarkTCPOneShot                 710024       718492   +1.19%
BenchmarkTCPOneShotTimeout          758178       748783   -1.24%
BenchmarkTCPPersistent              223464       227628   +1.86%
BenchmarkTCPPersistentTimeout       234494       238321   +1.63%

R=rsc, dvyukov, mikioh.mikioh, alex.brainman, bradfitz
CC=golang-dev, remyoudompheng
https://codereview.appspot.com/6866050

[dfc, 2012-12-05 05:27:48]

Looking good, only one builder failing at the moment,

http://code.google.com/p/go/issues/detail?id=4447