code review 6845119: net: remove unnecessary Close contention.

net: remove unnecessary Close contention.

Contention profile in BenchmarkTCPOneShot (Core 2 Quad):

Before
Total: 80.285 seconds
44.743  55.7%  55.7%   44.743  55.7% runtime.chanrecv1
31.995  39.9%  95.6%   31.995  39.9% sync.(*Mutex).Lock
 3.547   4.4% 100.0%    3.547   4.4% runtime.chansend1

After
Total: 48.341 seconds
45.810  94.8%  94.8%   45.810  94.8% runtime.chanrecv1
 2.530   5.2% 100.0%    2.530   5.2% runtime.chansend1
 0.001   0.0% 100.0%    0.001   0.0% sync.(*Mutex).Lock

[remyoudompheng, 2012-11-30 20:08:32]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dave_cheney.net, 2012-11-30 20:13:41]

Can you post the before and after svg's ?

https://codereview.appspot.com/6845119/diff/5001/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/6845119/diff/5001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:379: fd.pollServer.Unlock()
This looks like an unrelated change.

I'm not saying it is not a good idea, about a year ago I proposed a change which
removed defer from a few parts of net to reduce their allocation load. At the
time the decision was to fix the compiler/runtime rather than the code. I'm
happy to see this revisited.

[remyoudompheng, 2012-11-30 20:57:36]

https://codereview.appspot.com/6845119/diff/5001/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/6845119/diff/5001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:379: fd.pollServer.Unlock()
On 2012/11/30 20:13:41, dfc wrote:
> This looks like an unrelated change.
> 
> I'm not saying it is not a good idea, about a year ago I proposed a change
which
> removed defer from a few parts of net to reduce their allocation load. At the
> time the decision was to fix the compiler/runtime rather than the code. I'm
> happy to see this revisited.

The goal is not to remove the defer, but to move fd.decref() out of the
pollserver lock. fd.decref() calls the close syscall and we don't to lock the
pollserver during that.

[remyoudompheng, 2012-11-30 21:05:54]

On 2012/11/30 20:57:36, remyoudompheng wrote:
> https://codereview.appspot.com/6845119/diff/5001/src/pkg/net/fd_unix.go
> File src/pkg/net/fd_unix.go (right):
> 
>
https://codereview.appspot.com/6845119/diff/5001/src/pkg/net/fd_unix.go#newco...
> src/pkg/net/fd_unix.go:379: fd.pollServer.Unlock()
> On 2012/11/30 20:13:41, dfc wrote:
> > This looks like an unrelated change.
> > 
> > I'm not saying it is not a good idea, about a year ago I proposed a change
> which
> > removed defer from a few parts of net to reduce their allocation load. At
the
> > time the decision was to fix the compiler/runtime rather than the code. I'm
> > happy to see this revisited.
> 
> The goal is not to remove the defer, but to move fd.decref() out of the
> pollserver lock. fd.decref() calls the close syscall and we don't to lock the
> pollserver during that.

By the way, this part of the patch is precisely the one that improves the
contention. The other diff is for the particular case where connect() fails.

[dave_cheney.net, 2012-11-30 21:07:08]

LGTM.

https://codereview.appspot.com/6845119/diff/5001/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/6845119/diff/5001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:377: fd.pollServer.Lock() // needed for both
fd.incref(true) and pollserver.Evict
I wonder if we address the odd comment on line 24-25, the pollserver lock could
be moved further down.

[mikio, 2012-12-01 03:48:18]

https://codereview.appspot.com/6845119/diff/5001/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):

https://codereview.appspot.com/6845119/diff/5001/src/pkg/net/sock_posix.go#ne...
src/pkg/net/sock_posix.go:63: closesocket(s)
probably I'm missing something but I wonder how could we evict 
this netFD from poller's pending list w/o fd.Close()?

[mikio, 2012-12-01 04:12:50]

LGTM

On 2012/12/01 03:48:18, mikio wrote:

> probably I'm missing something but I wonder how could we evict 
> this netFD from poller's pending list w/o fd.Close()?

Never mind, I overlooked netFD doesn't contain underlying os.File
on Unix variants at this point and no fd.Close doesn't matter on 
Windows.

[remyoudompheng, 2012-12-01 08:26:37]

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

net: remove unnecessary Close contention.

Contention profile in BenchmarkTCPOneShot (Core 2 Quad):

Before
Total: 80.285 seconds
44.743  55.7%  55.7%   44.743  55.7% runtime.chanrecv1
31.995  39.9%  95.6%   31.995  39.9% sync.(*Mutex).Lock
 3.547   4.4% 100.0%    3.547   4.4% runtime.chansend1

After
Total: 48.341 seconds
45.810  94.8%  94.8%   45.810  94.8% runtime.chanrecv1
 2.530   5.2% 100.0%    2.530   5.2% runtime.chansend1
 0.001   0.0% 100.0%    0.001   0.0% sync.(*Mutex).Lock

R=golang-dev, dave, mikioh.mikioh
CC=golang-dev
https://codereview.appspot.com/6845119

[remyoudompheng, 2012-12-01 09:25:38]

https://codereview.appspot.com/6845119/diff/7003/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (left):

https://codereview.appspot.com/6845119/diff/7003/src/pkg/net/sock_posix.go#ol...
src/pkg/net/sock_posix.go:64: fd.Close()
This patch of the patch may break windows in mysterious ways because netFD has a
finalizer on Widnows whereas it hasn't on Unix.
Do you confirm?