net/http: re-simplify HTTP/1.x status line writing

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 <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>

diff --git a/src/net/http/export_test.go b/src/net/http/export_test.go
index 596171f5f034b7df2d72876340994bcd8ab79e3b..98fb0834ddcb94b54d96e82fc5632655a0b99426 100644 (file)
--- 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 e140721c91f52b29da6650159b31b4179ccecac1..0a7459a0dcd9aa7edc22ab7e4a9d776ce788ad8f 100644 (file)
--- 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 a9d739610640d17742b11a81f6caebc86d9475ab..3cb490d8a749647418acf0fab8a4c790bdc79201 100644 (file)
--- 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.