|
|
Descriptionnet/http: add setCork function
Largely stolen from the go.net repo.
Only on Linux for now. FreeBSD 4.5+ could come later.
Not in package net, as to not further contribute API to that
cesspool.
Future CL will use these.
Update Issue 5352
Patch Set 1 #Patch Set 2 : diff -r 8f1fb6b6f141 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 8f1fb6b6f141 https://go.googlecode.com/hg/ #Patch Set 4 : diff -r 8f1fb6b6f141 https://go.googlecode.com/hg/ #
Total comments: 1
MessagesTotal messages: 13
Hello pabuhr@google.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
CL updated with a test. On Tue, May 14, 2013 at 8:10 AM, <bradfitz@golang.org> wrote: > Reviewers: pabuhr, > > Message: > Hello pabuhr@google.com (cc: golang-dev@googlegroups.com), > > I'd like you to review this change to > https://go.googlecode.com/hg/ > > > Description: > net/http: add setCork function > > Largely stolen from the go.net repo. > > Only on Linux for now. FreeBSD 4.5+ could come later. > > Not in package net, as to not further contribute API to that > cesspool. > > Future CL will use these. > > Update Issue 5352 > > Please review this at https://codereview.appspot.**com/9045045/<https://codereview.appspot.com/9045... > > Affected files: > A src/pkg/net/http/cork_linux.go > A src/pkg/net/http/cork_stub.go > > > Index: src/pkg/net/http/cork_linux.go > ==============================**==============================**======= > new file mode 100644 > --- /dev/null > +++ b/src/pkg/net/http/cork_linux.**go > @@ -0,0 +1,44 @@ > +// Copyright 2013 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 http > + > +import ( > + "errors" > + "net" > + "reflect" > + "syscall" > +) > + > +var errCorkReflect = errors.New("http: failed to reflect enough to cork") > + > +func setCork(c net.Conn, v bool) error { > + _, ok := c.(*net.TCPConn) > + if !ok { > + return nil > + } > + fd, err := sysfd(c) > + if err != nil { > + return err > + } > + vint := 0 > + if v { > + vint = 1 > + } > + return syscall.SetsockoptInt(fd, syscall.SOL_TCP, > syscall.TCP_CORK, vint) > +} > + > +func sysfd(c net.Conn) (int, error) { > + cv := reflect.ValueOf(c) > + switch ce := cv.Elem(); ce.Kind() { > + case reflect.Struct: > + netfd := ce.FieldByName("conn").**FieldByName("fd") > + switch fe := netfd.Elem(); fe.Kind() { > + case reflect.Struct: > + fd := fe.FieldByName("sysfd") > + return int(fd.Int()), nil > + } > + } > + return 0, errCorkReflect > +} > Index: src/pkg/net/http/cork_stub.go > ==============================**==============================**======= > new file mode 100644 > --- /dev/null > +++ b/src/pkg/net/http/cork_stub.**go > @@ -0,0 +1,18 @@ > +// Copyright 2013 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. > + > +// +build !linux > + > +package http > + > +import ( > + "errors" > + "net" > +) > + > +var errNoCork = errors.New("http: cork support not available") > + > +func setCork(c net.Conn, v bool) error { > + return errNoCork > +} > > >
Sign in to reply to this message.
What is TCP_CORK? Also as you note, net is already a cesspool. Should we let it accumulate more fecal matter rather than spread the mess to other packages? -rob
Sign in to reply to this message.
This was discussed in the meeting yesterday. On Tue, May 14, 2013 at 9:23 AM, Rob Pike <r@golang.org> wrote: > What is TCP_CORK? > > Also as you note, net is already a cesspool. Should we let it > accumulate more fecal matter rather than spread the mess to other > packages? > > -rob >
Sign in to reply to this message.
Background for this CL: Peter proposed https://codereview.appspot.com/9333044/ adding a package net TCPConn.SetCork function to the net package. Rob and Russ expressed disgust and sadness at how the net package has grown, hiding its true inner beauty of net.Dial and net.Listen in a sea of sockopts. I asked whether https://codereview.appspot.com/9333044/ should be in package net/http instead. Russ said approximately: "I'd rather not see it at all, but if we need it anyway, yes, hide it in http so it doesn't add more crap to package net." Hence this CL. TCP_CORK turns on buffering in the kernel, making sure only full packages get sent. It lets you do: setsockopt(fd, TCP_CORK, on) write(fd, "some header") sendfile(fd, some_file_fd) setsockopt(fd, TCP_CORK, off) // flush ... and only send one packet, instead of two. It differs from TCP_NOPUSH in that TCP_CORK on the 1->0 transition causes a flush. FreeBSD 4.5+ has that behavior too. I don't care where this goes. I actually don't really even care about this CL. On Tue, May 14, 2013 at 8:10 AM, <bradfitz@golang.org> wrote: > Reviewers: pabuhr, > > Message: > Hello pabuhr@google.com (cc: golang-dev@googlegroups.com), > > I'd like you to review this change to > https://go.googlecode.com/hg/ > > > Description: > net/http: add setCork function > > Largely stolen from the go.net repo. > > Only on Linux for now. FreeBSD 4.5+ could come later. > > Not in package net, as to not further contribute API to that > cesspool. > > Future CL will use these. > > Update Issue 5352 > > Please review this at https://codereview.appspot.**com/9045045/<https://codereview.appspot.com/9045... > > Affected files: > A src/pkg/net/http/cork_linux.go > A src/pkg/net/http/cork_stub.go > > > Index: src/pkg/net/http/cork_linux.go > ==============================**==============================**======= > new file mode 100644 > --- /dev/null > +++ b/src/pkg/net/http/cork_linux.**go > @@ -0,0 +1,44 @@ > +// Copyright 2013 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 http > + > +import ( > + "errors" > + "net" > + "reflect" > + "syscall" > +) > + > +var errCorkReflect = errors.New("http: failed to reflect enough to cork") > + > +func setCork(c net.Conn, v bool) error { > + _, ok := c.(*net.TCPConn) > + if !ok { > + return nil > + } > + fd, err := sysfd(c) > + if err != nil { > + return err > + } > + vint := 0 > + if v { > + vint = 1 > + } > + return syscall.SetsockoptInt(fd, syscall.SOL_TCP, > syscall.TCP_CORK, vint) > +} > + > +func sysfd(c net.Conn) (int, error) { > + cv := reflect.ValueOf(c) > + switch ce := cv.Elem(); ce.Kind() { > + case reflect.Struct: > + netfd := ce.FieldByName("conn").**FieldByName("fd") > + switch fe := netfd.Elem(); fe.Kind() { > + case reflect.Struct: > + fd := fe.FieldByName("sysfd") > + return int(fd.Int()), nil > + } > + } > + return 0, errCorkReflect > +} > Index: src/pkg/net/http/cork_stub.go > ==============================**==============================**======= > new file mode 100644 > --- /dev/null > +++ b/src/pkg/net/http/cork_stub.**go > @@ -0,0 +1,18 @@ > +// Copyright 2013 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. > + > +// +build !linux > + > +package http > + > +import ( > + "errors" > + "net" > +) > + > +var errNoCork = errors.New("http: cork support not available") > + > +func setCork(c net.Conn, v bool) error { > + return errNoCork > +} > > >
Sign in to reply to this message.
I didn't hear Russ say that. Personally, I'd rather keep the mess in one place rather than splashing net/http, which is still pretty clean, with overflow from that cesspool, to use your metaphor. For example, why does HTTP want it but not raw TCP? -rob
Sign in to reply to this message.
Russ: make the call. On Tue, May 14, 2013 at 9:49 AM, Rob Pike <r@golang.org> wrote: > I didn't hear Russ say that. Personally, I'd rather keep the mess in > one place rather than splashing net/http, which is still pretty clean, > with overflow from that cesspool, to use your metaphor. For example, > why does HTTP want it but not raw TCP? > > -rob >
Sign in to reply to this message.
I don't want this in the library at all. But if it's here, it should not result in new API in net nor in net/http, so that some day we can remove it without breaking anyone. That means it should be done the way Brad's CL does it. Russ
Sign in to reply to this message.
i remain unhappy but will LGTM if rsc agrees. sockets are just foul https://codereview.appspot.com/9045045/diff/9001/src/pkg/net/http/cork_linux.go File src/pkg/net/http/cork_linux.go (right): https://codereview.appspot.com/9045045/diff/9001/src/pkg/net/http/cork_linux.... src/pkg/net/http/cork_linux.go:14: var errCorkReflect = errors.New("http: failed to reflect enough to cork") this message describes the process that failed instead of what went wrong maybe: http: unable to get file descriptor for TCP_CORK this is also the first use of reflect in this package. since http uses everything already, at least indirectly, i don't think that matters but it's worth noting.
Sign in to reply to this message.
I don't understand how far we are willing to go for how much performance on what benchmark. We could start writing header parsing in assembly, or JITing it somehow, and I am sure that would help performance too. But I think those are pretty clearly off limits. Where is the boundary? TCP_CORK is a great example of bad performance hacks spawning more bad performance hacks. The only reason TCP_CORK is ever useful compared to just buffering your own writes is that someone introduced a different performance hack that took away control over your own writes (sendfile). So now to force buffering across the boundary between user-generated writes and sendfile-generated writes, we have to introduce yet another clumsy hack. At least we managed to add sendfile behind the scenes, without bothering any public API, in a way that people just get it for free and don't need to know or care (except when the implementation is buggy), and in a way that applies to all network connections, not just http. As I said before, if we are going to do TCP_CORK, this CL is the right way to do it, at least for now, because it avoids committing to any public API and can be backed out without breaking any code. But should we do it? It depends. Is it getting 1% on a microbenchmark we don't really care about? Then no. Is it getting 50% on every possible benchmark scenario? Then yes. I assume it is somewhere in the middle. Where exactly is it? Russ
Sign in to reply to this message.
Peter? On Tue, May 14, 2013 at 12:59 PM, Russ Cox <rsc@golang.org> wrote: > I don't understand how far we are willing to go for how much performance > on what benchmark. We could start writing header parsing in assembly, or > JITing it somehow, and I am sure that would help performance too. But I > think those are pretty clearly off limits. Where is the boundary? > > TCP_CORK is a great example of bad performance hacks spawning more bad > performance hacks. The only reason TCP_CORK is ever useful compared to just > buffering your own writes is that someone introduced a different > performance hack that took away control over your own writes (sendfile). So > now to force buffering across the boundary between user-generated writes > and sendfile-generated writes, we have to introduce yet another clumsy > hack. > > At least we managed to add sendfile behind the scenes, without bothering > any public API, in a way that people just get it for free and don't need to > know or care (except when the implementation is buggy), and in a way that > applies to all network connections, not just http. > > As I said before, if we are going to do TCP_CORK, this CL is the right way > to do it, at least for now, because it avoids committing to any public API > and can be backed out without breaking any code. But should we do it? It > depends. Is it getting 1% on a microbenchmark we don't really care about? > Then no. Is it getting 50% on every possible benchmark scenario? Then yes. > > I assume it is somewhere in the middle. Where exactly is it? > > Russ >
Sign in to reply to this message.
Russ, let me run some experiments to see if I can answer your question. I'll put Brad's CL into my version of Go, and do some local and borg runs. Clearly, the proof is in the pudding. With respect to corking and sendfile, the issue is that sendfile is sending the file directly from the file-system cache (I know you know that), so there is no opportunity to concatenate the header onto this data. As a result, there is often a separate packet sent for the header before the data packets are sent. For smallish files this can increase traffic. Sendfile is usually a big win over user buffering for static content because of the lack of copying (again, you know this). So think of this as a design document, and I'll try to get something back to you quickly to justify or refute the idea. BTW, that's why I'm here! ;-) On Tue, May 14, 2013 at 12:59 PM, Russ Cox <rsc@golang.org> wrote: > I don't understand how far we are willing to go for how much performance > on what benchmark. We could start writing header parsing in assembly, or > JITing it somehow, and I am sure that would help performance too. But I > think those are pretty clearly off limits. Where is the boundary? > > TCP_CORK is a great example of bad performance hacks spawning more bad > performance hacks. The only reason TCP_CORK is ever useful compared to just > buffering your own writes is that someone introduced a different > performance hack that took away control over your own writes (sendfile). So > now to force buffering across the boundary between user-generated writes > and sendfile-generated writes, we have to introduce yet another clumsy > hack. > > At least we managed to add sendfile behind the scenes, without bothering > any public API, in a way that people just get it for free and don't need to > know or care (except when the implementation is buggy), and in a way that > applies to all network connections, not just http. > > As I said before, if we are going to do TCP_CORK, this CL is the right way > to do it, at least for now, because it avoids committing to any public API > and can be backed out without breaking any code. But should we do it? It > depends. Is it getting 1% on a microbenchmark we don't really care about? > Then no. Is it getting 50% on every possible benchmark scenario? Then yes. > > I assume it is somewhere in the middle. Where exactly is it? > > Russ >
Sign in to reply to this message.
|