code review 12413043: net: reduce number of memory allocations during IO oper...

net: reduce number of memory allocations during IO operations
Embed all data necessary for read/write operations directly into netFD.

benchmark                    old ns/op    new ns/op    delta
BenchmarkTCP4Persistent          27669        23341  -15.64%
BenchmarkTCP4Persistent-2        18173        12558  -30.90%
BenchmarkTCP4Persistent-4        10390         7319  -29.56%

This change will intentionally break all builders to see
how many allocations they do per read/write.
This will be fixed soon afterwards.

[dvyukov, 2013-08-03 16:11:20]

Hello golang-dev@googlegroups.com,

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

[brainman, 2013-08-04 03:31:42]

Please add a test.

Alex

[dvyukov, 2013-08-04 10:36:13]

On 2013/08/04 03:31:42, brainman wrote:
> Please add a test.


Done
PTAL

[brainman, 2013-08-05 00:48:29]

LGTM

if you agree with my suggestions.

It is much simpler now, thank you very much.

Alex

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

https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:88: rsa    [2]syscall.RawSockaddrAny
These are quite large (112 each on windows-386). And aren't used for most common
tcp connection. Maybe allocate these separately: alloc and dealloc them in
accept, alloc and keep them forever in ReadFrom.

https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:109: submit func(o *operation) error
Document that submit == nil means "to cancel".

https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:129: // ExecIO executes a single IO operation oi. It
submits and cancels
s/oi/o/

https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:291: o := &fd.op[1]
Index values (1 and 0) are bit cryptic. Maybe create a const? Up to you.

https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/tcp_test.go
File src/pkg/net/tcp_test.go (right):

https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/tcp_test.go#ne...
src/pkg/net/tcp_test.go:299: func TestTCPReadWriteMallocs(t *testing.T) {
I think it is good idea to add test with timeouts. As you know, timeouts
execution path is quite different.

[brainman, 2013-08-05 01:59:21]

One more.

Alex

https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/tcp_test.go
File src/pkg/net/tcp_test.go (right):

https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/tcp_test.go#ne...
src/pkg/net/tcp_test.go:301: switch runtime.GOOS {
I think we should just run it for every OS. This way we will see which builders
are broken. Then we could create an issue an put temp "Skip" in here.

[dvyukov, 2013-08-06 10:26:36]

On 2013/08/05 00:48:29, brainman wrote:
> LGTM
> 
> if you agree with my suggestions.
> 
> It is much simpler now, thank you very much.
> 
> Alex
> 
> https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/fd_windows.go
> File src/pkg/net/fd_windows.go (right):
> 
>
https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/fd_windows.go#...
> src/pkg/net/fd_windows.go:88: rsa    [2]syscall.RawSockaddrAny
> These are quite large (112 each on windows-386). And aren't used for most
common
> tcp connection. Maybe allocate these separately: alloc and dealloc them in
> accept, alloc and keep them forever in ReadFrom.

Makes sense. Done.


>
https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/fd_windows.go#...
> src/pkg/net/fd_windows.go:109: submit func(o *operation) error
> Document that submit == nil means "to cancel".

Done.

 
>
https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/fd_windows.go#...
> src/pkg/net/fd_windows.go:129: // ExecIO executes a single IO operation oi. It
> submits and cancels
> s/oi/o/

Done.

>
https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/fd_windows.go#...
> src/pkg/net/fd_windows.go:291: o := &fd.op[1]
> Index values (1 and 0) are bit cryptic. Maybe create a const? Up to you.

Initially I though that maybe I will need to use for loop over the two
operations. It turned out to be not the case. So I just renamed them to rop and
wop. Indeed much better now.

> https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/tcp_test.go
> File src/pkg/net/tcp_test.go (right):
> 
>
https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/tcp_test.go#ne...
> src/pkg/net/tcp_test.go:299: func TestTCPReadWriteMallocs(t *testing.T) {
> I think it is good idea to add test with timeouts. As you know, timeouts
> execution path is quite different.

Do you mean the case when timeouts actually fire? I think it's not important for
performance. That's kind of corner case that usually happens at most once per
connection.

[dvyukov, 2013-08-06 10:27:03]

On 2013/08/05 01:59:21, brainman wrote:
> One more.
> 
> Alex
> 
> https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/tcp_test.go
> File src/pkg/net/tcp_test.go (right):
> 
>
https://codereview.appspot.com/12413043/diff/19001/src/pkg/net/tcp_test.go#ne...
> src/pkg/net/tcp_test.go:301: switch runtime.GOOS {
> I think we should just run it for every OS. This way we will see which
builders
> are broken. Then we could create an issue an put temp "Skip" in here.

Will do.

[dvyukov, 2013-08-06 10:40:33]

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

net: reduce number of memory allocations during IO operations
Embed all data necessary for read/write operations directly into netFD.

benchmark                    old ns/op    new ns/op    delta
BenchmarkTCP4Persistent          27669        23341  -15.64%
BenchmarkTCP4Persistent-2        18173        12558  -30.90%
BenchmarkTCP4Persistent-4        10390         7319  -29.56%

This change will intentionally break all builders to see
how many allocations they do per read/write.
This will be fixed soon afterwards.

R=golang-dev, alex.brainman
CC=golang-dev
https://codereview.appspot.com/12413043

[brainman, 2013-08-07 02:34:56]

I just noticed that this is broken on my Windows XP pc (windows-386).

https://code.google.com/p/go/issues/detail?id=6063

Surprisingly, both windows builders are fine. It must be one of those "syscall
does not exist here, so I am using different logic" scenarios.

Alex