net/http: use a struct instead of a string in transport conn cache key

The Transport's idle connection cache is keyed by a string,
for pre-Go 1.0 reasons.  Ever since Go has been able to use
structs as map keys, there's been a TODO in the code to use
structs instead of allocating strings. This change does that.

Saves 3 allocatins and ~100 bytes of garbage per client
request. But because string hashing is so fast these days
(thanks, Keith), the performance is a wash: what we gain
on GC and not allocating, we lose in slower hashing. (hashing
structs of strings is slower than 1 string)

This seems a bit faster usually, but I've also seen it be a
bit slower. But at least it's how I've wanted it now, and it
the allocation improvements are consistent.

LGTM=adg
R=adg
CC=golang-codereviews
https://golang.org/cl/58260043

diff --git a/src/pkg/net/http/export_test.go b/src/pkg/net/http/export_test.go
index 0494991bde6d07ecd344fcdc455b42c608a50d7c..8074df5bbde17f3ffad29dd1f1fe51be3c772d18 100644 (file)
--- a/src/pkg/net/http/export_test.go
+++ b/src/pkg/net/http/export_test.go
@@ -32,7 +32,7 @@ func (t *Transport) IdleConnKeysForTesting() (keys []string) {
                return
        }
        for key := range t.idleConn {
-               keys = append(keys, key)
+               keys = append(keys, key.String())
        }
        return
 }
@@ -43,11 +43,12 @@ func (t *Transport) IdleConnCountForTesting(cacheKey string) int {
        if t.idleConn == nil {
                return 0
        }
-       conns, ok := t.idleConn[cacheKey]
-       if !ok {
-               return 0
+       for k, conns := range t.idleConn {
+               if k.String() == cacheKey {
+                       return len(conns)
+               }
        }
-       return len(conns)
+       return 0
 }
 
 func (t *Transport) IdleConnChMapSizeForTesting() int {

diff --git a/src/pkg/net/http/proxy_test.go b/src/pkg/net/http/proxy_test.go
index 449ccaeea760a4cb7266028258f65b8ca725b18e..d0726f61f3bf3e64fa21839661a1a23354da724e 100644 (file)
--- a/src/pkg/net/http/proxy_test.go
+++ b/src/pkg/net/http/proxy_test.go
@@ -71,8 +71,8 @@ func TestCacheKeys(t *testing.T) {
                        proxy = u
                }
                cm := connectMethod{proxy, tt.scheme, tt.addr}
-               if cm.String() != tt.key {
-                       t.Fatalf("{%q, %q, %q} cache key %q; want %q", tt.proxy, tt.scheme, tt.addr, cm.String(), tt.key)
+               if got := cm.key().String(); got != tt.key {
+                       t.Fatalf("{%q, %q, %q} cache key = %q; want %q", tt.proxy, tt.scheme, tt.addr, got, tt.key)
                }
        }
 }

diff --git a/src/pkg/net/http/transport.go b/src/pkg/net/http/transport.go
index 8fc7329e361f0915fbe966c59bf4e831ea493804..df171782357968a52bdd2615b909eafa881a268b 100644 (file)
--- a/src/pkg/net/http/transport.go
+++ b/src/pkg/net/http/transport.go
@@ -41,8 +41,8 @@ const DefaultMaxIdleConnsPerHost = 2
 // Transport can also cache connections for future re-use.
 type Transport struct {
        idleMu     sync.Mutex
-       idleConn   map[string][]*persistConn
-       idleConnCh map[string]chan *persistConn
+       idleConn   map[connectMethodKey][]*persistConn
+       idleConnCh map[connectMethodKey]chan *persistConn
        reqMu      sync.Mutex
        reqConn    map[*Request]*persistConn
        altMu      sync.RWMutex
@@ -281,17 +281,11 @@ func (e *envOnce) reset() {
        e.val = ""
 }
 
-func (t *Transport) connectMethodForRequest(treq *transportRequest) (*connectMethod, error) {
-       cm := &connectMethod{
-               targetScheme: treq.URL.Scheme,
-               targetAddr:   canonicalAddr(treq.URL),
-       }
+func (t *Transport) connectMethodForRequest(treq *transportRequest) (cm connectMethod, err error) {
+       cm.targetScheme = treq.URL.Scheme
+       cm.targetAddr = canonicalAddr(treq.URL)
        if t.Proxy != nil {
-               var err error
                cm.proxyURL, err = t.Proxy(treq.Request)
-               if err != nil {
-                       return nil, err
-               }
        }
        return cm, nil
 }
@@ -347,7 +341,7 @@ func (t *Transport) putIdleConn(pconn *persistConn) bool {
                }
        }
        if t.idleConn == nil {
-               t.idleConn = make(map[string][]*persistConn)
+               t.idleConn = make(map[connectMethodKey][]*persistConn)
        }
        if len(t.idleConn[key]) >= max {
                t.idleMu.Unlock()
@@ -367,7 +361,7 @@ func (t *Transport) putIdleConn(pconn *persistConn) bool {
 // getIdleConnCh returns a channel to receive and return idle
 // persistent connection for the given connectMethod.
 // It may return nil, if persistent connections are not being used.
-func (t *Transport) getIdleConnCh(cm *connectMethod) chan *persistConn {
+func (t *Transport) getIdleConnCh(cm connectMethod) chan *persistConn {
        if t.DisableKeepAlives {
                return nil
        }
@@ -375,7 +369,7 @@ func (t *Transport) getIdleConnCh(cm *connectMethod) chan *persistConn {
        t.idleMu.Lock()
        defer t.idleMu.Unlock()
        if t.idleConnCh == nil {
-               t.idleConnCh = make(map[string]chan *persistConn)
+               t.idleConnCh = make(map[connectMethodKey]chan *persistConn)
        }
        ch, ok := t.idleConnCh[key]
        if !ok {
@@ -385,7 +379,7 @@ func (t *Transport) getIdleConnCh(cm *connectMethod) chan *persistConn {
        return ch
 }
 
-func (t *Transport) getIdleConn(cm *connectMethod) (pconn *persistConn) {
+func (t *Transport) getIdleConn(cm connectMethod) (pconn *persistConn) {
        key := cm.key()
        t.idleMu.Lock()
        defer t.idleMu.Unlock()
@@ -404,7 +398,7 @@ func (t *Transport) getIdleConn(cm *connectMethod) (pconn *persistConn) {
                        // 2 or more cached connections; pop last
                        // TODO: queue?
                        pconn = pconns[len(pconns)-1]
-                       t.idleConn[key] = pconns[0 : len(pconns)-1]
+                       t.idleConn[key] = pconns[:len(pconns)-1]
                }
                if !pconn.isBroken() {
                        return
@@ -436,7 +430,7 @@ func (t *Transport) dial(network, addr string) (c net.Conn, err error) {
 // specified in the connectMethod.  This includes doing a proxy CONNECT
 // and/or setting up TLS.  If this doesn't return an error, the persistConn
 // is ready to write requests to.
-func (t *Transport) getConn(cm *connectMethod) (*persistConn, error) {
+func (t *Transport) getConn(cm connectMethod) (*persistConn, error) {
        if pc := t.getIdleConn(cm); pc != nil {
                return pc, nil
        }
@@ -471,7 +465,7 @@ func (t *Transport) getConn(cm *connectMethod) (*persistConn, error) {
        }
 }
 
-func (t *Transport) dialConn(cm *connectMethod) (*persistConn, error) {
+func (t *Transport) dialConn(cm connectMethod) (*persistConn, error) {
        conn, err := t.dial("tcp", cm.addr())
        if err != nil {
                if cm.proxyURL != nil {
@@ -634,20 +628,20 @@ type connectMethod struct {
        targetAddr   string   // Not used if proxy + http targetScheme (4th example in table)
 }
 
-func (ck *connectMethod) key() string {
-       return ck.String() // TODO: use a struct type instead
-}
-
-func (ck *connectMethod) String() string {
+func (cm *connectMethod) key() connectMethodKey {
        proxyStr := ""
-       targetAddr := ck.targetAddr
-       if ck.proxyURL != nil {
-               proxyStr = ck.proxyURL.String()
-               if ck.targetScheme == "http" {
+       targetAddr := cm.targetAddr
+       if cm.proxyURL != nil {
+               proxyStr = cm.proxyURL.String()
+               if cm.targetScheme == "http" {
                        targetAddr = ""
                }
        }
-       return strings.Join([]string{proxyStr, ck.targetScheme, targetAddr}, "|")
+       return connectMethodKey{
+               proxy:  proxyStr,
+               scheme: cm.targetScheme,
+               addr:   targetAddr,
+       }
 }
 
 // addr returns the first hop "host:port" to which we need to TCP connect.
@@ -668,11 +662,23 @@ func (cm *connectMethod) tlsHost() string {
        return h
 }
 
+// connectMethodKey is the map key version of connectMethod, with a
+// stringified proxy URL (or the empty string) instead of a pointer to
+// a URL.
+type connectMethodKey struct {
+       proxy, scheme, addr string
+}
+
+func (k connectMethodKey) String() string {
+       // Only used by tests.
+       return fmt.Sprintf("%s|%s|%s", k.proxy, k.scheme, k.addr)
+}
+
 // persistConn wraps a connection, usually a persistent one
 // (but may be used for non-keep-alive requests as well)
 type persistConn struct {
        t        *Transport
-       cacheKey string // its connectMethod.String()
+       cacheKey connectMethodKey
        conn     net.Conn
        closed   bool                // whether conn has been closed
        br       *bufio.Reader       // from conn