code review 8038047: net/http: server optimization; reduce GCs, generate ~ha...

net/http: server optimization; reduce GCs, generate ~half the garbage

There was another bufio.Writer not being reused, found with
GOGC=off and -test.memprofile.

benchmark                               old ns/op    new ns/op    delta
BenchmarkServerFakeConnWithKeepAlive        18270        16046  -12.17%

benchmark                              old allocs   new allocs    delta
BenchmarkServerFakeConnWithKeepAlive           38           36   -5.26%

benchmark                               old bytes    new bytes    delta
BenchmarkServerFakeConnWithKeepAlive         4598         2488  -45.89%

Update issue 5100

[bradfitz, 2013-03-27 06:32:30]

Hello golang-dev@googlegroups.com,

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

[gri, 2013-03-27 17:58:25]

LGTM

but i am not familiar with the details

[bradfitz, 2013-03-27 20:35:51]

*** Submitted as https://code.google.com/p/go/source/detail?r=6fa8c13f4f93 ***

net/http: server optimization; reduce GCs, generate ~half the garbage

There was another bufio.Writer not being reused, found with
GOGC=off and -test.memprofile.

benchmark                               old ns/op    new ns/op    delta
BenchmarkServerFakeConnWithKeepAlive        18270        16046  -12.17%

benchmark                              old allocs   new allocs    delta
BenchmarkServerFakeConnWithKeepAlive           38           36   -5.26%

benchmark                               old bytes    new bytes    delta
BenchmarkServerFakeConnWithKeepAlive         4598         2488  -45.89%

Update issue 5100

R=golang-dev, gri
CC=golang-dev
https://codereview.appspot.com/8038047

[adg1, 2013-03-27 22:13:27]

LGTM
On 27 Mar 2013 17:32, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http: server optimization; reduce GCs, generate ~half the garbage
>
> There was another bufio.Writer not being reused, found with
> GOGC=off and -test.memprofile.
>
> benchmark                               old ns/op    new ns/op    delta
> BenchmarkServerFakeConnWithKee**pAlive        18270        16046  -12.17%
>
> benchmark                              old allocs   new allocs    delta
> BenchmarkServerFakeConnWithKee**pAlive           38           36   -5.26%
>
> benchmark                               old bytes    new bytes    delta
> BenchmarkServerFakeConnWithKee**pAlive         4598         2488  -45.89%
>
> Update issue 5100
>
> Please review this at
https://codereview.appspot.**com/8038047/<https://codereview.appspot.com/8038...
>
> Affected files:
>   M src/pkg/net/http/server.go
>
>
> Index: src/pkg/net/http/server.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/server.go
> +++ b/src/pkg/net/http/server.go
> @@ -281,6 +281,7 @@
>
>         w  *bufio.Writer // buffers output in chunks to chunkWriter
>         cw *chunkWriter
> +       sw *switchWriter // of the bufio.Writer, for return to
> putBufioWriter
>
>         // handlerHeader is the Header that Handlers get access to,
>         // which may be retained and mutated even after WriteHeader.
> @@ -381,7 +382,7 @@
>         c.sr = liveSwitchReader{r: c.rwc}
>         c.lr = io.LimitReader(&c.sr, noLimit).(*io.LimitedReader)
>         br, sr := newBufioReader(c.lr)
> -       bw, sw := newBufioWriter(c.rwc)
> +       bw, sw := newBufioWriterSize(c.rwc, 4<<10)
>         c.buf = bufio.NewReadWriter(br, bw)
>         c.bufswr = sr
>         c.bufsww = sw
> @@ -402,10 +403,21 @@
>
>  // TODO: use a sync.Cache instead
>  var (
> -       bufioReaderCache = make(chan bufioReaderPair, 4)
> -       bufioWriterCache = make(chan bufioWriterPair, 4)
> +       bufioReaderCache   = make(chan bufioReaderPair, 4)
> +       bufioWriterCache2k = make(chan bufioWriterPair, 4)
> +       bufioWriterCache4k = make(chan bufioWriterPair, 4)
>  )
>
> +func bufioWriterCache(size int) chan bufioWriterPair {
> +       switch size {
> +       case 2 << 10:
> +               return bufioWriterCache2k
> +       case 4 << 10:
> +               return bufioWriterCache4k
> +       }
> +       return nil
> +}
> +
>  func newBufioReader(r io.Reader) (*bufio.Reader, *switchReader) {
>         select {
>         case p := <-bufioReaderCache:
> @@ -429,14 +441,14 @@
>         }
>  }
>
> -func newBufioWriter(w io.Writer) (*bufio.Writer, *switchWriter) {
> +func newBufioWriterSize(w io.Writer, size int) (*bufio.Writer,
> *switchWriter) {
>         select {
> -       case p := <-bufioWriterCache:
> +       case p := <-bufioWriterCache(size):
>                 p.sw.Writer = w
>                 return p.bw, p.sw
>         default:
>                 sw := &switchWriter{w}
> -               return bufio.NewWriter(sw), sw
> +               return bufio.NewWriterSize(sw, size), sw
>         }
>  }
>
> @@ -454,7 +466,7 @@
>         }
>         sw.Writer = nil
>         select {
> -       case bufioWriterCache <- bufioWriterPair{bw, sw}:
> +       case bufioWriterCache(bw.Available(**)) <- bufioWriterPair{bw,
> sw}:
>         default:
>         }
>  }
> @@ -540,7 +552,7 @@
>                 cw:            new(chunkWriter),
>         }
>         w.cw.res = w
> -       w.w = bufio.NewWriterSize(w.cw, bufferBeforeChunkingSize)
> +       w.w, w.sw = newBufioWriterSize(w.cw, bufferBeforeChunkingSize)
>         return w, nil
>  }
>
> @@ -802,6 +814,7 @@
>         }
>
>         w.w.Flush()
> +       putBufioWriter(w.w, w.sw)
>         w.cw.close()
>         w.conn.buf.Flush()
>
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>