From 07a22bbc11de6c8cdac599f59ef00f019d22ff67 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Fri, 28 Apr 2017 16:46:18 +0000 Subject: [PATCH] net/http: re-simplify HTTP/1.x status line writing MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit It used to be simple, and then it got complicated for speed (to reduce allocations, mostly), but that involved a mutex and hurt multi-core performance, contending on the mutex. A change was sent to try to improve that mutex contention in https://go-review.googlesource.com/c/42110/2/src/net/http/server.go but that introduced its own allocations (the string->interface{} boxing for the sync.Map key), which runs counter to the whole point of that statusLine function: to remove allocations. Instead, make the code simple again and not have a mutex. It's a bit slower for the single-core case, but nobody with a single-user HTTP server cares about 50 nanoseconds: name old time/op new time/op delta ResponseStatusLine 37.5ns ± 2% 87.1ns ± 2% +132.42% (p=0.029 n=4+4) ResponseStatusLine-2 63.1ns ± 1% 43.1ns ±12% -31.67% (p=0.029 n=4+4) ResponseStatusLine-4 53.8ns ± 8% 40.2ns ± 2% -25.29% (p=0.029 n=4+4) name old alloc/op new alloc/op delta ResponseStatusLine 0.00B ±NaN% 0.00B ±NaN% ~ (all samples are equal) ResponseStatusLine-2 0.00B ±NaN% 0.00B ±NaN% ~ (all samples are equal) ResponseStatusLine-4 0.00B ±NaN% 0.00B ±NaN% ~ (all samples are equal) name old allocs/op new allocs/op delta ResponseStatusLine 0.00 ±NaN% 0.00 ±NaN% ~ (all samples are equal) ResponseStatusLine-2 0.00 ±NaN% 0.00 ±NaN% ~ (all samples are equal) ResponseStatusLine-4 0.00 ±NaN% 0.00 ±NaN% ~ (all samples are equal) (Note the code could be even simpler with fmt.Fprintf, but that is relatively slow and involves a bunch of allocations getting arguments into interface{} for the call) Change-Id: I1fa119132dbbf97a8e7204ce3e0707d433060da2 Reviewed-on: https://go-review.googlesource.com/42133 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Bryan Mills --- src/net/http/export_test.go | 26 +++++++------- src/net/http/serve_test.go | 11 ++++++ src/net/http/server.go | 67 ++++++++++++------------------------- 3 files changed, 46 insertions(+), 58 deletions(-) diff --git a/src/net/http/export_test.go b/src/net/http/export_test.go index 596171f5f0..98fb0834dd 100644 --- a/src/net/http/export_test.go +++ b/src/net/http/export_test.go @@ -17,17 +17,19 @@ import ( ) var ( - DefaultUserAgent = defaultUserAgent - NewLoggingConn = newLoggingConn - ExportAppendTime = appendTime - ExportRefererForURL = refererForURL - ExportServerNewConn = (*Server).newConn - ExportCloseWriteAndWait = (*conn).closeWriteAndWait - ExportErrRequestCanceled = errRequestCanceled - ExportErrRequestCanceledConn = errRequestCanceledConn - ExportServeFile = serveFile - ExportScanETag = scanETag - ExportHttp2ConfigureServer = http2ConfigureServer + DefaultUserAgent = defaultUserAgent + NewLoggingConn = newLoggingConn + ExportAppendTime = appendTime + ExportRefererForURL = refererForURL + ExportServerNewConn = (*Server).newConn + ExportCloseWriteAndWait = (*conn).closeWriteAndWait + ExportErrRequestCanceled = errRequestCanceled + ExportErrRequestCanceledConn = errRequestCanceledConn + ExportServeFile = serveFile + ExportScanETag = scanETag + ExportHttp2ConfigureServer = http2ConfigureServer + Export_shouldCopyHeaderOnRedirect = shouldCopyHeaderOnRedirect + Export_writeStatusLine = writeStatusLine ) func init() { @@ -188,8 +190,6 @@ func ExportHttp2ConfigureTransport(t *Transport) error { return nil } -var Export_shouldCopyHeaderOnRedirect = shouldCopyHeaderOnRedirect - func (s *Server) ExportAllConnsIdle() bool { s.mu.Lock() defer s.mu.Unlock() diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index e140721c91..0a7459a0dc 100644 --- a/src/net/http/serve_test.go +++ b/src/net/http/serve_test.go @@ -5539,3 +5539,14 @@ func TestServerValidatesMethod(t *testing.T) { } } } + +func BenchmarkResponseStatusLine(b *testing.B) { + b.ReportAllocs() + b.RunParallel(func(pb *testing.PB) { + bw := bufio.NewWriter(ioutil.Discard) + var buf3 [3]byte + for pb.Next() { + Export_writeStatusLine(bw, true, 200, buf3[:]) + } + }) +} diff --git a/src/net/http/server.go b/src/net/http/server.go index a9d7396106..3cb490d8a7 100644 --- a/src/net/http/server.go +++ b/src/net/http/server.go @@ -439,9 +439,10 @@ type response struct { handlerDone atomicBool // set true when the handler exits - // Buffers for Date and Content-Length - dateBuf [len(TimeFormat)]byte - clenBuf [10]byte + // Buffers for Date, Content-Length, and status code + dateBuf [len(TimeFormat)]byte + clenBuf [10]byte + statusBuf [3]byte // closeNotifyCh is the channel returned by CloseNotify. // TODO(bradfitz): this is currently (for Go 1.8) always @@ -1379,7 +1380,7 @@ func (cw *chunkWriter) writeHeader(p []byte) { } } - w.conn.bufw.WriteString(statusLine(w.req, code)) + writeStatusLine(w.conn.bufw, w.req.ProtoAtLeast(1, 1), code, w.statusBuf[:]) cw.header.WriteSubset(w.conn.bufw, excludeHeader) setHeader.Write(w.conn.bufw) w.conn.bufw.Write(crlf) @@ -1403,49 +1404,25 @@ func foreachHeaderElement(v string, fn func(string)) { } } -// statusLines is a cache of Status-Line strings, keyed by code (for -// HTTP/1.1) or negative code (for HTTP/1.0). This is faster than a -// map keyed by struct of two fields. This map's max size is bounded -// by 2*len(statusText), two protocol types for each known official -// status code in the statusText map. -var ( - statusMu sync.RWMutex - statusLines = make(map[int]string) -) - -// statusLine returns a response Status-Line (RFC 2616 Section 6.1) -// for the given request and response status code. -func statusLine(req *Request, code int) string { - // Fast path: - key := code - proto11 := req.ProtoAtLeast(1, 1) - if !proto11 { - key = -key - } - statusMu.RLock() - line, ok := statusLines[key] - statusMu.RUnlock() - if ok { - return line - } - - // Slow path: - proto := "HTTP/1.0" - if proto11 { - proto = "HTTP/1.1" - } - codestring := fmt.Sprintf("%03d", code) - text, ok := statusText[code] - if !ok { - text = "status code " + codestring +// writeStatusLine writes an HTTP/1.x Status-Line (RFC 2616 Section 6.1) +// to bw. is11 is whether the HTTP request is HTTP/1.1. false means HTTP/1.0. +// code is the response status code. +// scratch is an optional scratch buffer. If it has at least capacity 3, it's used. +func writeStatusLine(bw *bufio.Writer, is11 bool, code int, scratch []byte) { + if is11 { + bw.WriteString("HTTP/1.1 ") + } else { + bw.WriteString("HTTP/1.0 ") } - line = proto + " " + codestring + " " + text + "\r\n" - if ok { - statusMu.Lock() - defer statusMu.Unlock() - statusLines[key] = line + if text, ok := statusText[code]; ok { + bw.Write(strconv.AppendInt(scratch[:0], int64(code), 10)) + bw.WriteByte(' ') + bw.WriteString(text) + bw.WriteString("\r\n") + } else { + // don't worry about performance + fmt.Fprintf(bw, "%03d status code %d\r\n", code, code) } - return line } // bodyAllowed reports whether a Write is allowed for this response type. -- 2.48.1