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

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 {

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)
 		}
 	}
 }

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