code review 4536076: net: Sendfile for win32.

net: Sendfile for win32.
implement using TransmitFile().

[mattn, 2011-05-23 10:53:36]

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

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

[bsiegert, 2011-05-23 13:41:30]

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:117: var bytesToWrite uint32
var n uint32

[bradfitz, 2011-05-23 16:10:51]

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:108: _, e := Seek(infd, *offset, 0)
this Seek has the same race problem that Windows had with "ReadAt" (pread):
Sendfile with an offset shouldn't change the kernel's offset.

Which means this code needs the same locking as pkg/os/file_windows.go's pread
function.

I'm glad this ended up so simple (the TransmitFile call is very close to what we
need), but I'm wondering if Sendfile on Windows should be implemented at a
different layer, up in net/tcpsock_win.go instead.

Let's hold on this CL until I've finished implementing sendfile for posix and
I'll keep Windows in mind while I do it, leaving you a good place to move this
code.

[brainman, 2011-05-24 00:29:22]

Sorry, quite a few comments.

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (left):

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:106: func Sendfile(outfd int, infd int,
offset *int64, count int) (written int, errno int) {
Please, move Sendfile to down below, where all other socket related functions
are. Right after WSASendto would be fine.

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:108: _, e := Seek(infd, *offset, 0)
My reading of TransmitFile manual:

"
You can use the lpOverlapped parameter to specify a 64-bit offset within the
file at which to start the file data transfer by setting the Offset and
OffsetHigh member of the OVERLAPPED structure. If lpOverlapped is a NULL
pointer, the transmission of data always starts at the current byte offset in
the file.
"

is you don't need Seek if you employ Overlapped.

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:121: bytesToWrite = uint32(int64(high)<<32 +
int64(low))
bytesToWrite := uint32(count)

Do not need all that too. From the same manual:

"
Set this parameter to zero in order to transmit the entire file.
"

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:123: if TransmitFile(int32(outfd),
int32(infd), bytesToWrite, 0, o, nil, TF_WRITE_BEHIND) == 0 {
There is a race here. "Last error number" could change between TransmitFile
setting it and WSAGetLastError fetching it. Just do:

e := TransmitFile(int32(outfd), int32(infd), bytesToWrite, 0, o, nil,
TF_WRITE_BEHIND)
if e != 0 {
  ...
}
...

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:125: if e != ERROR_IO_PENDING {
I think you have problem here. What happens if e == 0, you would be returning

return 0, 0

Is that really what you want?

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:129: if WSAGetOverlappedResult(int32(infd),
o, &bytesToWrite, false, &flags) == 0 {
As above, do not need WSAGetLastError.

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:129: if WSAGetOverlappedResult(int32(infd),
o, &bytesToWrite, false, &flags) == 0 {
Please, use GetOverlappedResult instead. We use it everywhere else and it works
fine.

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:202: //sys	TransmitFile(s int32, handle
int32, bytesToWrite uint32, bytsPerSend uint32, overlapped *Overlapped,
transmitFileBuf *TransmitFileBuffers, flags uint32) (errno int) =
wsock32.TransmitFile
I hope we will find it in wsock32.dll. From the manual:

"
Note  The function pointer for the TransmitFile function must be obtained at run
time by making a call to the WSAIoctl function with the
SIO_GET_EXTENSION_FUNCTION_POINTER opcode specified. The input buffer passed to
the WSAIoctl function must contain WSAID_TRANSMITFILE, a globally unique
identifier (GUID) whose value identifies the TransmitFile extension function. On
success, the output returned by the WSAIoctl function contains a pointer to the
TransmitFile function. The WSAID_TRANSMITFILE GUID is defined in the Mswsock.h
header file.
"

We better test it before submitting.

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:772: func Dup(fd int) (nfd int, errno int) {
I'm bit concerned about the name of this function. Is it supposed to "emulate"
unix version? If yes, would DUPLICATE_SAME_ACCESS|DUPLICATE_CLOSE_SOURCE provide
required semantics. If no, should it be named something different. Maybe it can
be incorporated as part of Sendfile implementation instead.

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:778: e = DuplicateHandle(p, int32(nfd), p,
&pp, 0, true, DUPLICATE_SAME_ACCESS|DUPLICATE_CLOSE_SOURCE)
This is certainly wrong. Does it actually runs for you?

It should be:

e = DuplicateHandle(p, int32(fd), p, &pp, ...)
if e != 0 {
  return 0, e
}
return int(pp), 0

[mattn, 2011-05-24 01:36:32]

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

Please take another look.

[mattn, 2011-05-24 01:37:45]

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (left):

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:106: func Sendfile(outfd int, infd int,
offset *int64, count int) (written int, errno int) {
On 2011/05/24 00:29:22, brainman wrote:
> Please, move Sendfile to down below, where all other socket related functions
> are. Right after WSASendto would be fine.

Done.

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:108: _, e := Seek(infd, *offset, 0)
bradfiz, brainman.

ok.
It don't need to use overlapped.
I'll wait bradfiz's implementing, But I'll fix below/above points.

thanks.

On 2011/05/23 16:10:51, bradfitz wrote:
> this Seek has the same race problem that Windows had with "ReadAt" (pread):
> Sendfile with an offset shouldn't change the kernel's offset.
> 
> Which means this code needs the same locking as pkg/os/file_windows.go's pread
> function.
> 
> I'm glad this ended up so simple (the TransmitFile call is very close to what
we
> need), but I'm wondering if Sendfile on Windows should be implemented at a
> different layer, up in net/tcpsock_win.go instead.
> 
> Let's hold on this CL until I've finished implementing sendfile for posix and
> I'll keep Windows in mind while I do it, leaving you a good place to move this
> code.

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:117: var bytesToWrite uint32
On 2011/05/23 13:41:30, bsiegert wrote:
> var n uint32

Done.

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:121: bytesToWrite = uint32(int64(high)<<32 +
int64(low))
On 2011/05/24 00:29:22, brainman wrote:
> bytesToWrite := uint32(count)
> 
> Do not need all that too. From the same manual:
> 
> "
> Set this parameter to zero in order to transmit the entire file.
> "

But we need to return 'how many bytes sent'.

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:123: if TransmitFile(int32(outfd),
int32(infd), bytesToWrite, 0, o, nil, TF_WRITE_BEHIND) == 0 {
No, TransmitFile return FALSE if ERROR_IO_PENDING.

On 2011/05/24 00:29:22, brainman wrote:
> There is a race here. "Last error number" could change between TransmitFile
> setting it and WSAGetLastError fetching it. Just do:
> 
> e := TransmitFile(int32(outfd), int32(infd), bytesToWrite, 0, o, nil,
> TF_WRITE_BEHIND)
> if e != 0 {
>   ...
> }
> ...

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:125: if e != ERROR_IO_PENDING {
if TransmitFile return FALSE, WSAGetLastError() should return non-zero. But we
should follow ERROR_IO_PENDING. Because we should return value 'How many bytes
sent'.

On 2011/05/24 00:29:22, brainman wrote:
> I think you have problem here. What happens if e == 0, you would be returning
> 
> return 0, 0
> 
> Is that really what you want?

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:129: if WSAGetOverlappedResult(int32(infd),
o, &bytesToWrite, false, &flags) == 0 {
GetOverlappedResult/TransmitFile return boolean value. I think that sendfile()
should return e as error code meaning errno.

On 2011/05/24 00:29:22, brainman wrote:
> Please, use GetOverlappedResult instead. We use it everywhere else and it
works
> fine.

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:202: //sys	TransmitFile(s int32, handle
int32, bytesToWrite uint32, bytsPerSend uint32, overlapped *Overlapped,
transmitFileBuf *TransmitFileBuffers, flags uint32) (errno int) =
wsock32.TransmitFile
do you mean following code?
https://github.com/mattn/tinytinyhttpd/blob/master/httpd.cxx#L1948

On 2011/05/24 00:29:22, brainman wrote:
> I hope we will find it in wsock32.dll. From the manual:
> 
> "
> Note  The function pointer for the TransmitFile function must be obtained at
run
> time by making a call to the WSAIoctl function with the
> SIO_GET_EXTENSION_FUNCTION_POINTER opcode specified. The input buffer passed
to
> the WSAIoctl function must contain WSAID_TRANSMITFILE, a globally unique
> identifier (GUID) whose value identifies the TransmitFile extension function.
On
> success, the output returned by the WSAIoctl function contains a pointer to
the
> TransmitFile function. The WSAID_TRANSMITFILE GUID is defined in the Mswsock.h
> header file.
> "
> 
> We better test it before submitting.

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:772: func Dup(fd int) (nfd int, errno int) {
ooooops. I'll split to another CL.

On 2011/05/24 00:29:22, brainman wrote:
> I'm bit concerned about the name of this function. Is it supposed to "emulate"
> unix version? If yes, would DUPLICATE_SAME_ACCESS|DUPLICATE_CLOSE_SOURCE
provide
> required semantics. If no, should it be named something different. Maybe it
can
> be incorporated as part of Sendfile implementation instead.

http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:778: e = DuplicateHandle(p, int32(nfd), p,
&pp, 0, true, DUPLICATE_SAME_ACCESS|DUPLICATE_CLOSE_SOURCE)
ok, I'll fix your point in another CL.

On 2011/05/24 00:29:22, brainman wrote:
> This is certainly wrong. Does it actually runs for you?
> 
> It should be:
> 
> e = DuplicateHandle(p, int32(fd), p, &pp, ...)
> if e != 0 {
>   return 0, e
> }
> return int(pp), 0

[bradfitz, 2011-05-24 01:39:59]

Note that if we end up going with something like this latest sendfile
version:

      http://codereview.appspot.com/4538088/

.. then Windows should probably just implement
src/pkg/net/sendfile_windows.go and you can call into syscall.TransmitFile
directly, without trying to emulate Linux's sendfile semantics exactly.

But please continue to hold on this until we figure out Linux.


On Mon, May 23, 2011 at 6:37 PM, <mattn.jp@gmail.com> wrote:

>
>
>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
> File src/pkg/syscall/syscall_windows.go (left):
>
>
>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
> src/pkg/syscall/syscall_windows.go:106: func Sendfile(outfd int, infd
> int, offset *int64, count int) (written int, errno int) {
> On 2011/05/24 00:29:22, brainman wrote:
>
>> Please, move Sendfile to down below, where all other socket related
>>
> functions
>
>> are. Right after WSASendto would be fine.
>>
>
> Done.
>
>
>
>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
> File src/pkg/syscall/syscall_windows.go (right):
>
>
>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
> src/pkg/syscall/syscall_windows.go:108: _, e := Seek(infd, *offset, 0)
> bradfiz, brainman.
>
> ok.
> It don't need to use overlapped.
> I'll wait bradfiz's implementing, But I'll fix below/above points.
>
> thanks.
>
>
> On 2011/05/23 16:10:51, bradfitz wrote:
>
>> this Seek has the same race problem that Windows had with "ReadAt"
>>
> (pread):
>
>> Sendfile with an offset shouldn't change the kernel's offset.
>>
>
>  Which means this code needs the same locking as
>>
> pkg/os/file_windows.go's pread
>
>> function.
>>
>
>  I'm glad this ended up so simple (the TransmitFile call is very close
>>
> to what we
>
>> need), but I'm wondering if Sendfile on Windows should be implemented
>>
> at a
>
>> different layer, up in net/tcpsock_win.go instead.
>>
>
>  Let's hold on this CL until I've finished implementing sendfile for
>>
> posix and
>
>> I'll keep Windows in mind while I do it, leaving you a good place to
>>
> move this
>
>> code.
>>
>
>
>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
> src/pkg/syscall/syscall_windows.go:117: var bytesToWrite uint32
> On 2011/05/23 13:41:30, bsiegert wrote:
>
>> var n uint32
>>
>
> Done.
>
>
>
>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
> src/pkg/syscall/syscall_windows.go:121: bytesToWrite =
> uint32(int64(high)<<32 + int64(low))
> On 2011/05/24 00:29:22, brainman wrote:
>
>> bytesToWrite := uint32(count)
>>
>
>  Do not need all that too. From the same manual:
>>
>
>  "
>> Set this parameter to zero in order to transmit the entire file.
>> "
>>
>
> But we need to return 'how many bytes sent'.
>
>
>
>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
> src/pkg/syscall/syscall_windows.go:123: if TransmitFile(int32(outfd),
> int32(infd), bytesToWrite, 0, o, nil, TF_WRITE_BEHIND) == 0 {
> No, TransmitFile return FALSE if ERROR_IO_PENDING.
>
>
> On 2011/05/24 00:29:22, brainman wrote:
>
>> There is a race here. "Last error number" could change between
>>
> TransmitFile
>
>> setting it and WSAGetLastError fetching it. Just do:
>>
>
>  e := TransmitFile(int32(outfd), int32(infd), bytesToWrite, 0, o, nil,
>> TF_WRITE_BEHIND)
>> if e != 0 {
>>   ...
>> }
>> ...
>>
>
>
>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
> src/pkg/syscall/syscall_windows.go:125: if e != ERROR_IO_PENDING {
> if TransmitFile return FALSE, WSAGetLastError() should return non-zero.
> But we should follow ERROR_IO_PENDING. Because we should return value
> 'How many bytes sent'.
>
>
> On 2011/05/24 00:29:22, brainman wrote:
>
>> I think you have problem here. What happens if e == 0, you would be
>>
> returning
>
>  return 0, 0
>>
>
>  Is that really what you want?
>>
>
>
>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
> src/pkg/syscall/syscall_windows.go:129: if
> WSAGetOverlappedResult(int32(infd), o, &bytesToWrite, false, &flags) ==
> 0 {
> GetOverlappedResult/TransmitFile return boolean value. I think that
> sendfile() should return e as error code meaning errno.
>
>
> On 2011/05/24 00:29:22, brainman wrote:
>
>> Please, use GetOverlappedResult instead. We use it everywhere else and
>>
> it works
>
>> fine.
>>
>
>
>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
> src/pkg/syscall/syscall_windows.go:202: //sys   TransmitFile(s int32,
> handle int32, bytesToWrite uint32, bytsPerSend uint32, overlapped
> *Overlapped, transmitFileBuf *TransmitFileBuffers, flags uint32) (errno
> int) = wsock32.TransmitFile
> do you mean following code?
> https://github.com/mattn/tinytinyhttpd/blob/master/httpd.cxx#L1948
>
>
> On 2011/05/24 00:29:22, brainman wrote:
>
>> I hope we will find it in wsock32.dll. From the manual:
>>
>
>  "
>> Note  The function pointer for the TransmitFile function must be
>>
> obtained at run
>
>> time by making a call to the WSAIoctl function with the
>> SIO_GET_EXTENSION_FUNCTION_POINTER opcode specified. The input buffer
>>
> passed to
>
>> the WSAIoctl function must contain WSAID_TRANSMITFILE, a globally
>>
> unique
>
>> identifier (GUID) whose value identifies the TransmitFile extension
>>
> function. On
>
>> success, the output returned by the WSAIoctl function contains a
>>
> pointer to the
>
>> TransmitFile function. The WSAID_TRANSMITFILE GUID is defined in the
>>
> Mswsock.h
>
>> header file.
>> "
>>
>
>  We better test it before submitting.
>>
>
>
>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
> src/pkg/syscall/syscall_windows.go:772: func Dup(fd int) (nfd int, errno
> int) {
> ooooops. I'll split to another CL.
>
>
> On 2011/05/24 00:29:22, brainman wrote:
>
>> I'm bit concerned about the name of this function. Is it supposed to
>>
> "emulate"
>
>> unix version? If yes, would
>>
> DUPLICATE_SAME_ACCESS|DUPLICATE_CLOSE_SOURCE provide
>
>> required semantics. If no, should it be named something different.
>>
> Maybe it can
>
>> be incorporated as part of Sendfile implementation instead.
>>
>
>
>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
> src/pkg/syscall/syscall_windows.go:778: e = DuplicateHandle(p,
> int32(nfd), p, &pp, 0, true,
> DUPLICATE_SAME_ACCESS|DUPLICATE_CLOSE_SOURCE)
> ok, I'll fix your point in another CL.
>
>
> On 2011/05/24 00:29:22, brainman wrote:
>
>> This is certainly wrong. Does it actually runs for you?
>>
>
>  It should be:
>>
>
>  e = DuplicateHandle(p, int32(fd), p, &pp, ...)
>> if e != 0 {
>>   return 0, e
>> }
>> return int(pp), 0
>>
>
> http://codereview.appspot.com/4536076/
>

[brainman, 2011-05-24 01:45:05]

On 2011/05/24 01:39:59, bradfitz wrote:
> 
> But please continue to hold on this until we figure out Linux.
> 

I agree. Lets see the final result first.

Alex

[bradfitz, 2011-05-25 17:22:39]

Linux sendfile is in:
http://code.google.com/p/go/source/detail?r=535caa895f21

If you guys want to do this for Windows:

* don't worry about emulating Linux's sendfile in your syscall_windows
* modify src/pkg/net/Makefile to add a sendfile_windows.go file, instead of
sendfile_stub.go
* use whatever Windows system calls you need, but remember that do need to
advance the kernel's offset on the source file.


On Mon, May 23, 2011 at 6:48 PM, mattn <mattn.jp@gmail.com> wrote:

> ok, I'll wait your finish. :)
>
>
> On Tuesday, May 24, 2011 10:39:55 AM UTC+9, Brad Fitzpatrick wrote:
>>
>> Note that if we end up going with something like this latest sendfile
>> version:
>>
>>       http://codereview.appspot.com/4538088/
>>
>> .. then Windows should probably just implement
>> src/pkg/net/sendfile_windows.go and you can call into syscall.TransmitFile
>> directly, without trying to emulate Linux's sendfile semantics exactly.
>>
>> But please continue to hold on this until we figure out Linux.
>>
>>
>> On Mon, May 23, 2011 at 6:37 PM, <mattn.jp@gmail.com> wrote:
>>
>>>
>>>
>>>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
>>> File src/pkg/syscall/syscall_windows.go (left):
>>>
>>>
>>>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
>>> src/pkg/syscall/syscall_windows.go:106: func Sendfile(outfd int, infd
>>> int, offset *int64, count int) (written int, errno int) {
>>> On 2011/05/24 00:29:22, brainman wrote:
>>>
>>>> Please, move Sendfile to down below, where all other socket related
>>>>
>>> functions
>>>
>>>> are. Right after WSASendto would be fine.
>>>>
>>>
>>> Done.
>>>
>>>
>>>
>>>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
>>> File src/pkg/syscall/syscall_windows.go (right):
>>>
>>>
>>>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
>>> src/pkg/syscall/syscall_windows.go:108: _, e := Seek(infd, *offset, 0)
>>> bradfiz, brainman.
>>>
>>> ok.
>>> It don't need to use overlapped.
>>> I'll wait bradfiz's implementing, But I'll fix below/above points.
>>>
>>> thanks.
>>>
>>>
>>> On 2011/05/23 16:10:51, bradfitz wrote:
>>>
>>>> this Seek has the same race problem that Windows had with "ReadAt"
>>>>
>>> (pread):
>>>
>>>> Sendfile with an offset shouldn't change the kernel's offset.
>>>>
>>>
>>>  Which means this code needs the same locking as
>>>>
>>> pkg/os/file_windows.go's pread
>>>
>>>> function.
>>>>
>>>
>>>  I'm glad this ended up so simple (the TransmitFile call is very close
>>>>
>>> to what we
>>>
>>>> need), but I'm wondering if Sendfile on Windows should be implemented
>>>>
>>> at a
>>>
>>>> different layer, up in net/tcpsock_win.go instead.
>>>>
>>>
>>>  Let's hold on this CL until I've finished implementing sendfile for
>>>>
>>> posix and
>>>
>>>> I'll keep Windows in mind while I do it, leaving you a good place to
>>>>
>>> move this
>>>
>>>> code.
>>>>
>>>
>>>
>>>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
>>> src/pkg/syscall/syscall_windows.go:117: var bytesToWrite uint32
>>> On 2011/05/23 13:41:30, bsiegert wrote:
>>>
>>>> var n uint32
>>>>
>>>
>>> Done.
>>>
>>>
>>>
>>>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
>>> src/pkg/syscall/syscall_windows.go:121: bytesToWrite =
>>> uint32(int64(high)<<32 + int64(low))
>>> On 2011/05/24 00:29:22, brainman wrote:
>>>
>>>> bytesToWrite := uint32(count)
>>>>
>>>
>>>  Do not need all that too. From the same manual:
>>>>
>>>
>>>  "
>>>> Set this parameter to zero in order to transmit the entire file.
>>>> "
>>>>
>>>
>>> But we need to return 'how many bytes sent'.
>>>
>>>
>>>
>>>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
>>> src/pkg/syscall/syscall_windows.go:123: if TransmitFile(int32(outfd),
>>> int32(infd), bytesToWrite, 0, o, nil, TF_WRITE_BEHIND) == 0 {
>>> No, TransmitFile return FALSE if ERROR_IO_PENDING.
>>>
>>>
>>> On 2011/05/24 00:29:22, brainman wrote:
>>>
>>>> There is a race here. "Last error number" could change between
>>>>
>>> TransmitFile
>>>
>>>> setting it and WSAGetLastError fetching it. Just do:
>>>>
>>>
>>>  e := TransmitFile(int32(outfd), int32(infd), bytesToWrite, 0, o, nil,
>>>> TF_WRITE_BEHIND)
>>>> if e != 0 {
>>>>   ...
>>>> }
>>>> ...
>>>>
>>>
>>>
>>>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
>>> src/pkg/syscall/syscall_windows.go:125: if e != ERROR_IO_PENDING {
>>> if TransmitFile return FALSE, WSAGetLastError() should return non-zero.
>>> But we should follow ERROR_IO_PENDING. Because we should return value
>>> 'How many bytes sent'.
>>>
>>>
>>> On 2011/05/24 00:29:22, brainman wrote:
>>>
>>>> I think you have problem here. What happens if e == 0, you would be
>>>>
>>> returning
>>>
>>>  return 0, 0
>>>>
>>>
>>>  Is that really what you want?
>>>>
>>>
>>>
>>>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
>>> src/pkg/syscall/syscall_windows.go:129: if
>>> WSAGetOverlappedResult(int32(infd), o, &bytesToWrite, false, &flags) ==
>>> 0 {
>>> GetOverlappedResult/TransmitFile return boolean value. I think that
>>> sendfile() should return e as error code meaning errno.
>>>
>>>
>>> On 2011/05/24 00:29:22, brainman wrote:
>>>
>>>> Please, use GetOverlappedResult instead. We use it everywhere else and
>>>>
>>> it works
>>>
>>>> fine.
>>>>
>>>
>>>
>>>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
>>> src/pkg/syscall/syscall_windows.go:202: //sys   TransmitFile(s int32,
>>> handle int32, bytesToWrite uint32, bytsPerSend uint32, overlapped
>>> *Overlapped, transmitFileBuf *TransmitFileBuffers, flags uint32) (errno
>>> int) = wsock32.TransmitFile
>>> do you mean following code?
>>> https://github.com/mattn/tinytinyhttpd/blob/master/httpd.cxx#L1948
>>>
>>>
>>> On 2011/05/24 00:29:22, brainman wrote:
>>>
>>>> I hope we will find it in wsock32.dll. From the manual:
>>>>
>>>
>>>  "
>>>> Note  The function pointer for the TransmitFile function must be
>>>>
>>> obtained at run
>>>
>>>> time by making a call to the WSAIoctl function with the
>>>> SIO_GET_EXTENSION_FUNCTION_POINTER opcode specified. The input buffer
>>>>
>>> passed to
>>>
>>>> the WSAIoctl function must contain WSAID_TRANSMITFILE, a globally
>>>>
>>> unique
>>>
>>>> identifier (GUID) whose value identifies the TransmitFile extension
>>>>
>>> function. On
>>>
>>>> success, the output returned by the WSAIoctl function contains a
>>>>
>>> pointer to the
>>>
>>>> TransmitFile function. The WSAID_TRANSMITFILE GUID is defined in the
>>>>
>>> Mswsock.h
>>>
>>>> header file.
>>>> "
>>>>
>>>
>>>  We better test it before submitting.
>>>>
>>>
>>>
>>>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
>>> src/pkg/syscall/syscall_windows.go:772: func Dup(fd int) (nfd int, errno
>>> int) {
>>> ooooops. I'll split to another CL.
>>>
>>>
>>> On 2011/05/24 00:29:22, brainman wrote:
>>>
>>>> I'm bit concerned about the name of this function. Is it supposed to
>>>>
>>> "emulate"
>>>
>>>> unix version? If yes, would
>>>>
>>> DUPLICATE_SAME_ACCESS|DUPLICATE_CLOSE_SOURCE provide
>>>
>>>> required semantics. If no, should it be named something different.
>>>>
>>> Maybe it can
>>>
>>>> be incorporated as part of Sendfile implementation instead.
>>>>
>>>
>>>
>>>
http://codereview.appspot.com/4536076/diff/2001/src/pkg/syscall/syscall_windo...
>>> src/pkg/syscall/syscall_windows.go:778: e = DuplicateHandle(p,
>>> int32(nfd), p, &pp, 0, true,
>>> DUPLICATE_SAME_ACCESS|DUPLICATE_CLOSE_SOURCE)
>>> ok, I'll fix your point in another CL.
>>>
>>>
>>> On 2011/05/24 00:29:22, brainman wrote:
>>>
>>>> This is certainly wrong. Does it actually runs for you?
>>>>
>>>
>>>  It should be:
>>>>
>>>
>>>  e = DuplicateHandle(p, int32(fd), p, &pp, ...)
>>>> if e != 0 {
>>>>   return 0, e
>>>> }
>>>> return int(pp), 0
>>>>
>>>
>>> http://codereview.appspot.com/4536076/
>>>
>>
>>

[mattn, 2011-05-26 08:33:08]

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

Please take another look.

[bradfitz, 2011-05-26 13:59:28]

http://codereview.appspot.com/4536076/diff/9004/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

http://codereview.appspot.com/4536076/diff/9004/src/pkg/net/fd_windows.go#new...
src/pkg/net/fd_windows.go:112: if rc := (*anOp)(unsafe.Pointer(o)).resultc; rc
!= nil {
I don't know this code. When is this nil? Is this related to this change or
something unrelated you found in the process?

http://codereview.appspot.com/4536076/diff/9004/src/pkg/net/sendfile_windows.go
File src/pkg/net/sendfile_windows.go (right):

http://codereview.appspot.com/4536076/diff/9004/src/pkg/net/sendfile_windows....
src/pkg/net/sendfile_windows.go:13: // sendFile copies the contents of r to c
using the sendfile
using the TransmitFile

http://codereview.appspot.com/4536076/diff/9004/src/pkg/net/sendfile_windows....
src/pkg/net/sendfile_windows.go:52: for {
can you make this loop look a bit more like the linux one and make this "for
remain > 0 {".

then instead of all your return statements just use "break" and use the return
statement that Linux has?

http://codereview.appspot.com/4536076/diff/9004/src/pkg/net/sendfile_windows....
src/pkg/net/sendfile_windows.go:57: return 0, err, true
I don't think so you want "handled = true" here with 0 bytes.

that means the caller won't fall back to the default read/write path

[bradfitz, 2011-05-26 14:00:23]

Any CPU usage numbers before/after?

On Thu, May 26, 2011 at 1:41 AM, mattn <mattn.jp@gmail.com> wrote:

> I did tested CL with https://gist.github.com/992789 .
>
>

[bradfitz, 2011-05-27 00:22:08]

I find poor machines the most fun & rewarding to benchmark on.  :-)

On Thu, May 26, 2011 at 5:20 PM, mattn <mattn.jp@gmail.com> wrote:

> Sorry, currently, my environment is very poor so perhaps I can't get befit
> benchmark results.

[mattn, 2011-05-27 00:37:02]

Thanks for your reviews.

http://codereview.appspot.com/4536076/diff/9004/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

http://codereview.appspot.com/4536076/diff/9004/src/pkg/net/fd_windows.go#new...
src/pkg/net/fd_windows.go:112: if rc := (*anOp)(unsafe.Pointer(o)).resultc; rc
!= nil {
Sometimes, I got 'receive from nil channel' panic.
And I can't understand why original code is working.
o is pointer of OVERLAPPED structure. Why it can cast to pointer of anOp
structure?

On 2011/05/26 13:59:28, bradfitz wrote:
> I don't know this code. When is this nil? Is this related to this change or
> something unrelated you found in the process?

http://codereview.appspot.com/4536076/diff/9004/src/pkg/net/sendfile_windows.go
File src/pkg/net/sendfile_windows.go (right):

http://codereview.appspot.com/4536076/diff/9004/src/pkg/net/sendfile_windows....
src/pkg/net/sendfile_windows.go:13: // sendFile copies the contents of r to c
using the sendfile
On 2011/05/26 13:59:28, bradfitz wrote:
> using the TransmitFile

Done.

http://codereview.appspot.com/4536076/diff/9004/src/pkg/net/sendfile_windows....
src/pkg/net/sendfile_windows.go:52: for {
On 2011/05/26 13:59:28, bradfitz wrote:
> can you make this loop look a bit more like the linux one and make this "for
> remain > 0 {".
> 
> then instead of all your return statements just use "break" and use the return
> statement that Linux has?

Done.

http://codereview.appspot.com/4536076/diff/9004/src/pkg/net/sendfile_windows....
src/pkg/net/sendfile_windows.go:57: return 0, err, true
On 2011/05/26 13:59:28, bradfitz wrote:
> I don't think so you want "handled = true" here with 0 bytes.
> 
> that means the caller won't fall back to the default read/write path

Done.

[brainman, 2011-05-27 03:09:03]

On 2011/05/27 00:37:02, mattn wrote:

> Sometimes, I got 'receive from nil channel' panic.
> And I can't understand why original code is working.
> o is pointer of OVERLAPPED structure. Why it can cast to pointer of anOp
> structure?
> 

First, we create a "completion port", see

resultsrv.iocp, errno = syscall.CreateIoCompletionPort(-1, 0, 0, 1)

in startServer.

Every socket handle we use is associated with that "completion port", see all
the remaining syscall.CreateIoCompletionPort. That means that from that moment
onwards, windows will post an "io completed" message on any io completion to the
resultsrv.iocp.

You can see it here. For example, when reading, we make this call

syscall.WSARecv(uint32(o.fd.sysfd), &o.buf, 1, &d, &f, &o.o, nil)

where we pass o.o to Windows to notify us about completion, which is address of
o in

type anOp struct {
        // Used by IOCP interface, it must be first field
        // of the struct, as our code rely on it.
        o syscall.Overlapped

        resultc chan ioResult // io completion results
        errnoc  chan int      // io submit / cancel operation errors
        fd      *netFD
}

Once io is completed, Windows notify us via syscall.GetQueuedCompletionStatus,
which runs in a different thread (see (*resultSrv).Run), that gets our pointer
(&o.o). We need to get to resultc field, but, unfortunately, 

(*anOp)(unsafe.Pointer(o)).resultc

is the only way.

I suspect, you are seeing nil from syscall.GetQueuedCompletionStatus, because
you do io on sockets still associated with "completion port" and you don't
specify "overlapped" structure properly.

It is not pretty, but that is how Windows "completion ports" work, and these
aren't most complicated api I've seen <g>. Mind you, having goroutines simplify
things a lot!

Alex

[brainman, 2011-06-01 07:37:51]

On 2011/05/27 00:22:08, bradfitz wrote:
> I find poor machines the most fun & rewarding to benchmark on.  :-)
> 

Do you have a benchmark I can use to check this change?

Alex

[bradfitz, 2011-06-01 14:11:40]

On Wed, Jun 1, 2011 at 1:20 AM, mattn <mattn.jp@gmail.com> wrote:

> A "ImageMagick-i686-pc-mingw32.tar" is 127MB file.
>
> C:\temp\sendfile>sendfile-transmitfile.exe ImageMagick-i686-pc-mingw32.tar
> 2011/06/01 17:18:08 Seed: 60.64970078048813 mb/s
>
> C:\temp\sendfile>sendfile-stub.exe ImageMagick-i686-pc-mingw32.tar
> 2011/06/01 17:18:16 Seed: 59.7642813529661 mb/s
>
> C:\temp\sendfile>sendfile-transmitfile.exe ImageMagick-i686-pc-mingw32.tar
> 2011/06/01 17:18:24 Seed: 63.47059820180257 mb/s
>
> C:\temp\sendfile>sendfile-stub.exe ImageMagick-i686-pc-mingw32.tar
> 2011/06/01 17:18:29 Seed: 59.33122352106373 mb/s
>
> C:\temp\sendfile>sendfile-transmitfile.exe ImageMagick-i686-pc-mingw32.tar
> 2011/06/01 17:18:34 Seed: 61.102322161383505 mb/s
>
> C:\temp\sendfile>sendfile-stub.exe ImageMagick-i686-pc-mingw32.tar
> 2011/06/01 17:18:40 Seed: 63.470629691903994 mb/s
>
>
> Hmm, I can't see big effect with my patch. X-(
>

You want to look at CPU usage more than mb/s.

[brainman, 2011-06-02 03:35:16]

On 2011/06/01 14:11:40, bradfitz wrote:
> 
> You want to look at CPU usage more than mb/s.

I tried to use "time" from mingw, but it gives me the same figure every time.
What do you use to measure CPU?

I tried PsTools (http://technet.microsoft.com/en-us/sysinternals/bb896649.aspx)
and that show about 2x speed up with sendfile_....

Alex

[bradfitz, 2011-06-02 04:49:30]

On Wed, Jun 1, 2011 at 8:35 PM, <alex.brainman@gmail.com> wrote:

> On 2011/06/01 14:11:40, bradfitz wrote:
>
>  You want to look at CPU usage more than mb/s.
>>
>
> I tried to use "time" from mingw, but it gives me the same figure every
> time. What do you use to measure CPU?
>

"time" from unix.


> I tried PsTools
> (http://technet.microsoft.com/en-us/sysinternals/bb896649.aspx) and that
> show about 2x speed up with sendfile_....


sounds nice!

looks like the latest CL has some "Done!" comments but no new uploaded
version of the CL.

is it ready for review again?

[brainman, 2011-06-02 05:12:36]

On 2011/06/02 04:49:30, bradfitz wrote:
> 
> looks like the latest CL has some "Done!" comments but no new uploaded
> version of the CL.
> 
> is it ready for review again?

I didn't have chance to look at it properly yet. Sorry.

Brief look, I can see that you didn't implement "timeouts" - you should be able
handle timeouts set by SetTimeout.

Alex

[bradfitz, 2011-06-02 05:14:26]

http://codereview.appspot.com/4536076/diff/6008/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

http://codereview.appspot.com/4536076/diff/6008/src/pkg/net/fd_windows.go#new...
src/pkg/net/fd_windows.go:112: if rc := (*anOp)(unsafe.Pointer(o)).resultc; rc
!= nil {
this part of the change doesn't seem related to TransmitFile / sendfile.

if this part of the change is necessary, it seems like it's only here to cover
up a problem.  but what problem?

I asked why this was here in the previous CL version but there's no change or no
new comment here now.

[mattn, 2011-06-02 05:43:34]

http://codereview.appspot.com/4536076/diff/6008/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

http://codereview.appspot.com/4536076/diff/6008/src/pkg/net/fd_windows.go#new...
src/pkg/net/fd_windows.go:112: if rc := (*anOp)(unsafe.Pointer(o)).resultc; rc
!= nil {
I asked brainman about this.

https://groups.google.com/d/msg/golang-dev/8qKqbiq7oYw/r1M7GghKxfQJ

And I understood that using Overlapped manually don't set resultc, then it skip
nil channel read.
Certainly, you are right, This should be another CL.

brainman, have I mis-understanding?

On 2011/06/02 05:14:26, bradfitz wrote:
> this part of the change doesn't seem related to TransmitFile / sendfile.
> 
> if this part of the change is necessary, it seems like it's only here to cover
> up a problem.  but what problem?
> 
> I asked why this was here in the previous CL version but there's no change or
no
> new comment here now.

[brainman, 2011-06-02 06:03:08]

On 2011/06/02 05:43:34, mattn wrote:
> 
> brainman, have I mis-understanding?
> 

I don't know <g>. I tried to explain it here: http://tinyurl.com/3dghhcx.

All I can add, aee how writeOp and acceptOp are implemented, you should be able
to do something similar.

Alex

[mattn, 2011-06-02 06:32:46]

On 2011/06/02 06:03:08, brainman wrote:
> On 2011/06/02 05:43:34, mattn wrote:
> > 
> > brainman, have I mis-understanding?
> > 
> 
> I don't know <g>. I tried to explain it here: http://tinyurl.com/3dghhcx.
> 
> All I can add, aee how writeOp and acceptOp are implemented, you should be
able
> to do something similar.
> 
> Alex

I don't think why I should add completion port.

sendfile_windows is transfering & waiting. This is blocking function.
Sendfile() should return "how number bytes sent".

[brainman, 2011-06-02 06:39:53]

On 2011/06/02 06:32:46, mattn wrote:
> 
> I don't think why I should add completion port.
> 

It's already done. Your socket is associated with completion port on creation
(see my explanation). If you don't want your particular socket to be assotiated,
well, don't but then you have to redo all other operations, because they will
not work without of completion port. That is how fd_windows.go is implemented at
this moment.

> sendfile_windows is transfering & waiting. This is blocking function.
> Sendfile() should return "how number bytes sent".

(*netFD).Write is blocking too. It does the same. What is your point?

Alex

[mattn, 2011-06-02 06:59:04]

On 2011/06/02 06:39:53, brainman wrote:
> On 2011/06/02 06:32:46, mattn wrote:
> > 
> > I don't think why I should add completion port.
> > 
> 
> It's already done. Your socket is associated with completion port on creation
> (see my explanation). If you don't want your particular socket to be
assotiated,
> well, don't but then you have to redo all other operations, because they will
> not work without of completion port. That is how fd_windows.go is implemented
at
> this moment.
> 
> > sendfile_windows is transfering & waiting. This is blocking function.
> > Sendfile() should return "how number bytes sent".
> 
> (*netFD).Write is blocking too. It does the same. What is your point?
> 
> Alex

Ok, I understood.
BTW, in future, if someone want to create cgo that using socket. and he want to
pass the socket to windows API with Overlapped structure, Will we follow that?

I'll update patch in later.

Thanks.
- Yasuhiro Matsumoto

[brainman, 2011-06-02 07:11:03]

On 2011/06/02 06:59:04, mattn wrote:

> BTW, in future, if someone want to create cgo that using socket. and he want
to
> pass the socket to windows API with Overlapped structure, Will we follow that?
> 

I'm not sure I understand what your problem is. If you're saying that you don't
like how our sockets are setup, because it might break other unrelated code that
uses our socket handle. Well, I suspect, you can DuplicateHandle and do what you
want with it. But there are other things to worry about, like internal locks
that serialize all io, timeouts, what else ...

On the other hand, I'm not saying what we have is best or even good way of doing
socket io. But it is something that works for the moment. If you want to offer
and alternative, you know what to do. :-)

Alex

[mattn, 2011-06-02 08:47:01]

On 2011/06/02 07:11:03, brainman wrote:
> On 2011/06/02 06:59:04, mattn wrote:
> 
> > BTW, in future, if someone want to create cgo that using socket. and he want
> to
> > pass the socket to windows API with Overlapped structure, Will we follow
that?
> > 
> 
> I'm not sure I understand what your problem is. If you're saying that you
don't
> like how our sockets are setup, because it might break other unrelated code
that
> uses our socket handle. Well, I suspect, you can DuplicateHandle and do what
you
> want with it. But there are other things to worry about, like internal locks
> that serialize all io, timeouts, what else ...
> 
> On the other hand, I'm not saying what we have is best or even good way of
doing
> socket io. But it is something that works for the moment. If you want to offer
> and alternative, you know what to do. :-)
> 
> Alex

> you know what to do. :-)

Ah, yes. Then I just uploaded. Please check.

I modified test program also. https://gist.github.com/992789
#accept large large file

This test program use temporary file. Then benchmark result will be worse.

Thanks
- Yasuhiro Matsumoto

[mattn, 2011-06-02 08:55:30]

On 2011/06/02 08:47:01, mattn wrote:
> On 2011/06/02 07:11:03, brainman wrote:
> > On 2011/06/02 06:59:04, mattn wrote:
> > 
> > > BTW, in future, if someone want to create cgo that using socket. and he
want
> > to
> > > pass the socket to windows API with Overlapped structure, Will we follow
> that?
> > > 
> > 
> > I'm not sure I understand what your problem is. If you're saying that you
> don't
> > like how our sockets are setup, because it might break other unrelated code
> that
> > uses our socket handle. Well, I suspect, you can DuplicateHandle and do what
> you
> > want with it. But there are other things to worry about, like internal locks
> > that serialize all io, timeouts, what else ...
> > 
> > On the other hand, I'm not saying what we have is best or even good way of
> doing
> > socket io. But it is something that works for the moment. If you want to
offer
> > and alternative, you know what to do. :-)
> > 
> > Alex
> 
> > you know what to do. :-)
> 
> Ah, yes. Then I just uploaded. Please check.
> 
> I modified test program also. https://gist.github.com/992789
> #accept large large file
> 
> This test program use temporary file. Then benchmark result will be worse.
> 
> Thanks
> - Yasuhiro Matsumoto

Sorry. uploaded again.

[mattn, 2011-06-02 09:10:41]

ChromeOS-Vanilla-...-VirtualBox.vdi is 872MB file.

C:\>sendfile-stub.exe
ChromeOS-Vanilla-0.13.576.2011_05_29_1658-r89cfbacc-VirtualBox.vdi
2011/06/02 18:03:48 Seed: 14.211402193206688 mb/s

C:\>sendfile-transmitfile.exe
ChromeOS-Vanilla-0.13.576.2011_05_29_1658-r89cfbacc-VirtualBox.vdi
2011/06/02 18:06:59 Seed: 20.858243644490038 mb/s

WindowsXP SP3 Intel Core 2 Duo 2.66GHz CPU
1.97GHz 1.96GB RAM

Thanks.
- mattn

[rsc, 2011-06-02 15:49:02]

Thanks for working on this.

http://codereview.appspot.com/4536076/diff/12003/src/pkg/net/sendfile_windows.go
File src/pkg/net/sendfile_windows.go (right):

http://codereview.appspot.com/4536076/diff/12003/src/pkg/net/sendfile_windows...
src/pkg/net/sendfile_windows.go:34: if errno != 0 {
This is pretty deeply nested.
Use return x instead of errno = x; break.
Also check errno == 0 first to drop a level
of indentation:

if errno == 0 {
    return syscall.ERROR_BAD_ARGUMENTS
}
if errno != syscall.ERROR_IO_PENDING {
    return errno
}
...

http://codereview.appspot.com/4536076/diff/12003/src/pkg/net/sendfile_windows...
src/pkg/net/sendfile_windows.go:47: errno = syscall.GetLastError()
Don't the system calls return the last error already?  It was 0.

[mattn, 2011-06-03 00:14:07]

http://codereview.appspot.com/4536076/diff/12003/src/pkg/net/sendfile_windows.go
File src/pkg/net/sendfile_windows.go (right):

http://codereview.appspot.com/4536076/diff/12003/src/pkg/net/sendfile_windows...
src/pkg/net/sendfile_windows.go:34: if errno != 0 {
On 2011/06/02 15:49:02, rsc wrote:
> This is pretty deeply nested.
> Use return x instead of errno = x; break.
> Also check errno == 0 first to drop a level
> of indentation:
> 
> if errno == 0 {
>     return syscall.ERROR_BAD_ARGUMENTS
> }
> if errno != syscall.ERROR_IO_PENDING {
>     return errno
> }
> ...

Done.

http://codereview.appspot.com/4536076/diff/12003/src/pkg/net/sendfile_windows...
src/pkg/net/sendfile_windows.go:47: errno = syscall.GetLastError()
On 2011/06/02 15:49:02, rsc wrote:
> Don't the system calls return the last error already?  It was 0.

Done.
Thanks for good catch.

[brainman, 2011-06-03 07:31:57]

Thank you for doing this.

Alex

http://codereview.appspot.com/4536076/diff/21006/src/pkg/net/sendfile_windows.go
File src/pkg/net/sendfile_windows.go (right):

http://codereview.appspot.com/4536076/diff/21006/src/pkg/net/sendfile_windows...
src/pkg/net/sendfile_windows.go:5: package net
It doesn't need to be so complicated (I didn't tested this code properly):

// Copyright 2011 The Go Authors.  All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package net

import (
	"io"
	"os"
	"syscall"
)

type sendfileOp struct {
	anOp
	src int32 // source
	n   int64 // how many bytes to send (0 means till EOF)
}

func (o *sendfileOp) Submit() (errno int) {
	return syscall.TransmitFile(int32(o.fd.sysfd), o.src, uint32(o.n), 0, &o.o,
nil, 0)
}

func (o *sendfileOp) Name() string {
	return "TransmitFile"
}

// sendFile copies the contents of r to c using the TransmitFile
// system call to minimize copies.
//
// if handled == true, sendFile returns the number of bytes copied and any
// non-EOF error.
//
// if handled == false, sendFile performed no work.
func sendFile(c *netFD, r io.Reader) (written int64, err os.Error, handled bool)
{
	var n int64 = 0 // by default, copy until EOF

	lr, ok := r.(*io.LimitedReader)
	if ok {
		n, r = lr.N, lr.R
		if n <= 0 {
			return 0, nil, true
		}
	}
	f, ok := r.(*os.File)
	if !ok {
		return 0, nil, false
	}

	c.wio.Lock()
	defer c.wio.Unlock()
	c.incref()
	defer c.decref()

	var o sendfileOp
	o.Init(c)
	o.n = n
	o.src = int32(f.Fd())
	done, err := iosrv.ExecIO(&o, 0)
	if lr != nil {
		lr.N -= int64(done)
	}
	return int64(done), err, done > 0
}

http://codereview.appspot.com/4536076/diff/21006/src/pkg/net/sendfile_windows...
src/pkg/net/sendfile_windows.go:33: errno = syscall.TransmitFile(fd, o.src,
uint32(remain), 0, &o.o, nil, syscall.TF_WRITE_BEHIND)
Brad, I take it, we start from "current position" in the file?

http://codereview.appspot.com/4536076/diff/21006/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4536076/diff/21006/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:176: //sys	GetOverlappedResult(handle int32,
overlapped *Overlapped, bytesTransferred *uint32, wait bool) (errno int)
[failretval==-1]
You won't need this function. Same for CreateEvent.

http://codereview.appspot.com/4536076/diff/21006/src/pkg/syscall/ztypes_windo...
File src/pkg/syscall/ztypes_windows_386.go (right):

http://codereview.appspot.com/4536076/diff/21006/src/pkg/syscall/ztypes_windo...
src/pkg/syscall/ztypes_windows_386.go:29: ERROR_DISCARDED           = 157
I don't think you need to change this file. But tell me if you think otherwise.

[mattn, 2011-06-03 11:13:49]

http://codereview.appspot.com/4536076/diff/21006/src/pkg/net/sendfile_windows.go
File src/pkg/net/sendfile_windows.go (right):

http://codereview.appspot.com/4536076/diff/21006/src/pkg/net/sendfile_windows...
src/pkg/net/sendfile_windows.go:5: package net
On 2011/06/03 07:31:57, brainman wrote:
> It doesn't need to be so complicated (I didn't tested this code properly):

Hmm, it can't send large file than 0xffffffff.

[brainman, 2011-06-06 06:52:22]

On 2011/06/03 11:13:49, mattn wrote:
>
http://codereview.appspot.com/4536076/diff/21006/src/pkg/net/sendfile_windows.go
> File src/pkg/net/sendfile_windows.go (right):
> 
>
http://codereview.appspot.com/4536076/diff/21006/src/pkg/net/sendfile_windows...
> src/pkg/net/sendfile_windows.go:5: package net
> On 2011/06/03 07:31:57, brainman wrote:
> > It doesn't need to be so complicated (I didn't tested this code properly):
> 
> Hmm, it can't send large file than 0xffffffff.

I don't know anything about that. Searching about I find these:

http://www.ms-news.net/f3603/transmitfile-file-siez-limit-2067347.html

http://www.mombu.com/microsoft/alt-winsock-programming/t-transmitfile-and-lar...

I don't know what to suggest. I guess, if all else fails we can always go "slow"
way.

Alex

[peterGo, 2011-06-06 13:04:34]

mattn,

On 2011/06/03 11:13:49, mattn wrote:
> Hmm, it can't send large file than 0xffffffff.

TransmitFile Function
http://msdn.microsoft.com/en-us/library/ms740565.aspx

ThenNumberOfBytesToWrite parameter is a DWORD (uint32).

This is an optimization. If the optimization is not applicable, don't optimize.

Peter

[brainman, 2011-06-07 00:30:25]

http://codereview.appspot.com/4536076/diff/21006/src/pkg/net/sendfile_windows.go
File src/pkg/net/sendfile_windows.go (right):

http://codereview.appspot.com/4536076/diff/21006/src/pkg/net/sendfile_windows...
src/pkg/net/sendfile_windows.go:40: rv, errno :=
syscall.WaitForSingleObject(o.o.HEvent, syscall.INFINITE)
I've said it before. I'll say it again. You don't need all that waiting code
here - it is already implemented in fd_windows.go, you just need to use it.

That other code also implements "timeouts", which you don't do here.

If you need to loop, please do it inside sendFile. But according to
http://www.ms-news.net/f3603/transmitfile-file-siez-limit-2067347.html,
TransmitFile will fail for big files on some platforms regardless:

>>>>

... it was not possible to solve this
issue by transmitting smaller portions of the file because
TransmitFile() failed with the same error just given a handle to file
larger than 2 GB even if the size to transmit was smaller.

<<<<

[mattn, 2011-06-07 01:21:00]

ok, I uploaded code.

http://codereview.appspot.com/4536076/diff/21006/src/pkg/net/sendfile_windows.go
File src/pkg/net/sendfile_windows.go (right):

http://codereview.appspot.com/4536076/diff/21006/src/pkg/net/sendfile_windows...
src/pkg/net/sendfile_windows.go:40: rv, errno :=
syscall.WaitForSingleObject(o.o.HEvent, syscall.INFINITE)
On 2011/06/07 00:30:26, brainman wrote:
> I've said it before. I'll say it again. You don't need all that waiting code
> here - it is already implemented in fd_windows.go, you just need to use it.
> 
> That other code also implements "timeouts", which you don't do here.
> 
> If you need to loop, please do it inside sendFile. But according to
> http://www.ms-news.net/f3603/transmitfile-file-siez-limit-2067347.html,
> TransmitFile will fail for big files on some platforms regardless:
> 
> >>>>
> 
> ... it was not possible to solve this
> issue by transmitting smaller portions of the file because
> TransmitFile() failed with the same error just given a handle to file
> larger than 2 GB even if the size to transmit was smaller.
> 
> <<<<

Done.

[brainman, 2011-06-07 01:47:12]

http://codereview.appspot.com/4536076/diff/15009/src/pkg/net/sendfile_windows.go
File src/pkg/net/sendfile_windows.go (right):

http://codereview.appspot.com/4536076/diff/15009/src/pkg/net/sendfile_windows...
src/pkg/net/sendfile_windows.go:62: return int64(done), err, done > 0
Did you test all this? Does it work for you? What about big >2G files? Don't
need to loop here sending chunk after chunk?

http://codereview.appspot.com/4536076/diff/15009/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4536076/diff/15009/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:176: //sys	GetOverlappedResult(handle int32,
overlapped *Overlapped, bytesTransferred *uint32, wait bool) (errno int)
[failretval==-1]
Don't need GetOverlappedResult or CreateEvent. Please, remove them.

http://codereview.appspot.com/4536076/diff/15009/src/pkg/syscall/ztypes_windo...
File src/pkg/syscall/ztypes_windows_386.go (right):

http://codereview.appspot.com/4536076/diff/15009/src/pkg/syscall/ztypes_windo...
src/pkg/syscall/ztypes_windows_386.go:29: ERROR_DISCARDED           = 157
You aren't using any of these error const. Please remove them.

[mattn, 2011-06-07 01:50:50]

http://codereview.appspot.com/4536076/diff/15009/src/pkg/net/sendfile_windows.go
File src/pkg/net/sendfile_windows.go (right):

http://codereview.appspot.com/4536076/diff/15009/src/pkg/net/sendfile_windows...
src/pkg/net/sendfile_windows.go:62: return int64(done), err, done > 0
> Did you test all this? Does it work for you? What about big >2G files? Don't
> need to loop here sending chunk after chunk?

Yes, I tested it.
Oh sorry, I had understand as that you say "net.Sendfile don't seed to support >
2G file".

Ok, I'll update the code.

Thanks.

[brainman, 2011-06-07 02:02:43]

On 2011/06/07 01:50:50, mattn wrote:
> 
> Ok, I'll update the code.
> 

I'm not sure you need to. I think what you have is fine even for big files:
TransmitFile will send as much data as it can in one go, we return number of
bytes sent, if the code that called us decide that we need to send more, it will
call us again. That is my theory, but I would like you to confirm by putting
println() in your code while sending big files.

Alex

[mattn, 2011-06-07 05:38:05]

At the first, sorry my poor grasp and long discuss.

I've just understood that TransmitFile DO NOT support >2GB file quite.
This mean that if added loop/offset, it can't send.

When specify no-limit(meaning o.n = 0), TransmitFile don't send next serialized
block.
And When o.n 0x7fffffff, io.Exec() return error like "connection is disconnected
from server".

I decide, if pass to >2GB file, Sendfile(on windows) should pass it fallback
function 'Sendfile_Stub()'.

I updated code to simple.

Thanks.

http://codereview.appspot.com/4536076/diff/15009/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4536076/diff/15009/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:176: //sys	GetOverlappedResult(handle int32,
overlapped *Overlapped, bytesTransferred *uint32, wait bool) (errno int)
[failretval==-1]
On 2011/06/07 01:47:12, brainman wrote:
> Don't need GetOverlappedResult or CreateEvent. Please, remove them.

Done.

[brainman, 2011-06-07 06:17:54]

LGTM. Thank you.

http://codereview.appspot.com/4536076/diff/15025/src/pkg/net/sendfile_windows.go
File src/pkg/net/sendfile_windows.go (right):

http://codereview.appspot.com/4536076/diff/15025/src/pkg/net/sendfile_windows...
src/pkg/net/sendfile_windows.go:60: done, err := iosrv.ExecIO(&o, 0)
Sorry I didn't noticed earlier. You should, probably, check error here and get
out if needed:

if err != nil {
  return 0, err, false
}

Who knows what is in "done".

http://codereview.appspot.com/4536076/diff/15025/src/pkg/net/sendfile_windows...
src/pkg/net/sendfile_windows.go:64: return int64(done), err, done > 0
Then this could change to:

return int64(done), nil, true

[mattn, 2011-06-07 06:27:25]

http://codereview.appspot.com/4536076/diff/15025/src/pkg/net/sendfile_windows.go
File src/pkg/net/sendfile_windows.go (right):

http://codereview.appspot.com/4536076/diff/15025/src/pkg/net/sendfile_windows...
src/pkg/net/sendfile_windows.go:60: done, err := iosrv.ExecIO(&o, 0)
On 2011/06/07 06:17:54, brainman wrote:
> Sorry I didn't noticed earlier. You should, probably, check error here and get
> out if needed:
> 
> if err != nil {
>   return 0, err, false
> }
> 
> Who knows what is in "done".

Done.

http://codereview.appspot.com/4536076/diff/15025/src/pkg/net/sendfile_windows...
src/pkg/net/sendfile_windows.go:64: return int64(done), err, done > 0
On 2011/06/07 06:17:54, brainman wrote:
> Then this could change to:
> 
> return int64(done), nil, true

Done.

[mattn, 2011-06-07 06:29:17]

Hello bsiegert@gmail.com, bradfitz@golang.org, alex.brainman@gmail.com,
rsc@golang.org, go.peter.90@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[mattn, 2011-06-10 01:54:49]

On 2011/06/07 06:29:17, mattn wrote:
> Hello mailto:bsiegert@gmail.com, mailto:bradfitz@golang.org,
mailto:alex.brainman@gmail.com,
> mailto:rsc@golang.org, mailto:go.peter.90@gmail.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

ping

[bradfitz, 2011-06-10 17:08:57]

I'm assuming you're pinging brainman?

I trust he understands this iosrv.ExecIO stuff, because I don't.

Anyway, I'm happy with this if brainman is.

On Thu, Jun 9, 2011 at 6:54 PM, <mattn.jp@gmail.com> wrote:

> On 2011/06/07 06:29:17, mattn wrote:
>
>> Hello mailto:bsiegert@gmail.com, mailto:bradfitz@golang.org,
>>
> mailto:alex.brainman@gmail.com**,
>
>> mailto:rsc@golang.org, mailto:go.peter.90@gmail.com (cc:
>>
> mailto:golang-dev@**googlegroups.com <golang-dev@googlegroups.com>),
>
>  Please take another look.
>>
>
> ping
>
>
>
http://codereview.appspot.com/**4536076/<http://codereview.appspot.com/4536076/>
>

[brainman, 2011-06-11 00:44:03]

On 2011/06/10 17:08:57, bradfitz wrote:
> 
> I trust he understands this iosrv.ExecIO stuff, because I don't.
> 

I would have to, I wrote the staff :-). I welcome any suggestions to make it
easier to understand from anyone.

> Anyway, I'm happy with this if brainman is.
> 

It LGTM. Should I go ahead and submit it? Maybe someone else has any comments.

Alex

[bradfitz, 2011-06-11 01:58:51]

If you're happy, submit away.
On Jun 10, 2011 5:44 PM, <alex.brainman@gmail.com> wrote:
> On 2011/06/10 17:08:57, bradfitz wrote:
>
>> I trust he understands this iosrv.ExecIO stuff, because I don't.
>
>
> I would have to, I wrote the staff :-). I welcome any suggestions to
> make it easier to understand from anyone.
>
>> Anyway, I'm happy with this if brainman is.
>
>
> It LGTM. Should I go ahead and submit it? Maybe someone else has any
> comments.
>
> Alex
>
> http://codereview.appspot.com/4536076/

[brainman, 2011-06-11 03:24:59]

*** Submitted as http://code.google.com/p/go/source/detail?r=599657138e00 ***

net: Sendfile for win32.
implement using TransmitFile().

R=bsiegert, bradfitz, alex.brainman, rsc, go.peter.90
CC=golang-dev
http://codereview.appspot.com/4536076

Committer: Alex Brainman <alex.brainman@gmail.com>