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

http,io,net: sendfile support

v2 using ReaderFrom / ReaderNFrom

[bradfitz, 2011-05-23 23:45:17]

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

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

[r2, 2011-05-24 00:23:51]

I dislike the precedent this sets.  Sendfile is a disgusting API and
we're propagating through the tree.  Further propagation is sure to
come, and further disgusting APIs will follow in its path.

I'm not vetoing this, just registering a deep objection.  If others
agree this should go forward, I will recuse myself. It seems the
choice is clean code or happy users. I don't like that choice.

-rob

[bradfitz, 2011-05-24 00:34:39]

A systems programming language but we can't get too close to the system if
it gets ugly?

Sendfile fwiw is real & makes a difference.  Let's try to find a way to make
it not offensive.

If I make LimitedReader a public type, exposing its bytes remaining, I think
we can ditch all the ReaderNFrom stuff.  This CL is just another potential
version, this one largely influenced by Russ & Ian's comments.  I'll
implement them all so people can see how they all end up feeling.


On Mon, May 23, 2011 at 5:23 PM, Rob 'Commander' Pike <r@google.com> wrote:

> I dislike the precedent this sets.  Sendfile is a disgusting API and
> we're propagating through the tree.  Further propagation is sure to
> come, and further disgusting APIs will follow in its path.
>
> I'm not vetoing this, just registering a deep objection.  If others
> agree this should go forward, I will recuse myself. It seems the
> choice is clean code or happy users. I don't like that choice.
>
> -rob
>

[bradfitz, 2011-05-24 00:38:36]

Rob,

Ignoring the io pkg changes, what are your thoughts on just the net pkg
changes in this CL?

Notably, the addition of this:

    func (c *TCPConn) SendFile(f *os.File, n int64) (written int64, err
os.Error)

How does that rate on the ugliness scale?

- Brad

On Mon, May 23, 2011 at 5:23 PM, Rob 'Commander' Pike <r@google.com> wrote:

> I dislike the precedent this sets.  Sendfile is a disgusting API and
> we're propagating through the tree.  Further propagation is sure to
> come, and further disgusting APIs will follow in its path.
>
> I'm not vetoing this, just registering a deep objection.  If others
> agree this should go forward, I will recuse myself. It seems the
> choice is clean code or happy users. I don't like that choice.
>
> -rob
>

[r2, 2011-05-24 00:39:10]

On Tue, May 24, 2011 at 10:34 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> A systems programming language but we can't get too close to the system if
> it gets ugly?

Correct. Cleanliness matters even - especially! - when we want to get
cozy with the muck.  If that weren't true we'd still be using
assembler.

> Sendfile fwiw is real & makes a difference.  Let's try to find a way to make
> it not offensive.

As I've said before, every feature is useful, but not every feature
belongs.  I'm just putting backpressure on the feature set.  If we can
find a way to make the API clean, I'll be happy. So yes please on
finding a better API.

-rob

[r, 2011-05-24 00:44:37]

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

http://codereview.appspot.com/4538088/diff/5001/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:17: // SendFile sends f to c using sendfile(2),
bypassing userspace copies,
sendfile(2) is jargon.  "the Linux sendfile system call".
"userspace copy" is clear but not accurate - the copies are across the
user/kernel boundary. how about "unnecessary copies"?

http://codereview.appspot.com/4538088/diff/5001/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:18: // if possible. If n >= 0, it specifies the
max number of bytes to copy.
s/max/maximum/

http://codereview.appspot.com/4538088/diff/5001/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:19: // If n < 0, it means until EOF. SendFile
returns the number of bytes copied
s/it means/Sendfile copies/

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

[r2, 2011-05-24 00:46:09]

Why does sendfile_stub.go not implement Sendfile?

I would expect the design to be to add the peculiar interface and make
it work everywhere, but exploit the syscall where available.

-rob

[bradfitz, 2011-05-24 00:50:25]

On Mon, May 23, 2011 at 5:45 PM, Rob 'Commander' Pike <r@google.com> wrote:

> Why does sendfile_stub.go not implement Sendfile?
>
> I would expect the design to be to add the peculiar interface and make
> it work everywhere, but exploit the syscall where available.
>

Fair enough.  I did it in ReadFrom instead, but yeah, it makes more sense
here.

[bradfitz, 2011-05-24 00:57:23]

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

http://codereview.appspot.com/4538088/diff/5001/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:17: // SendFile sends f to c using sendfile(2),
bypassing userspace copies,
On 2011/05/24 00:44:37, r wrote:
> sendfile(2) is jargon.  "the Linux sendfile system call".
> "userspace copy" is clear but not accurate - the copies are across the
> user/kernel boundary. how about "unnecessary copies"?

Done.

http://codereview.appspot.com/4538088/diff/5001/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:18: // if possible. If n >= 0, it specifies the
max number of bytes to copy.
On 2011/05/24 00:44:37, r wrote:
> s/max/maximum/

Done.

http://codereview.appspot.com/4538088/diff/5001/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:19: // If n < 0, it means until EOF. SendFile
returns the number of bytes copied
On 2011/05/24 00:44:37, r wrote:
> s/it means/Sendfile copies/

Done.

http://codereview.appspot.com/4538088/diff/5001/src/pkg/net/sendfile_linux.go...
src/pkg/net/sendfile_linux.go:35: sn, err := syscall.Sendfile(destfd, srcfd,
(*int64)(unsafe.Pointer(nil)), int(toCopy))
On 2011/05/24 00:44:37, r wrote:
> s/err/errno/

Done.

[bradfitz, 2011-05-24 00:57:33]

Hello rsc@golang.org, iant@golang.org, r@google.com, r@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2011-05-24 03:00:29]

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

http://codereview.appspot.com/4538088/diff/9002/src/pkg/io/io.go#newcode223
src/pkg/io/io.go:223: // the copy is implemented by calling dst.ReadFrom(src).
or ReadNFrom

http://codereview.appspot.com/4538088/diff/9002/src/pkg/io/io.go#newcode228
src/pkg/io/io.go:228: if rt, hasReader := dst.(ReaderNFrom); hasReader {
this hasReader shadows the other one.

http://codereview.appspot.com/4538088/diff/9002/src/pkg/io/io.go#newcode243
src/pkg/io/io.go:243: func BufferedCopyn(dst Writer, src Reader, n int64)
(written int64, err os.Error) {
doc comment.

i'm not happy to see BufferedCopy arrive.  function names shouldn't have to
advertise their implementation.

http://codereview.appspot.com/4538088/diff/9002/src/pkg/net/sendfile_stub.go
File src/pkg/net/sendfile_stub.go (right):

http://codereview.appspot.com/4538088/diff/9002/src/pkg/net/sendfile_stub.go#...
src/pkg/net/sendfile_stub.go:13: func (c *TCPConn) SendFile(f *os.File, n int64)
(int64, os.Error) {
this is no longer a stub. the file should be sendfile.go.

http://codereview.appspot.com/4538088/diff/9002/src/pkg/net/sendfile_stub.go#...
src/pkg/net/sendfile_stub.go:13: func (c *TCPConn) SendFile(f *os.File, n int64)
(int64, os.Error) {
shouldn't there be a doc comment here?

http://codereview.appspot.com/4538088/diff/9002/src/pkg/net/sendfile_stub.go#...
src/pkg/net/sendfile_stub.go:17: return io.BufferedCopyn(c, r, n)
why doesn't BufferedCopyn have the same if statement? then you could avoid it
here and every other call site.

[rsc, 2011-05-24 03:12:53]

I'd still like to find a way that involves touching less code.
Another possibility is to define a by-convention method

FileIO() *os.File

that returns an os.File if there's one that you can use
for direct I/O instead of the receiver, or nil otherwise.
(Maybe the File method on TCPConn etc is good enough;
maybe not.)

Then if both arguments have FileIO methods, Copy and Copyn
can hand off to os.Sendfile, without having to introduce any
new interfaces or functions in package io.  Or maybe the
current code is better.  I'd feel a lot better without ReaderNFrom
though.

I'll think more about it overnight.

[bradfitz, 2011-05-24 04:52:10]

On Mon, May 23, 2011 at 8:12 PM, Russ Cox <rsc@golang.org> wrote:

> I'd still like to find a way that involves touching less code.
> Another possibility is to define a by-convention method
>
> FileIO() *os.File
>
> that returns an os.File if there's one that you can use
> for direct I/O instead of the receiver, or nil otherwise.
> (Maybe the File method on TCPConn etc is good enough;
> maybe not.)
>
> Then if both arguments have FileIO methods, Copy and Copyn
> can hand off to os.Sendfile, without having to introduce any
> new interfaces or functions in package io.  Or maybe the
> current code is better.  I'd feel a lot better without ReaderNFrom
> though.


That's kinda how my first version worked:
http://codereview.appspot.com/4528084/

I'll think more about it overnight.


k.  I'm continue to think on it too, trying to find a solution everybody
likes.

What are your thoughts on either/both of the following:

* exposing LimitReader as a public type/superset interface with a
BytesRemaining() int64 method

* using a magic error value (ErrDeclined) that io.Copy[n] respect to mean
fall through to the next strategy.  (avoiding exposing e.g.
io.BufferedCopy[n] helpers for ReadFroms that fail to sendfile)

[rsc, 2011-05-24 17:44:12]

On Mon, May 23, 2011 at 20:34, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> A systems programming language but we can't get too close to the system if
> it gets ugly?

For what it's worth, I don't think anyone said that.

What is important is that the ugliness of the underlying
system is not allowed to show through.  The details of
the ugliness tend to change; baking them into your
APIs is a recipe for being out of date (and ugly on top of that).
Consider how the net package hides the ugliness of both
non-blocking I/O and sockets.  If Linux comes out with
eeepoll, we won't have to change any visible Go APIs.
If someone adds a new socket address type, net.Dial
doesn't change.

> Sendfile fwiw is real & makes a difference.

Sure, but it's okay to omit until we have a clean way
to do it (as we have omitted it until now).  We're usually
willing to sacrifice some performance or even functionality
now in order to preserve the ability to do it right later.

Russ