code review 4528084: syscall,io,net,os,http: sendfile support

io,net,os,http: sendfile support

Speeds up static fileserver, avoiding copies userspace.

Basic design:

* io.Copy checks if its read source is a SendfileSource and its
  write destination is a SendfileDestination. If so, asks those
  for their underlying fds and if that succeeds, does the sendfile.
  (io.Copyn not yet done)

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-20 00:07:17]

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

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

[bradfitz, 2011-05-20 00:17:57]

This is still rough (not much in the way of comments of tests) but if people
are cool with the general idea / design I can polish it up.

On Thu, May 19, 2011 at 5:07 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg
>
>
> Description:
> syscall,io,net,os,http: sendfile support
>
> Speeds up static fileserver, avoiding copies userspace.
>
> Basic design:
>
> * io.Copy checks if its read source is a SendfileSource and its
>  write destination is a SendfileDestination. If so, asks those
>  for their underlying fds and if that succeeds, does the sendfile.
>  (io.Copyn not yet done)
>
> 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)
>
> Please review this at http://codereview.appspot.com/4528084/
>
> Affected files:
>  M src/pkg/http/fs.go
>  M src/pkg/http/server.go
>  M src/pkg/io/io.go
>  M src/pkg/net/fd.go
>  M src/pkg/net/tcpsock.go
>  M src/pkg/os/file.go
>  M src/pkg/syscall/syscall_linux.go
>  M src/pkg/syscall/zsyscall_linux_386.go
>  M src/pkg/syscall/zsyscall_linux_amd64.go
>  M src/pkg/syscall/zsyscall_linux_arm.go
>
>
>

[adg, 2011-05-20 01:46:19]

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

http://codereview.appspot.com/4528084/diff/4011/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:122: func (r *response) SendfileDestinationFd() (fd int,
waitfn func(), err os.Error) {
Hmm. I'd like to avoid making response a SendfileDestination.

Is there some way we can make http use only io.Copy and avoid teaching it about
the Sendfile stuff?

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

http://codereview.appspot.com/4528084/diff/4011/src/pkg/io/io.go#newcode261
src/pkg/io/io.go:261: type SendfileSource interface {
I think these interface type decls should be in the os package.

http://codereview.appspot.com/4528084/diff/4011/src/pkg/io/io.go#newcode269
src/pkg/io/io.go:269: func copySendfile(destfd, srcfd int, waitfn func())
(written int64, err os.Error) {
This should probably be in os, too. It should take a SendfileDestination and a
SendfileSource instead of plain descriptors.

http://codereview.appspot.com/4528084/diff/4011/src/pkg/io/io.go#newcode300
src/pkg/io/io.go:300: if runtime.GOOS == "linux" {
This check should happen in the SendfileSourceFd and SendfileDestination methods
in os.

[bradfitz, 2011-05-20 02:56:07]

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

http://codereview.appspot.com/4528084/diff/4011/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:122: func (r *response) SendfileDestinationFd() (fd int,
waitfn func(), err os.Error) {
On 2011/05/20 01:46:19, adg wrote:
> Hmm. I'd like to avoid making response a SendfileDestination.

Why?  I think it's quite elegant actually.

It lets us Flush() here and get ready for it.

> Is there some way we can make http use only io.Copy
> and avoid teaching it about the Sendfile stuff?

I don't follow... elaborate?

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

http://codereview.appspot.com/4528084/diff/4011/src/pkg/io/io.go#newcode261
src/pkg/io/io.go:261: type SendfileSource interface {
On 2011/05/20 01:46:19, adg wrote:
> I think these interface type decls should be
> in the os package.

I'm open to putting them anywhere.  I did it here out of laziness only.

This is still a proof-of-concept CL more than a finished product.

http://codereview.appspot.com/4528084/diff/4011/src/pkg/io/io.go#newcode269
src/pkg/io/io.go:269: func copySendfile(destfd, srcfd int, waitfn func())
(written int64, err os.Error) {
On 2011/05/20 01:46:19, adg wrote:
> This should probably be in os, too. It should take a
> SendfileDestination and a SendfileSource instead of plain descriptors.

Probably. Or in net.

http://codereview.appspot.com/4528084/diff/4011/src/pkg/io/io.go#newcode300
src/pkg/io/io.go:300: if runtime.GOOS == "linux" {
On 2011/05/20 01:46:19, adg wrote:
> This check should happen in the SendfileSourceFd and
> SendfileDestination methods in os.

Yeah, this is a blatant hack.  I should've added // XXXXHACKTEMPFIXMETODO

[eds2, 2011-05-20 03:04:36]

I realize this is a work in progress and I think it's worthwhile, but
please make sure the final CL leaves a way for other OSes to implement
something similar. Windows, FreeBSD, and OS X all have similar
functions available.

- Evan

[adg, 2011-05-20 03:06:46]

On 20 May 2011 12:56,  <bradfitz@golang.org> wrote:
>
> http://codereview.appspot.com/4528084/diff/4011/src/pkg/http/server.go
> File src/pkg/http/server.go (right):
>
>
http://codereview.appspot.com/4528084/diff/4011/src/pkg/http/server.go#newcod...
> src/pkg/http/server.go:122: func (r *response) SendfileDestinationFd()
> (fd int, waitfn func(), err os.Error) {
> On 2011/05/20 01:46:19, adg wrote:
>>
>> Hmm. I'd like to avoid making response a SendfileDestination.
>
> Why?  I think it's quite elegant actually.
>
> It lets us Flush() here and get ready for it.
>
>> Is there some way we can make http use only io.Copy
>> and avoid teaching it about the Sendfile stuff?
>
> I don't follow... elaborate?

What I mean is that Sendfile isn't something the application
programmer should have to think about. It should be hidden away in the
internals of os, and sending a fd down a socket should "just work". In
other words, http shouldn't need to be a special case.

Andrew

[bradfitz, 2011-05-20 03:13:59]

On Thu, May 19, 2011 at 8:06 PM, Andrew Gerrand <adg@golang.org> wrote:

> On 20 May 2011 12:56,  <bradfitz@golang.org> wrote:
> >
> > http://codereview.appspot.com/4528084/diff/4011/src/pkg/http/server.go
> > File src/pkg/http/server.go (right):
> >
> >
>
http://codereview.appspot.com/4528084/diff/4011/src/pkg/http/server.go#newcod...
> > src/pkg/http/server.go:122: func (r *response) SendfileDestinationFd()
> > (fd int, waitfn func(), err os.Error) {
> > On 2011/05/20 01:46:19, adg wrote:
> >>
> >> Hmm. I'd like to avoid making response a SendfileDestination.
> >
> > Why?  I think it's quite elegant actually.
> >
> > It lets us Flush() here and get ready for it.
> >
> >> Is there some way we can make http use only io.Copy
> >> and avoid teaching it about the Sendfile stuff?
> >
> > I don't follow... elaborate?
>
> What I mean is that Sendfile isn't something the application
> programmer should have to think about. It should be hidden away in the
> internals of os, and sending a fd down a socket should "just work".


And it does!  I implemented the source side on os.File and the dest side on
net.Conn.  So those work transparently.

But...


> In other words, http shouldn't need to be a special case.
>

http.response is NOT just a net.Conn.  HTTP/1.1 chunking is a transport on
top of a raw socket.  Which is why I implement the interface on
http.response too.  I'm asking it:  "Is it appropriate for me to send a
bunch of raw unformatted bytes down your socket?  If so, can I have its fd
please?"  And if it's in chunking mode, we don't do that.  (although I could
fix that with a bit more work)

I don't think this is disgusting in the least.  It seems like a perfect use
of interface, outside of not being documented in the current CL.

[bradfitz, 2011-05-20 03:15:51]

On Thu, May 19, 2011 at 8:04 PM, Evan Shaw <edsrzf@gmail.com> wrote:

> I realize this is a work in progress and I think it's worthwhile, but
> please make sure the final CL leaves a way for other OSes to implement
> something similar. Windows, FreeBSD, and OS X all have similar
> functions available.
>

Yup, for sure.  I've done sendfile on FreeBSD in the past, and I think OS X
is similar. (arguments reversed, iirc)

I have no clue about Windows completion ports or whatever, but I'm sure
somebody will clue me in.  I want Windows to work and I won't get it in the
way.

[adg, 2011-05-20 03:20:00]

On 20 May 2011 13:13, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>> In other words, http shouldn't need to be a special case.
>
> http.response is NOT just a net.Conn.  HTTP/1.1 chunking is a transport on
> top of a raw socket.  Which is why I implement the interface on
> http.response too.  I'm asking it:  "Is it appropriate for me to send a
> bunch of raw unformatted bytes down your socket?  If so, can I have its fd
> please?"  And if it's in chunking mode, we don't do that.  (although I could
> fix that with a bit more work)

Is Sendfile the only time where you would want raw access to the
underlying fd? Maybe there's a more general interface that could be
invented and used here.

Maybe my objection is rooted more in the naming. I haven't thought
about it enough.

> I don't think this is disgusting in the least.  It seems like a perfect use
> of interface, outside of not being documented in the current CL.

I didn't say it was disgusting. :-)

FWIW I think this is really nice. I just want it to be nicer.

Andrew

[bradfitz, 2011-05-20 03:25:58]

On Thu, May 19, 2011 at 8:19 PM, Andrew Gerrand <adg@golang.org> wrote:

> On 20 May 2011 13:13, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> >> In other words, http shouldn't need to be a special case.
> >
> > http.response is NOT just a net.Conn.  HTTP/1.1 chunking is a transport
> on
> > top of a raw socket.  Which is why I implement the interface on
> > http.response too.  I'm asking it:  "Is it appropriate for me to send a
> > bunch of raw unformatted bytes down your socket?  If so, can I have its
> fd
> > please?"  And if it's in chunking mode, we don't do that.  (although I
> could
> > fix that with a bit more work)
>
> Is Sendfile the only time where you would want raw access to the
> underlying fd? Maybe there's a more general interface that could be
> invented and used here.
>
> Maybe my objection is rooted more in the naming. I haven't thought
> about it enough.
>

There could perhaps be a more general name, but generality often brings
ambiguity and eventually complexity.

The benefit to having a very specific name and signature is that io.Copy can
be documented to say, "Hey, I'm only going to do this crazy thing if you
implement this crazy method.", as opposed to it being more magical in more
cases.

Imagine if the only method was:

     FD() int

... and now we start accidentally sendfile'ing to people's FDs without them
being prepared for it (perhaps they have unbuffered data they haven't sent).

I don't expect this to spread far.  sendfile itself is pretty limited in
what types of fds it can operate on, and we've pretty much implemented the
two already.  http is a "third" in that it's a popular application that
warrants the speed-up.

[mattn, 2011-05-20 03:54:50]

Sorry about that I sent few messages into google groups.

We can port it to windows using TransmitFile().
http://msdn.microsoft.com/en-us/library/ms740565(v=vs.85).aspx

I can do it.

On 2011/05/20 03:25:58, bradfitz wrote:
> On Thu, May 19, 2011 at 8:19 PM, Andrew Gerrand <mailto:adg@golang.org> wrote:
> 
> > On 20 May 2011 13:13, Brad Fitzpatrick <mailto:bradfitz@golang.org> wrote:
> > >> In other words, http shouldn't need to be a special case.
> > >
> > > http.response is NOT just a net.Conn.  HTTP/1.1 chunking is a transport
> > on
> > > top of a raw socket.  Which is why I implement the interface on
> > > http.response too.  I'm asking it:  "Is it appropriate for me to send a
> > > bunch of raw unformatted bytes down your socket?  If so, can I have its
> > fd
> > > please?"  And if it's in chunking mode, we don't do that.  (although I
> > could
> > > fix that with a bit more work)
> >
> > Is Sendfile the only time where you would want raw access to the
> > underlying fd? Maybe there's a more general interface that could be
> > invented and used here.
> >
> > Maybe my objection is rooted more in the naming. I haven't thought
> > about it enough.
> >
> 
> There could perhaps be a more general name, but generality often brings
> ambiguity and eventually complexity.
> 
> The benefit to having a very specific name and signature is that io.Copy can
> be documented to say, "Hey, I'm only going to do this crazy thing if you
> implement this crazy method.", as opposed to it being more magical in more
> cases.
> 
> Imagine if the only method was:
> 
>      FD() int
> 
> ... and now we start accidentally sendfile'ing to people's FDs without them
> being prepared for it (perhaps they have unbuffered data they haven't sent).
> 
> I don't expect this to spread far.  sendfile itself is pretty limited in
> what types of fds it can operate on, and we've pretty much implemented the
> two already.  http is a "third" in that it's a popular application that
> warrants the speed-up.

[iant, 2011-05-20 05:26:56]

I guess it seems to me that this can fit into the existing ReadFrom interface. 
It seems to be only interesting when copying from a regular file to a network
socket.  So implement ReadFrom for *netFD (and *TCPConn and *http.response). 
ReadFrom can simply ask whether its argument is a os.File.  If it is, call the
Fd() method, and try to call sendfile.  If it is not an os.File, or if sendfile
fails, fall back to simply copying the data.  I suppose we could make the
copying a little simpler if we split up io.Copy to create io.RawCopy or
something which would do the copy without checking for ReadFrom.

I guess a problem with that is that it doesn't take advantage of io.CopyNt won't
see through the limitedReader.

In general I'm not crazy about having to have two interfaces, one for the source
and one for the destination.  At least on GNU/Linux this is a fairly specialized
operation and it might as well be treated as such.

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

http://codereview.appspot.com/4528084/diff/4011/src/pkg/io/io.go#newcode12
src/pkg/io/io.go:12: import "runtime"
Use parentheses.

http://codereview.appspot.com/4528084/diff/4011/src/pkg/io/io.go#newcode271
src/pkg/io/io.go:271: n, err := syscall.Sendfile(destfd, srcfd,
(*int)(unsafe.Pointer(uintptr(0))), 256*1024)
s/uintptr(0)/nil/

http://codereview.appspot.com/4528084/diff/4011/src/pkg/syscall/syscall_linux.go
File src/pkg/syscall/syscall_linux.go (right):

http://codereview.appspot.com/4528084/diff/4011/src/pkg/syscall/syscall_linux...
src/pkg/syscall/syscall_linux.go:799: //sys   Sendfile(outfd int, infd int,
offset *int, count int) (written int, errno int)
Using *int as the type for offset is wrong, as the C type is *off_t.  I think it
will be necessary to put a version of this in each syscall_linux_ARCH.go, with
the appropriate type for offset.

[bradfitz, 2011-05-20 19:59:09]

On Thu, May 19, 2011 at 10:26 PM, <iant@golang.org> wrote:

>  ReadFrom can simply ask whether its
> argument is a os.File.  If it is, call the Fd() method, and try to call
> sendfile.  If it is not an os.File, or if sendfile fails, fall back to
> simply copying the data.  I suppose we could make the copying a little
> simpler if we split up io.Copy to create io.RawCopy or something which
> would do the copy without checking for ReadFrom.
>

Do you like that more (an io.RawCopy) more than an an ErrDeclined returned
value?

With ErrDeclined, io.Copy[n] could proceed to its next step:

        // Similarly, if the reader has a WriteTo method, use it to do the
copy.
        if wt, ok := src.(WriterTo); ok {
                return wt.WriteTo(dst)
        }

... without http.response and TCPConn needing to try that check first, or
without them knowing to need to call
io.{RawCopy,RawCopyButWriterToCheckFirst}.

[iant2, 2011-05-21 03:23:22]

Brad Fitzpatrick <bradfitz@golang.org> writes:

> On Thu, May 19, 2011 at 10:26 PM, <iant@golang.org> wrote:
>
>>  ReadFrom can simply ask whether its
>> argument is a os.File.  If it is, call the Fd() method, and try to call
>> sendfile.  If it is not an os.File, or if sendfile fails, fall back to
>> simply copying the data.  I suppose we could make the copying a little
>> simpler if we split up io.Copy to create io.RawCopy or something which
>> would do the copy without checking for ReadFrom.
>>
>
> Do you like that more (an io.RawCopy) more than an an ErrDeclined returned
> value?

Personally, I do.  My leaning is to have an error be an error.  If we
add ErrDeclined, then every caller has to handle that possibility;
caller can no longer simply return the error if there is one.  Since
every caller would have to handle it, if we want to go this route, I
would rather add it as a third result parameter, rather than overloading
the error result.

> With ErrDeclined, io.Copy[n] could proceed to its next step:
> ...
> without http.response and TCPConn needing to try that check first, or
> without them knowing to need to call
> io.{RawCopy,RawCopyButWriterToCheckFirst}.

It's a fair point.  I don't have a good answer offhand.


As a separate issue, we'll have to consider adding a ReadFromN method to
efficiently handle io.CopyN.

Ian