code review 4543071: io, net, http: sendfile support

io, net, http: sendfile support

Speeds up static fileserver, avoiding kernel/userspace copies.

Numbers: downloading 14 MB AppEngine Go SDK with ab (Apache Bench)
with 5 threads:

Before/after numbers:

CPU:
user    0m3.910s
sys     0m23.650s
->
user    0m0.720s
sys     0m4.890s

Time taken for tests:   8.906 seconds
->
Time taken for tests:   8.545 seconds

Percentage of the requests served within a certain time (ms)
50%     44
66%     45
75%     46
80%     46
90%     48
95%     51
98%     59
99%     71
100     74 (longest request)
->
50%     42
66%     43
75%     43
80%     44
90%     46
95%     57
98%     62
99%     63
100%    64 (longest request)

[bradfitz, 2011-05-24 17:17:19]

Hello [email protected] (cc: [email protected]),

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

[rsc1, 2011-05-24 17:26:07]

I like this version better than any I've seen yet.

I think the two errors can be combined into a
single io.ErrUnimplemented.

http shouldn't need to change.
I don't mind exposing the underlying type
that LimitReader returns (say, LimitedReader)
so that the network ReadFrom can look for it.

Russ

[bradfitz, 2011-05-24 17:33:18]

On Tue, May 24, 2011 at 10:25 AM, Russ Cox <[email protected]> wrote:

> I like this version better than any I've seen yet.
>
> I think the two errors can be combined into a
> single io.ErrUnimplemented.
>

io.ErrUnimplemented works.


> http shouldn't need to change.
>

Why not?  What if we're chunking?  What if compression headers are set?  We
can't do a raw sendfile in either of those cases.  Where would you put such
checks?


> I don't mind exposing the underlying type
> that LimitReader returns (say, LimitedReader)
> so that the network ReadFrom can look for it.


Oh, you mean don't change http/fs.go, but keep the change to http/server.go?

[rsc1, 2011-05-24 17:35:34]

> Why not?  What if we're chunking?  What if compression headers are set?  We
> can't do a raw sendfile in either of those cases.  Where would you put such
> checks?
>
>>
>> I don't mind exposing the underlying type
>> that LimitReader returns (say, LimitedReader)
>> so that the network ReadFrom can look for it.
>
> Oh, you mean don't change http/fs.go, but keep the change to http/server.go?

Yes, sorry.

[bradfitz, 2011-05-24 17:57:17]

On Tue, May 24, 2011 at 10:35 AM, Russ Cox <[email protected]> wrote:

> > Why not?  What if we're chunking?  What if compression headers are set?
>  We
> > can't do a raw sendfile in either of those cases.  Where would you put
> such
> > checks?
> >
> >>
> >> I don't mind exposing the underlying type
> >> that LimitReader returns (say, LimitedReader)
> >> so that the network ReadFrom can look for it.
> >
> > Oh, you mean don't change http/fs.go, but keep the change to
> http/server.go?
>
> Yes, sorry.
>

So without a ReaderFromN, you're thinking:

io.Copyn(httpReadWriter, diskFile, 10)
->
(*httpReadWriter) ReadFrom(LimitedReader(diskFile, 10))
->
(*net.TCPConn) ReadFrom(LimitedReader(diskFile, 10))
->

   if lw, ok := dest.(*io.LimitedReader); ok {
          n := lw.MaxBytesRemaining()
          tcpConn.SendFile(?????, n)
          // ^^^^^^^^^^^^^^^^^^^^^^^^^^^  where to get the *os.File?
          // else io.ErrNotImplemented
   }
   // else io.ErrNotImplemented


If we ask the LimitedReader for its backing io.Reader (or its backing
*os.File if its io.Reader is a File), is that more or less ugly than saying
sendfile is only implemented for io.Copy and not io.Copyn?

Do you want:

type LimitedReader interface {
     io.Reader
     MaxBytesRemaining() int64
     UnderlyingReader() io.Reader
}

or something like:

type LimitedReader struct {
     io.Reader
}

func (lr *LimitedReader) MaxBytesRemaining() int64 { ... }

?

We don't have to do either of those if we just say that only io.Copy gets
sendfile (at least for now).  I'm totally happy not having io.Copyn be fast,
since HTTP Range requests are typically a) not present or b) from a point
midway through a big file until the very end.  They're rarely just some
middle region.

[bradfitz, 2011-05-24 17:57:51]

On Tue, May 24, 2011 at 10:57 AM, Brad Fitzpatrick <[email protected]>wrote:

> On Tue, May 24, 2011 at 10:35 AM, Russ Cox <[email protected]> wrote:
>
>> > Why not?  What if we're chunking?  What if compression headers are set?
>>  We
>> > can't do a raw sendfile in either of those cases.  Where would you put
>> such
>> > checks?
>> >
>> >>
>> >> I don't mind exposing the underlying type
>> >> that LimitReader returns (say, LimitedReader)
>> >> so that the network ReadFrom can look for it.
>> >
>> > Oh, you mean don't change http/fs.go, but keep the change to
>> http/server.go?
>>
>> Yes, sorry.
>>
>
> So without a ReaderFromN, you're thinking:
>
> io.Copyn(httpReadWriter, diskFile, 10)
> ->
> (*httpReadWriter) ReadFrom(LimitedReader(diskFile, 10))
>

er, s/httpReadWriter/httpResponseWriter/g

[iant2, 2011-05-24 17:59:32]

Russ Cox <[email protected]> writes:

> I like this version better than any I've seen yet.
>
> I think the two errors can be combined into a
> single io.ErrUnimplemented.
>
> http shouldn't need to change.
> I don't mind exposing the underlying type
> that LimitReader returns (say, LimitedReader)
> so that the network ReadFrom can look for it.

What makes me uncomfortable about ErrUnimplemented is that it means that
anybody who calls the function has to be aware of the possibility that
this error could occur.  You can't just pass back any error code to the
caller.  Still, io.ErrUnimplemented is better than os.ErrUnimplemented.

Ian

[rsc1, 2011-05-24 18:01:27]

> What makes me uncomfortable about ErrUnimplemented is that it means that
> anybody who calls the function has to be aware of the possibility that
> this error could occur.  You can't just pass back any error code to the
> caller.  Still, io.ErrUnimplemented is better than os.ErrUnimplemented.

I just found your comment about this in the other thread
a minute or two ago.  I agree that this is not quite there yet.

Russ

[rsc1, 2011-05-24 18:02:41]

> Do you want:
> type LimitedReader interface {
>      io.Reader
>      MaxBytesRemaining() int64
>      UnderlyingReader() io.Reader
> }
> or something like:
> type LimitedReader struct {
>      io.Reader
> }
> func (lr *LimitedReader) MaxBytesRemaining() int64 { ... }

I was thinking the only change would be to turn

type limitedReader struct {
        r Reader
        n int64
}

into

type LimitedReader struct {
        R Reader
        N int64
}

Russ

[bradfitzgoog, 2011-05-24 18:04:25]

On Tue, May 24, 2011 at 11:01 AM, Russ Cox <[email protected]> wrote:

> > What makes me uncomfortable about ErrUnimplemented is that it means that
> > anybody who calls the function has to be aware of the possibility that
> > this error could occur.  You can't just pass back any error code to the
> > caller.  Still, io.ErrUnimplemented is better than os.ErrUnimplemented.
>
> I just found your comment about this in the other thread
> a minute or two ago.  I agree that this is not quite there yet.


Rather than ErrUnimplemented, and avoiding exposing io.{Raw,Buffered}Copy
(which r dislikes), I could copy/paste the implementation of io.Copy into
both http.response's ReadFrom and TCPConn's ReadFrom.  That's a fair bit of
copy/paste into two places, though.

[gburd, 2011-05-24 18:33:18]

http://codereview.appspot.com/4543071/diff/4001/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4543071/diff/4001/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:122: func (r *response) ReadFrom(src io.Reader) (int64,
os.Error) {
Here's a way to eliminate ErrDeclined. Define type writerOnly:

type writerOnly struct { io.Writer }

Call io.Copy with a writerOnly instead of returning io.ErrDeclined:

    io.Copy(writerOnly{r}, src)

[bradfitzgoog, 2011-05-24 19:16:19]

On Tue, May 24, 2011 at 11:33 AM, <[email protected]> wrote:

>
> http://codereview.appspot.com/4543071/diff/4001/src/pkg/http/server.go
> File src/pkg/http/server.go (right):
>
>
>
http://codereview.appspot.com/4543071/diff/4001/src/pkg/http/server.go#newcod...
> src/pkg/http/server.go:122: func (r *response) ReadFrom(src io.Reader)
> (int64, os.Error) {
> Here's a way to eliminate ErrDeclined. Define type writerOnly:
>
> type writerOnly struct { io.Writer }
>
> Call io.Copy with a writerOnly instead of returning io.ErrDeclined:
>
>    io.Copy(writerOnly{r}, src)


Nice!

Let me try that out...

[rsc1, 2011-05-24 19:19:36]

> type writerOnly struct { io.Writer }
>
> Call io.Copy with a writerOnly instead of returning io.ErrDeclined:
>
>    io.Copy(writerOnly{r}, src)

well done

[bradfitz, 2011-05-24 19:51:26]

Hello [email protected], [email protected], [email protected],
[email protected], [email protected] (cc: [email protected]),

Please take another look.

[bradfitz, 2011-05-24 19:52:43]

Now with public io.LimitedReader and Gary's onlyWriter suggestion to remove
need for ErrDeclined or io.BufferedCopy.

On Tue, May 24, 2011 at 12:51 PM, <[email protected]> wrote:

> Hello [email protected], [email protected], [email protected],
> [email protected], [email protected] (cc:
> [email protected]),
>
> Please take another look.
>
>
> http://codereview.appspot.com/4543071/
>

[rsc, 2011-05-24 20:08:54]

I think this is converging on something nice and clean.

Detailed comments only on http + io.

For net, can SendFile go?  It's an implementation detail.
ReadFrom should be the only public way to get at this
functionality.

I'd suggest putting 

func genericReadFrom(c net.Conn, r io.Reader) (int64, os.Error)

in some non-os-specific file, and then still in portable code:

func (c *TCPConn) ReadFrom(r io.Reader) (int64, os.Error) {
    if n, err, handled := sendFile(c, r); handled {
        return n, err
    }
    return genericReadFrom(c, r)
}

(and same for UnixConn)

and then the only os-specific code is

func sendFile(c net.Conn, r io.Reader) (n int64, err os.Error, handled bool)

which is a 1-line implementation in the stub file and
maybe a few more in the linux one.  :-)

The sendfile implementation should use OpError and os.Errno instead of printf
%d.  Grep for examples.

http://codereview.appspot.com/4543071/diff/6008/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4543071/diff/6008/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:127: if r.chunking {
just to avoid the doubling, I'd nest this

// Can use raw network ReadFrom if it's available
// and we're not chunking.
if !r.chunking {
    if rf, ok := r.conn.rwc.(io.ReaderFrom); ok {
        r.Flush()
        retrn rf.ReadFrom(src)
    }
}

// Fall back to default io.Copy implementation.
// Use wrapper to hide r.ReadFrom from io.Copy.
return io.Copy(writerOnly{r}, src)

http://codereview.appspot.com/4543071/diff/6008/src/pkg/io/io.go
File src/pkg/io/io.go (right):

http://codereview.appspot.com/4543071/diff/6008/src/pkg/io/io.go#newcode306
src/pkg/io/io.go:306: func LimitReader(r Reader, n int64) *LimitedReader {
I'd like this t continue to return io.Reader.
Most of the time that's what you want, and 
keeping it as it was will avoid breaking any := uses.

// LimitReader returns a Reader that reads from r
// but stops with os.EOF after reading n bytes.
// The underlying implementation is a *LimitedReader.

http://codereview.appspot.com/4543071/diff/6008/src/pkg/io/io.go#newcode310
src/pkg/io/io.go:310: // LimitedReader
// A LimitedReader reads from R but limits the amount of
// data returned to just N bytes.  Each call to Read
// updates N to reflect the new amount remaining.

[bradfitz, 2011-05-24 21:48:34]

Hello [email protected], [email protected], [email protected] (cc:
[email protected]),

Please take another look.

[bradfitz, 2011-05-24 21:56:01]

PTAL

all.bash succeeds on both Linux/amd64 and Darwin/amd64.

Trying Linux/386 now.

[bradfitz, 2011-05-24 22:15:48]

On Tue, May 24, 2011 at 2:56 PM, <[email protected]> wrote:

> PTAL
>
> all.bash succeeds on both Linux/amd64 and Darwin/amd64.
>
> Trying Linux/386 now.


linux/386 passes too.

[rsc, 2011-05-24 22:22:01]

>> all.bash succeeds on both Linux/amd64 and Darwin/amd64.
>>
>> Trying Linux/386 now.
>
> linux/386 passes too.

GOARCH=arm make.bash

?

[bradfitzgoog, 2011-05-24 22:28:24]

On Tue, May 24, 2011 at 3:21 PM, Russ Cox <[email protected]> wrote:

> >> all.bash succeeds on both Linux/amd64 and Darwin/amd64.
> >>
> >> Trying Linux/386 now.
> >
> > linux/386 passes too.
>
> GOARCH=arm make.bash
>

Also good.

[bradfitz, 2011-05-24 22:51:56]

On Tue, May 24, 2011 at 3:28 PM, Brad Fitzpatrick <[email protected]>wrote:

> On Tue, May 24, 2011 at 3:21 PM, Russ Cox <[email protected]> wrote:
>
>> >> all.bash succeeds on both Linux/amd64 and Darwin/amd64.
>> >>
>> >> Trying Linux/386 now.
>> >
>> > linux/386 passes too.
>>
>> GOARCH=arm make.bash
>>
>
> Also good.
>

And all tests pass on the Xoom, linux/arm.

[iant, 2011-05-24 22:53:28]

FYI

http://codereview.appspot.com/4543071/diff/3009/src/pkg/net/sendfile_linux.go
File src/pkg/net/sendfile_linux.go (right):

http://codereview.appspot.com/4543071/diff/3009/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:46: if toCopy <= 0 {
Seems like this (if toCopy <= 0) might as well be in the "if n >= 0" block.

http://codereview.appspot.com/4543071/diff/3009/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:55: if n == 0 {
Shouldn't this be "n <= 0"?

[bradfitz, 2011-05-24 22:57:13]

http://codereview.appspot.com/4543071/diff/3009/src/pkg/net/sendfile_linux.go
File src/pkg/net/sendfile_linux.go (right):

http://codereview.appspot.com/4543071/diff/3009/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:46: if toCopy <= 0 {
On 2011/05/24 22:53:28, iant wrote:
> Seems like this (if toCopy <= 0) might as well be in the "if n >= 0" block.

Oh, yes.

http://codereview.appspot.com/4543071/diff/3009/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:55: if n == 0 {
On 2011/05/24 22:53:28, iant wrote:
> Shouldn't this be "n <= 0"?

I guess? When would sendfile return no errno but a negative count?

I'd almost rather return an error if I saw that. Want me to do that?

[bradfitz, 2011-05-24 22:57:29]

Hello [email protected], [email protected], [email protected], [email protected]
(cc: [email protected]),

Please take another look.

[rsc, 2011-05-25 03:42:42]

http://codereview.appspot.com/4543071/diff/3009/src/pkg/net/sendfile_linux.go
File src/pkg/net/sendfile_linux.go (right):

http://codereview.appspot.com/4543071/diff/3009/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:31: osf, ok := r.(*os.File)
s/osf/f/

http://codereview.appspot.com/4543071/diff/3009/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:36: destfd := c.sysfd
the pairing for src is dst

http://codereview.appspot.com/4543071/diff/3009/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:38: for {
I'd be more comfortable if this looked like the Write loop in fd.go, if only for
consistency.  I think it will clarify some of the logic too.

http://codereview.appspot.com/4543071/diff/3009/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:48: handled = true
Isn't there a specific errno to look for to decide "not handled"?

http://codereview.appspot.com/4543071/diff/3009/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:52: sn, errno := syscall.Sendfile(destfd, srcfd,
(*int64)(unsafe.Pointer(nil)), int(toCopy))
s/(*int64)(unsafe.Pointer(nil))/nil/

[rsc, 2011-05-25 03:54:26]

http://codereview.appspot.com/4543071/diff/5016/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4543071/diff/5016/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:127: r.Flush()
only needed before call to rf.ReadFrom

http://codereview.appspot.com/4543071/diff/5016/src/pkg/net/sendfile_linux.go
File src/pkg/net/sendfile_linux.go (right):

http://codereview.appspot.com/4543071/diff/5016/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:38: for {
This loop confuses me.  Here's something simpler.
Maybe it's not right but it might be a start.

for limit != 0 {
    n := maxSendfileSize
    if limit > 0 && int64(n) > limit {
        n = int(limit)
    }
    n, errno := syscall.Sendfile(dst, src, nil, n)
    if n > 0 {
        written += n
        if limit > 0 {
            limit -= n
        }
    }
    if n == 0 && errno == 0 {
        break
    }
    if errno == syscall.EAGAIN && fd.wdeadline >= 0 {
        pollserver.WaitWrite(fd)
        continue
    }
    if errno != 0 {
        err = &OpError{"sendfile", fd.net, fd.raddr, os.Errno(err)}
        break
    }
}
return written, err, written > 0

http://codereview.appspot.com/4543071/diff/5016/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:55: if n == 0 {
I'm confused.  Is this supposed to be sn?
Same two lines later.  I think you'd have
less potential for confusion if you renamed
the n above limit.  It has a very large scope
for a one-letter variable name.
Then the sn can be n, which I think this code assumes it is.

http://codereview.appspot.com/4543071/diff/5016/src/pkg/net/tcpsock.go
File src/pkg/net/tcpsock.go (right):

http://codereview.appspot.com/4543071/diff/5016/src/pkg/net/tcpsock.go#newcode99
src/pkg/net/tcpsock.go:99: // ReadFrom implements the io.ReaderFrom ReadFrom
method.
This file looks great; please do the same to UnixConn.

[rsc, 2011-05-25 03:55:37]

> This loop confuses me.  Here's something simpler.
> Maybe it's not right but it might be a start.

can simplify further by

limit := 1<<62

instead of

limit := -1

[bradfitz, 2011-05-25 16:10:53]

Hello [email protected], [email protected], [email protected], [email protected]
(cc: [email protected]),

Please take another look.

[bradfitz, 2011-05-25 16:13:50]

http://codereview.appspot.com/4543071/diff/5016/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4543071/diff/5016/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:127: r.Flush()
On 2011/05/25 03:54:26, rsc wrote:
> only needed before call to rf.ReadFrom

No, it's needed before we check r.chunking, which is set by Flush() ->
WriteHeader() -> which checks whether we've set Content-Length in the response
headers.

I'll add a comment, since that's probably unclear.

http://codereview.appspot.com/4543071/diff/5016/src/pkg/net/sendfile_linux.go
File src/pkg/net/sendfile_linux.go (right):

http://codereview.appspot.com/4543071/diff/5016/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:38: for {
On 2011/05/25 03:54:26, rsc wrote:
> This loop confuses me.  Here's something simpler.
> Maybe it's not right but it might be a start.
>

Okay, I did something similar.

Yes, this is much prettier in hindsight.

Your fd.wdeadline >= 0 check isn't correct, though.  Watching it serve a large
file with strace, I see an EAGAIN flood without any epolls if I include that.  I
removed that clause as I had it before and that goes away.

http://codereview.appspot.com/4543071/diff/5016/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:55: if n == 0 {
On 2011/05/25 03:54:26, rsc wrote:
> I'm confused.  Is this supposed to be sn?
> Same two lines later.  I think you'd have
> less potential for confusion if you renamed
> the n above limit.  It has a very large scope
> for a one-letter variable name.
> Then the sn can be n, which I think this code assumes it is.

Renamed.

[rsc, 2011-05-25 16:24:02]

looks good but can still be a little simpler i think

http://codereview.appspot.com/4543071/diff/7010/src/pkg/net/sendfile_linux.go
File src/pkg/net/sendfile_linux.go (right):

http://codereview.appspot.com/4543071/diff/7010/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:40: for remain != 0 {
this will then be for remain > 0

http://codereview.appspot.com/4543071/diff/7010/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:42: if remain > 0 && int64(n) > remain {
s/remain > 0 && //

http://codereview.appspot.com/4543071/diff/7010/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:46: switch errno {
the convention (regardless of Sendfile's implementation)
is that a routine that returns n, errno can return 
n >0, errno != 0 if it transferred some data before
hitting the error.  so the handling of n > 0 should
be up here before the switch, not inside case 0.

http://codereview.appspot.com/4543071/diff/7010/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:51: if remain > 0 {
this entire if block becomes

remain -= int64(n)

http://codereview.appspot.com/4543071/diff/7010/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:60: default:
i think this can go

http://codereview.appspot.com/4543071/diff/7010/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:64: case syscall.ENOSYS, syscall.EINVAL:
the return below says written>0 instead of handled.
if you think it is not accurate enough to use these to
mark handled = false, then they can be deleted from
the switch and handled like any other error.

[bradfitz, 2011-05-25 16:45:36]

Hello [email protected], [email protected], [email protected], [email protected]
(cc: [email protected]),

Please take another look.

[bradfitz, 2011-05-25 16:45:54]

Simpler!

http://codereview.appspot.com/4543071/diff/7010/src/pkg/net/sendfile_linux.go
File src/pkg/net/sendfile_linux.go (right):

http://codereview.appspot.com/4543071/diff/7010/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:40: for remain != 0 {
On 2011/05/25 16:24:02, rsc wrote:
> this will then be for remain > 0

Done.

http://codereview.appspot.com/4543071/diff/7010/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:46: switch errno {
On 2011/05/25 16:24:02, rsc wrote:
> the convention (regardless of Sendfile's implementation)
> is that a routine that returns n, errno can return 
> n >0, errno != 0 if it transferred some data before
> hitting the error.  so the handling of n > 0 should
> be up here before the switch, not inside case 0.

Done.

http://codereview.appspot.com/4543071/diff/7010/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:64: case syscall.ENOSYS, syscall.EINVAL:
On 2011/05/25 16:24:02, rsc wrote:
> the return below says written>0 instead of handled.
> if you think it is not accurate enough to use these to
> mark handled = false, then they can be deleted from
> the switch and handled like any other error.

Rob had also requested checking for explicit errno values that signal sendfile
wasn't supported.  Turned it into a comment instead.

[rsc, 2011-05-25 16:46:34]

http://codereview.appspot.com/4543071/diff/15003/src/pkg/net/sendfile_linux.go
File src/pkg/net/sendfile_linux.go (right):

http://codereview.appspot.com/4543071/diff/15003/src/pkg/net/sendfile_linux.g...
src/pkg/net/sendfile_linux.go:64: return written, err, written > 0
if lr != nil {
    lr.N = remain
}

[bradfitz, 2011-05-25 16:48:11]

http://codereview.appspot.com/4543071/diff/15003/src/pkg/net/sendfile_linux.go
File src/pkg/net/sendfile_linux.go (right):

http://codereview.appspot.com/4543071/diff/15003/src/pkg/net/sendfile_linux.g...
src/pkg/net/sendfile_linux.go:64: return written, err, written > 0
On 2011/05/25 16:46:34, rsc wrote:
> if lr != nil {
>     lr.N = remain
> }

I suppose that's polite. :)

Done.

[rsc, 2011-05-25 16:48:23]

http://codereview.appspot.com/4543071/diff/15003/src/pkg/net/sendfile_linux.go
File src/pkg/net/sendfile_linux.go (right):

http://codereview.appspot.com/4543071/diff/15003/src/pkg/net/sendfile_linux.g...
src/pkg/net/sendfile_linux.go:52: if errno == syscall.EAGAIN {
now that the code looks like this you can put back

if errno == syscall.EAGAIN && c.wdeadline >= 0 {

[bradfitz, 2011-05-25 16:48:52]

Hello [email protected], [email protected], [email protected], [email protected]
(cc: [email protected]),

Please take another look.

[bradfitz, 2011-05-25 16:58:18]

Hello [email protected], [email protected], [email protected], [email protected]
(cc: [email protected]),

Please take another look.

[bradfitz, 2011-05-25 17:03:06]

Hello [email protected], [email protected], [email protected], [email protected]
(cc: [email protected]),

Please take another look.

[rsc, 2011-05-25 17:03:45]

add unixsock.go?

http://codereview.appspot.com/4543071/diff/13006/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4543071/diff/13006/src/pkg/http/server.go#newco...
src/pkg/http/server.go:134: if err == nil {
drop if statement
(r.written += n unconditionally)

http://codereview.appspot.com/4543071/diff/13006/src/pkg/net/sendfile_linux.go
File src/pkg/net/sendfile_linux.go (right):

http://codereview.appspot.com/4543071/diff/13006/src/pkg/net/sendfile_linux.g...
src/pkg/net/sendfile_linux.go:27: c.wio.Lock()
might as well do this after the two type assertion tests below.
if you're not going to use it no sense setting up

http://codereview.appspot.com/4543071/diff/13006/src/pkg/net/sock.go
File src/pkg/net/sock.go (right):

http://codereview.appspot.com/4543071/diff/13006/src/pkg/net/sock.go#newcode11
src/pkg/net/sock.go:11: "io"
up a line

[bradfitz, 2011-05-25 17:07:27]

On Wed, May 25, 2011 at 10:03 AM, <[email protected]> wrote:

> add unixsock.go?
>

UnixSock has the previously-discussed problem where ReadFrom is already used
by a different interface (packet reading) with a different signature.  You
had said way back when to somebody else to just focus on TCP for now then.

Not sure how much I care about UnixSocket copy anyway. Seems like a rare
case?



>
>
>
> http://codereview.appspot.com/4543071/diff/13006/src/pkg/http/server.go
>
> File src/pkg/http/server.go (right):
>
>
>
http://codereview.appspot.com/4543071/diff/13006/src/pkg/http/server.go#newco...
> src/pkg/http/server.go:134: if err == nil {
> drop if statement
> (r.written += n unconditionally)
>

done


>
>
> http://codereview.appspot.com/4543071/diff/13006/src/pkg/net/sendfile_linux.go
>
> File src/pkg/net/sendfile_linux.go (right):
>
>
>
http://codereview.appspot.com/4543071/diff/13006/src/pkg/net/sendfile_linux.g...
> src/pkg/net/sendfile_linux.go:27: c.wio.Lock()
> might as well do this after the two type assertion tests below.
> if you're not going to use it no sense setting up
>

done


>
> http://codereview.appspot.com/4543071/diff/13006/src/pkg/net/sock.go
> File src/pkg/net/sock.go (right):
>
>
> http://codereview.appspot.com/4543071/diff/13006/src/pkg/net/sock.go#newcode11
> src/pkg/net/sock.go:11: "io"
> up a line
>


done

[bradfitz, 2011-05-25 17:08:38]

Hello [email protected], [email protected], [email protected], [email protected]
(cc: [email protected]),

Please take another look.

[rsc, 2011-05-25 17:08:42]

> UnixSock has the previously-discussed problem where ReadFrom is already used
> by a different interface (packet reading) with a different signature.  You
> had said way back when to somebody else to just focus on TCP for now then.
> Not sure how much I care about UnixSocket copy anyway. Seems like a rare
> case?

aha; sgtm

[rsc, 2011-05-25 17:10:46]

LGTM++

Turned out very nice.

[bradfitz, 2011-05-25 17:15:34]

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

io, net, http: sendfile support

Speeds up static fileserver, avoiding kernel/userspace copies.

Numbers: downloading 14 MB AppEngine Go SDK with ab (Apache Bench)
with 5 threads:

Before/after numbers:

CPU:
user    0m3.910s
sys     0m23.650s
->
user    0m0.720s
sys     0m4.890s

Time taken for tests:   8.906 seconds
->
Time taken for tests:   8.545 seconds

Percentage of the requests served within a certain time (ms)
50%     44
66%     45
75%     46
80%     46
90%     48
95%     51
98%     59
99%     71
100     74 (longest request)
->
50%     42
66%     43
75%     43
80%     44
90%     46
95%     57
98%     62
99%     63
100%    64 (longest request)

R=iant, gary.burd, rsc, bradfitz
CC=golang-dev
http://codereview.appspot.com/4543071

[gburd, 2011-05-25 19:13:25]

http://codereview.appspot.com/4543071/diff/21002/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4543071/diff/21002/src/pkg/http/server.go#newco...
src/pkg/http/server.go:140: return io.Copy(writerOnly{r}, src)
Should r.written be updated here?

n, err = io.Copy(writerOnly{r}, src)
r.written += n
return

[bradfitz, 2011-05-25 19:14:15]

http://codereview.appspot.com/4543071/diff/21002/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4543071/diff/21002/src/pkg/http/server.go#newco...
src/pkg/http/server.go:140: return io.Copy(writerOnly{r}, src)
On 2011/05/25 19:13:25, gburd wrote:
> Should r.written be updated here?
> 
> n, err = io.Copy(writerOnly{r}, src)
> r.written += n
> return

yes! good catch, again.

[rsc, 2011-05-25 19:14:18]

> n, err = io.Copy(writerOnly{r}, src)
> r.written += n
> return

Copy will call r.Write which will update r.written

[bradfitz, 2011-05-25 19:16:00]

On Wed, May 25, 2011 at 12:14 PM, Russ Cox <[email protected]> wrote:

> > n, err = io.Copy(writerOnly{r}, src)
> > r.written += n
> > return
>
> Copy will call r.Write which will update r.written
>

 oh right.

and that would explain why all the tests pass too.  :)

(the tests were catching failures in early versions before I incremented
r.written in the sendfile case...)