code review 12409044: net: use SetFileCompletionNotificationModes on windows ...

net: use SetFileCompletionNotificationModes on windows if available
This allows to skip GetQueuedCompletionStatus if an IO operation completes synchronously.
benchmark                    old ns/op    new ns/op    delta
BenchmarkTCP4Persistent          27669        25863   -6.53%
BenchmarkTCP4Persistent-2        18173        15908  -12.46%
BenchmarkTCP4Persistent-4        10390         9766   -6.01%

[dvyukov, 2013-08-03 12:15:43]

Hello golang-dev@googlegroups.com,

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

[mikio, 2013-08-03 13:18:00]

On Sat, Aug 3, 2013 at 9:15 PM,  <dvyukov@google.com> wrote:

> This allows to skip GQCS if an IO operation completes instantly.

GQCS stands for GetQueuedCompletionStatus?

[dvyukov, 2013-08-03 13:24:13]

Yes
On Aug 3, 2013 5:18 PM, "Mikio Hara" <mikioh.mikioh@gmail.com> wrote:

> On Sat, Aug 3, 2013 at 9:15 PM,  <dvyukov@google.com> wrote:
>
> > This allows to skip GQCS if an IO operation completes instantly.
>
> GQCS stands for GetQueuedCompletionStatus?
>

[dvyukov, 2013-08-03 16:29:59]

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

Please take another look.

[brainman, 2013-08-04 02:48:08]

Speed up is nice, but I am concerned with validity of this change.

Alex

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

https://codereview.appspot.com/12409044/diff/13001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:295: syscall.SetFileCompletionNotificationModes(fd,
Shouldn't you check for error here?

https://codereview.appspot.com/12409044/diff/13001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:296:
syscall.FILE_SKIP_COMPLETION_PORT_ON_SUCCESS|syscall.FILE_SKIP_SET_EVENT_ON_HANDLE)
I am not convinced if this facility can be used for sockets. From doco:

"... When the FileHandle parameter is a socket, this mode is only compatible
with Layered Service Providers (LSP) that return Installable File Systems (IFS)
handles. To detect whether a non-IFS LSP is installed, use the WSAEnumProtocols
function and examine the dwServiceFlag1 member in each returned WSAPROTOCOL_INFO
structure. If the XP1_IFS_HANDLES (0x20000) bit is cleared then the specified
LSP is not an IFS LSP. Vendors that have non-IFS LSPs are encouraged to migrate
to the Windows Filtering Platform (WFP). ..."

Do you have any proof that this can be used with sockets? Shouldn't you go
through process as described above, before you make decision about using this
facility here?

[dvyukov, 2013-08-04 10:02:08]

On 2013/08/04 02:48:08, brainman wrote:
> Speed up is nice, but I am concerned with validity of this change.
> 
> Alex
> 
> https://codereview.appspot.com/12409044/diff/13001/src/pkg/net/fd_windows.go
> File src/pkg/net/fd_windows.go (right):
> 
>
https://codereview.appspot.com/12409044/diff/13001/src/pkg/net/fd_windows.go#...
> src/pkg/net/fd_windows.go:295: syscall.SetFileCompletionNotificationModes(fd,
> Shouldn't you check for error here?
> 
>
https://codereview.appspot.com/12409044/diff/13001/src/pkg/net/fd_windows.go#...
> src/pkg/net/fd_windows.go:296:
>
syscall.FILE_SKIP_COMPLETION_PORT_ON_SUCCESS|syscall.FILE_SKIP_SET_EVENT_ON_HANDLE)
> I am not convinced if this facility can be used for sockets. From doco:
> 
> "... When the FileHandle parameter is a socket, this mode is only compatible
> with Layered Service Providers (LSP) that return Installable File Systems
(IFS)
> handles. To detect whether a non-IFS LSP is installed, use the
WSAEnumProtocols
> function and examine the dwServiceFlag1 member in each returned
WSAPROTOCOL_INFO
> structure. If the XP1_IFS_HANDLES (0x20000) bit is cleared then the specified
> LSP is not an IFS LSP. Vendors that have non-IFS LSPs are encouraged to
migrate
> to the Windows Filtering Platform (WFP). ..."
> 
> Do you have any proof that this can be used with sockets? Shouldn't you go
> through process as described above, before you make decision about using this
> facility here?


Thanks for double checking me.
I am pretty sure that it is exactly intended for sockets.
But you are right about possible problems. I see issues reported for UDP and non
IFS providers.
I will enable this only for TCP, check for non-IFS providers and check return
codes.
I will make the change more complicated, but I am sure it's worth it. It does
not just reduce overheads, it also helps with scalability on large multicore
systems and can significantly reduce latency. I am in complete perplexity why
they even introduced that senseless completion notifications in the first
place...

[dvyukov, 2013-08-04 15:58:41]

Hello golang-dev@googlegroups.com, mikioh.mikioh@gmail.com,
alex.brainman@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[dvyukov, 2013-08-04 16:01:29]

On Sun, Aug 4, 2013 at 2:02 PM,  <dvyukov@google.com> wrote:
> On 2013/08/04 02:48:08, brainman wrote:
>>
>> Speed up is nice, but I am concerned with validity of this change.
>
>
>> Alex
>
>
>
> https://codereview.appspot.com/12409044/diff/13001/src/pkg/net/fd_windows.go
>>
>> File src/pkg/net/fd_windows.go (right):
>
>
>
>
https://codereview.appspot.com/12409044/diff/13001/src/pkg/net/fd_windows.go#...
>>
>> src/pkg/net/fd_windows.go:295:
>
> syscall.SetFileCompletionNotificationModes(fd,
>>
>> Shouldn't you check for error here?
>
>
>
>
https://codereview.appspot.com/12409044/diff/13001/src/pkg/net/fd_windows.go#...
>>
>> src/pkg/net/fd_windows.go:296:
>
>
>
syscall.FILE_SKIP_COMPLETION_PORT_ON_SUCCESS|syscall.FILE_SKIP_SET_EVENT_ON_HANDLE)
>>
>> I am not convinced if this facility can be used for sockets. From
>
> doco:
>
>> "... When the FileHandle parameter is a socket, this mode is only
>
> compatible
>>
>> with Layered Service Providers (LSP) that return Installable File
>
> Systems (IFS)
>>
>> handles. To detect whether a non-IFS LSP is installed, use the
>
> WSAEnumProtocols
>>
>> function and examine the dwServiceFlag1 member in each returned
>
> WSAPROTOCOL_INFO
>>
>> structure. If the XP1_IFS_HANDLES (0x20000) bit is cleared then the
>
> specified
>>
>> LSP is not an IFS LSP. Vendors that have non-IFS LSPs are encouraged
>
> to migrate
>>
>> to the Windows Filtering Platform (WFP). ..."
>
>
>> Do you have any proof that this can be used with sockets? Shouldn't
>
> you go
>>
>> through process as described above, before you make decision about
>
> using this
>>
>> facility here?
>
>
>
> Thanks for double checking me.
> I am pretty sure that it is exactly intended for sockets.
> But you are right about possible problems. I see issues reported for UDP
> and non IFS providers.
> I will enable this only for TCP, check for non-IFS providers and check
> return codes.
> I will make the change more complicated, but I am sure it's worth it. It
> does not just reduce overheads, it also helps with scalability on large
> multicore systems and can significantly reduce latency. I am in complete
> perplexity why they even introduced that senseless completion
> notifications in the first place...


Done
PTAL

[brainman, 2013-08-05 00:55:02]

It looks reasonable, but lets land https://codereview.appspot.com/12413043/
first. Please, ping me when ready. Thank you.

Alex

[dvyukov, 2013-08-06 10:58:48]

On 2013/08/05 00:55:02, brainman wrote:
> It looks reasonable, but lets land https://codereview.appspot.com/12413043/
> first. Please, ping me when ready. Thank you.


12413043 is landed
I've updated this CL
PTAL

[brainman, 2013-08-07 06:22:34]

Here https://code.google.com/p/go/issues/detail?id=6063#c4 are all my
suggestions incorporated and more. But I will try to understand why
net/http.TestChunkReaderAllocs fails before we proceed.

Alex

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

https://codereview.appspot.com/12409044/diff/29001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:49: if hasLoadSetFileCompletionNotificationModes {
I don't know why, but now net/test fails:

C:\go\root\src>go test net/http
--- FAIL: TestChunkReaderAllocs (0.02 seconds)
        chunked_test.go:67: 14 mallocs; want <= 1

that is on system where hasLoadSetFileCompletionNotificationModes is false.

https://codereview.appspot.com/12409044/diff/29001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:54: var buf [32]syscall.WSAProtocolInfo
This buffer will be quite large. You can ask windows for exact size instead.

https://codereview.appspot.com/12409044/diff/29001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:290: if skipSyncNotif && net == "tcp" {
s/net/fd.net/

https://codereview.appspot.com/12409044/diff/29001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:293: err :=
syscall.SetFileCompletionNotificationModes(sysfd, flags)
s/sysfd/fd.sysfd/

https://codereview.appspot.com/12409044/diff/29001/src/pkg/syscall/syscall_wi...
File src/pkg/syscall/syscall_windows.go (right):

https://codereview.appspot.com/12409044/diff/29001/src/pkg/syscall/syscall_wi...
src/pkg/syscall/syscall_windows.go:549: //sys	WSAEnumProtocols(protocols *int32,
protocolBuffer *WSAProtocolInfo, bufferLength *uint32) (n int32, err error)
[failretval==-1] = ws2_32.WSAEnumProtocolsW
s/-1/socket_error/

for consistency.

[brainman, 2013-08-07 06:58:41]

On 2013/08/07 06:22:34, brainman wrote:
> Here https://code.google.com/p/go/issues/detail?id=6063#c4 are ...

It does not compile. Try https://code.google.com/p/go/issues/detail?id=6063#c5
instead.

Alex

[dvyukov, 2013-08-07 20:48:10]

PTAL

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

https://codereview.appspot.com/12409044/diff/29001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:49: if hasLoadSetFileCompletionNotificationModes {
On 2013/08/07 06:22:34, brainman wrote:
> I don't know why, but now net/test fails:
> 
> C:\go\root\src>go test net/http
> --- FAIL: TestChunkReaderAllocs (0.02 seconds)
>         chunked_test.go:67: 14 mallocs; want <= 1
> 
> that is on system where hasLoadSetFileCompletionNotificationModes is false.

if hasLoadSetFileCompletionNotificationModes false, nothing must be affected,
right? Are you sure it reliably passed w/o this patch?

https://codereview.appspot.com/12409044/diff/29001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:54: var buf [32]syscall.WSAProtocolInfo
On 2013/08/07 06:22:34, brainman wrote:
> This buffer will be quite large. You can ask windows for exact size instead.

This happens only during startup, so I don't care how much memory it uses.
Detecting exact size will require more complex code. And I doubt that more than
32 windows drivers can correctly work together :)

https://codereview.appspot.com/12409044/diff/29001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:290: if skipSyncNotif && net == "tcp" {
On 2013/08/07 06:22:34, brainman wrote:
> s/net/fd.net/

Done.

https://codereview.appspot.com/12409044/diff/29001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:293: err :=
syscall.SetFileCompletionNotificationModes(sysfd, flags)
On 2013/08/07 06:22:34, brainman wrote:
> s/sysfd/fd.sysfd/

Done.

[brainman, 2013-08-07 23:18:44]

On 2013/08/07 20:48:10, dvyukov wrote:
> ...
> if hasLoadSetFileCompletionNotificationModes false, nothing must be affected,
> right? ...

Yes, that sounds reasonable.

> ... Are you sure it reliably passed w/o this patch?

I am never sure of anything, but seems that way. Perhaps the error I see is
unrelated, but I would like to try and find the reason for the failure before we
move on. I will look at your latest changes once I settle with this.

Alex

[brainman, 2013-08-08 01:27:30]

On 2013/08/07 23:18:44, brainman wrote:
> 
> > ... Are you sure it reliably passed w/o this patch?
> 

Yes, it fails without this patch. I created
https://code.google.com/p/go/issues/detail?id=6076.

Alex

[brainman, 2013-08-08 06:40:00]

LGTM

Alex

[dvyukov, 2013-08-08 13:37:00]

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

net: use SetFileCompletionNotificationModes on windows if available
This allows to skip GetQueuedCompletionStatus if an IO operation completes
synchronously.
benchmark                    old ns/op    new ns/op    delta
BenchmarkTCP4Persistent          27669        25863   -6.53%
BenchmarkTCP4Persistent-2        18173        15908  -12.46%
BenchmarkTCP4Persistent-4        10390         9766   -6.01%

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