http2: reduce garbage in Server on requests with bodies

It was allocating 64k of garbage per request.

benchmark                         old ns/op     new ns/op     delta
BenchmarkServer_GetRequest-2      234445        236751        +0.98%
BenchmarkServer_PostRequest-2     284443        263019        -7.53%

benchmark                         old allocs     new allocs     delta
BenchmarkServer_GetRequest-2      25             25             +0.00%
BenchmarkServer_PostRequest-2     31             30             -3.23%

benchmark                         old bytes     new bytes     delta
BenchmarkServer_GetRequest-2      1540          1540          +0.00%
BenchmarkServer_PostRequest-2     67658         1800          -97.34%

Change-Id: I9deee0ad2fddecd65c82e40782f61da0217735fc
Reviewed-on: https://go-review.googlesource.com/20542
Reviewed-by: Blake Mizerany <blake.mizerany@gmail.com>
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

http2/server.go

@@ -409,6 +409,7 @@ type serverConn struct {
 	goAwayCode            ErrCode
 	shutdownTimerCh       <-chan time.Time // nil until used
 	shutdownTimer         *time.Timer      // nil until used
+	freeRequestBodyBuf    []byte           // if non-nil, a free initialWindowSize buffer for getRequestBodyBuf
 
 	// Owned by the writeFrameAsync goroutine:
 	headerWriteBuf bytes.Buffer
@@ -453,6 +454,7 @@ type stream struct {
 	sentReset        bool // only true once detached from streams map
 	gotReset         bool // only true once detacted from streams map
 	gotTrailerHeader bool // HEADER frame for trailers was seen
+	reqBuf           []byte
 
 	trailer    http.Header // accumulated trailers
 	reqTrailer http.Header // handler's Request.Trailer
@@ -1176,6 +1178,18 @@ func (sc *serverConn) closeStream(st *stream, err error) {
 	}
 	st.cw.Close() // signals Handler's CloseNotifier, unblocks writes, etc
 	sc.writeSched.forgetStream(st.id)
+	if st.reqBuf != nil {
+		// Stash this request body buffer (64k) away for reuse
+		// by a future POST/PUT/etc.
+		//
+		// TODO(bradfitz): share on the server? sync.Pool?
+		// Server requires locks and might hurt contention.
+		// sync.Pool might work, or might be worse, depending
+		// on goroutine CPU migrations. (get and put on
+		// separate CPUs).  Maybe a mix of strategies. But
+		// this is an easy win for now.
+		sc.freeRequestBodyBuf = st.reqBuf
+	}
 }
 
 func (sc *serverConn) processSettings(f *SettingsFrame) error {
@@ -1602,8 +1616,9 @@ func (sc *serverConn) newWriterAndRequest(st *stream, f *MetaHeadersFrame) (*res
 		Trailer:    trailer,
 	}
 	if bodyOpen {
+		st.reqBuf = sc.getRequestBodyBuf()
 		body.pipe = &pipe{
-			b: &fixedBuffer{buf: make([]byte, initialWindowSize)}, // TODO: garbage
+			b: &fixedBuffer{buf: st.reqBuf},
 		}
 
 		if vv, ok := header["Content-Length"]; ok {
@@ -1627,6 +1642,15 @@ func (sc *serverConn) newWriterAndRequest(st *stream, f *MetaHeadersFrame) (*res
 	return rw, req, nil
 }
 
+func (sc *serverConn) getRequestBodyBuf() []byte {
+	sc.serveG.check()
+	if buf := sc.freeRequestBodyBuf; buf != nil {
+		sc.freeRequestBodyBuf = nil
+		return buf
+	}
+	return make([]byte, initialWindowSize)
+}
+
 // Run on its own goroutine.
 func (sc *serverConn) runHandler(rw *responseWriter, req *http.Request, handler func(http.ResponseWriter, *http.Request)) {
 	didPanic := true

http2/server_test.go

@@ -204,6 +204,13 @@ func (st *serverTester) awaitIdle() {
 }
 
 func (st *serverTester) Close() {
+	if st.t.Failed() {
+		// If we failed already (and are likely in a Fatal,
+		// unwindowing), force close the connection, so the
+		// httptest.Server doesn't wait forever for the conn
+		// to close.
+		st.cc.Close()
+	}
 	st.ts.Close()
 	if st.cc != nil {
 		st.cc.Close()
@@ -2759,6 +2766,7 @@ func TestServerDoesntWriteInvalidHeaders(t *testing.T) {
 }
 
 func BenchmarkServerGets(b *testing.B) {
+	defer disableGoroutineTracking()()
 	b.ReportAllocs()
 
 	const msg = "Hello, world"
@@ -2790,6 +2798,7 @@ func BenchmarkServerGets(b *testing.B) {
 }
 
 func BenchmarkServerPosts(b *testing.B) {
+	defer disableGoroutineTracking()()
 	b.ReportAllocs()
 
 	const msg = "Hello, world"
@@ -3002,6 +3011,76 @@ func TestServerNoDuplicateContentType(t *testing.T) {
 	}
 }
 
+func disableGoroutineTracking() (restore func()) {
+	old := DebugGoroutines
+	DebugGoroutines = false
+	return func() { DebugGoroutines = old }
+}
+
+func BenchmarkServer_GetRequest(b *testing.B) {
+	defer disableGoroutineTracking()()
+	b.ReportAllocs()
+	const msg = "Hello, world."
+	st := newServerTester(b, func(w http.ResponseWriter, r *http.Request) {
+		n, err := io.Copy(ioutil.Discard, r.Body)
+		if err != nil || n > 0 {
+			b.Error("Read %d bytes, error %v; want 0 bytes.", n, err)
+		}
+		io.WriteString(w, msg)
+	})
+	defer st.Close()
+
+	st.greet()
+	// Give the server quota to reply. (plus it has the the 64KB)
+	if err := st.fr.WriteWindowUpdate(0, uint32(b.N*len(msg))); err != nil {
+		b.Fatal(err)
+	}
+	hbf := st.encodeHeader(":method", "GET")
+	for i := 0; i < b.N; i++ {
+		streamID := uint32(1 + 2*i)
+		st.writeHeaders(HeadersFrameParam{
+			StreamID:      streamID,
+			BlockFragment: hbf,
+			EndStream:     true,
+			EndHeaders:    true,
+		})
+		st.wantHeaders()
+		st.wantData()
+	}
+}
+
+func BenchmarkServer_PostRequest(b *testing.B) {
+	defer disableGoroutineTracking()()
+	b.ReportAllocs()
+	const msg = "Hello, world."
+	st := newServerTester(b, func(w http.ResponseWriter, r *http.Request) {
+		n, err := io.Copy(ioutil.Discard, r.Body)
+		if err != nil || n > 0 {
+			b.Error("Read %d bytes, error %v; want 0 bytes.", n, err)
+		}
+		io.WriteString(w, msg)
+	})
+	defer st.Close()
+	st.greet()
+	// Give the server quota to reply. (plus it has the the 64KB)
+	if err := st.fr.WriteWindowUpdate(0, uint32(b.N*len(msg))); err != nil {
+		b.Fatal(err)
+	}
+	hbf := st.encodeHeader(":method", "POST")
+	for i := 0; i < b.N; i++ {
+		streamID := uint32(1 + 2*i)
+		st.writeHeaders(HeadersFrameParam{
+			StreamID:      streamID,
+			BlockFragment: hbf,
+			EndStream:     false,
+			EndHeaders:    true,
+		})
+		st.writeData(streamID, true, nil)
+		st.wantHeaders()
+		st.wantData()
+	}
+}
+
 type connStateConn struct {
 	net.Conn
 	cs tls.ConnectionState